Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialects: (x86) PR5_2 - single operand instructions - one destination #2455

Merged
merged 4 commits into from
Apr 19, 2024
Merged

dialects: (x86) PR5_2 - single operand instructions - one destination #2455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fb4f4e
single destination ops + pop
KGrykiel Apr 12, 2024
6a21377
forgot assembly_emission
KGrykiel Apr 12, 2024
62a87fe
clarified docstring
KGrykiel Apr 12, 2024
42766fe
changed ROperationDst to also include side effects
KGrykiel Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions tests/filecheck/dialects/x86/x86_assembly_emission.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

%0 = x86.get_register : () -> !x86.reg<rax>
%1 = x86.get_register : () -> !x86.reg<rdx>
%rsp = x86.get_register : () -> !x86.reg<rsp>

%add = x86.add %0, %1 : (!x86.reg<rax>, !x86.reg<rdx>) -> !x86.reg<rax>
// CHECK: add rax, rdx
Expand All @@ -19,3 +20,5 @@
// CHECK: mov rax, rdx
x86.push %0 : (!x86.reg<rax>) -> ()
// CHECK: push rax
%pop, %poprsp = x86.pop %rsp : (!x86.reg<rsp>) -> (!x86.reg<rax>, !x86.reg<rsp>)
// CHECK: pop rax
3 changes: 3 additions & 0 deletions tests/filecheck/dialects/x86/x86_ops.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
// CHECK: %{{.*}} = "test.op"() : () -> !x86.reg<rax>

%0, %1 = "test.op"() : () -> (!x86.reg<>, !x86.reg<>)
%rsp = "test.op"() : () -> !x86.reg<rsp>

%add = x86.add %0, %1 : (!x86.reg<>, !x86.reg<>) -> !x86.reg<>
// CHECK: %{{.*}} = x86.add %{{.*}}, %{{.*}} : (!x86.reg<>, !x86.reg<>) -> !x86.reg<>
Expand All @@ -20,3 +21,5 @@
// CHECK-NEXT: %{{.*}} = x86.mov %{{.*}}, %{{.*}} : (!x86.reg<>, !x86.reg<>) -> !x86.reg<>
x86.push %0 : (!x86.reg<>) -> ()
// CHECK-NEXT: x86.push %{{.*}} : (!x86.reg<>)
%pop, %poprsp = x86.pop %rsp : (!x86.reg<rsp>) -> (!x86.reg<>, !x86.reg<rsp>)
// CHECK-NEXT: %{{.*}}, %{{.*}} = x86.pop %{{.*}} : (!x86.reg<rsp>) -> (!x86.reg<>, !x86.reg<rsp>)
2 changes: 2 additions & 0 deletions xdsl/dialects/x86/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
ImulOp,
MovOp,
OrOp,
PopOp,
PushOp,
SubOp,
XorOp,
Expand All @@ -24,6 +25,7 @@
XorOp,
MovOp,
PushOp,
PopOp,
GetRegisterOp,
],
[
Expand Down
42 changes: 42 additions & 0 deletions xdsl/dialects/x86/ops.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,48 @@ class PushOp(ROperationSrc[GeneralRegisterType]):
name = "x86.push"


class ROperationDst(Generic[R1InvT], SingleOperandInstruction):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This instruction doesn't have any operands, am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is by design although worth discussing. The pop function moves data from the top of the stack to a destination register. Whatever is inside the register is irrelevant, so I thought it best to only designate it as a result rather than an operand.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's exactly the right design, IMO, I don't see why inherit from that class

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, right, in that sense. I used the single/double/tripleOperandInstruction classes as a way to differentiate the instructions over how they look in assembly. In the sense how many arguments, whatever that may be, an assembly instruction takes. I suppose pop creates this kind of gray area where the assembly instruction takes an argument (i.e. "pop rax") but it's not really an operand.

In hindsight this might be a little confusing, but I'm not sure how best to handle the software engineering aspect of class hierarchy in this case. Now that I've implemented more instructions since first devising the plan, I couldn't find anything meaningful to put in the single/double/tripleOperandInstruction parent classes so perhaps it's best to get rid of them all together and have everything inherit from x86Instruction directly?

I'm fine with whatever you think is best tbh

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might as well get rid of them, we have another bunch of abstract class helpers for the inits already, so we might as well drop the marker classes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I opened a new issue for removing the parent classes (#2466), I'll do it later because now it would mess up merging for the other PR I have open.

Going forward with this, would you rather I categorize the instructions by the number of arguments the assembly instruction takes or by the actual number of operands including the intrinsic ones? In this example the 'push' instruction for instance could be an ROperation (because in the assembly it would look like 'push rdx') or an RR Operation (because it technically affects 2 registers). What are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't think of it as a big taxaonomy, the only reason we added it to riscv was to have less duplication of code for the initializers, I'd just group them by the init signature and not overthink it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think our current parent class naming convention (eg: add rax, rbx: RROperatio) only makes sense when one of the operands is also the result. IMO renaming the parent classes to reflect other cases as well would make more sense. Something like [Result]-[Operands]-Operation (eg: add rax, rbx: R-RR-Operation). So, push would technically be M-R-Operation (because it's dereferencing rsp)?

I think it would be helpful to de-sugar the assembly and keep the information explicitly in the SSA.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's also an option, i can change that in a fell swoop with the removal of the parent classes 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not, we can think of push and pop as a mov and do the same thing we do there. Whichever makes more sense.

"""
A base class for x86 operations that have one destination register.
"""

rsp_input = operand_def(GeneralRegisterType("rsp"))
destination = result_def(R1InvT)
rsp_output = result_def(GeneralRegisterType("rsp"))

def __init__(
self,
*,
comment: str | StringAttr | None = None,
rsp_input: Operation | SSAValue,
destination: R1InvT,
rsp_output: GeneralRegisterType,
):
if isinstance(comment, str):
comment = StringAttr(comment)

super().__init__(
operands=[rsp_input],
attributes={
"comment": comment,
},
result_types=[destination, rsp_output],
)

def assembly_line_args(self) -> tuple[AssemblyInstructionArg | None, ...]:
return (self.destination,)


@irdl_op_definition
class PopOp(ROperationDst[GeneralRegisterType]):
"""
Copies the value at the top of the stack into r1 and increases %rsp.
https://www.felixcloutier.com/x86/pop
"""

name = "x86.pop"


# region Assembly printing
def _append_comment(line: str, comment: StringAttr | None) -> str:
if comment is None:
Expand Down
Loading