dialects: (x86) - Reformats and changes in naming convention #2487
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2487 +/- ##
==========================================
- Coverage 89.81% 89.78% -0.03%
==========================================
Files 350 350
Lines 42902 43002 +100
Branches 6371 6392 +21
==========================================
+ Hits 38531 38609 +78
- Misses 3427 3447 +20
- Partials 944 946 +2 ☔ View full report in Codecov by Sentry. |
| @@ -4,23 +4,23 @@ | |||
| %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> | |||
| %add = x86.rr_add %0, %1 : (!x86.reg<rax>, !x86.reg<rdx>) -> !x86.reg<rax> | |||
There was a problem hiding this comment.
Why this change? I thought the idea was to just change the abstract base class names? Since the operands and results are clearly denoted already, I'm not sure what this mnemonic adds
There was a problem hiding this comment.
Ah, I see, it's for the cases when the same assembly instruction can have multiple configurations. Is that possible for all the operations changed in this diff? Can you give me an example IR that mixes multiple versions of the same assembly instruction?
There was a problem hiding this comment.
Yeah, it's for the different configurations, all of the operations in the current version have several different configurations. Here's an example snippet from a matrix multiplication program that mixes them up a bit:
%20 = x86.rm_add %19, %rdx, 12: (!x86.reg<rax>, !x86.reg<rdx>) -> !x86.reg<rax>
x86.mr_mov %rdx, %20, 12 : (!x86.reg<rdx>, !x86.reg<rax>) -> ()
%21 = x86.rm_mov %17, %rsi, 12: (!x86.reg<rcx>, !x86.reg<rsi>) -> !x86.reg<rcx>
%22 = x86.rm_imul %21, %rdi, 12: (!x86.reg<rcx>, !x86.reg<rdi>) -> !x86.reg<rcx>
%23 = x86.rr_add %22, %20: (!x86.reg<rcx>, !x86.reg<rax>) -> !x86.reg<rcx>
x86.mr_mov %rdx, %23, 12 : (!x86.reg<rdx>, !x86.reg<rcx>) -> ()
which corresponds to:
add rax, [rdx + 12]
mov [rdx + 12], rax
mov rcx, [rsi + 12]
imul rcx, [rdi + 12]
add rcx, rax
mov [rdx + 12], rcx
My solution to handling memory accesses might be a bit experimental, but we'll get to that in the next PRs lol
There was a problem hiding this comment.
In that case, for the time being, I'd probably recommend making a small change to x86.rm.add instead of the underscore, the lexer in my head breaks on . more easilty than _, hopefully making the IR a bit more readable. It should also make it easier to handle the assembly instruction printing, it'll be the last component in the name split on .
xdsl/dialects/x86/ops.py
Outdated
| @@ -152,7 +152,7 @@ def assembly_instruction_name(self) -> str: | |||
|
|
|||
| def assembly_line(self) -> str | None: | |||
| # default assembly code generator | |||
| instruction_name = self.assembly_instruction_name() | |||
| instruction_name = self.assembly_instruction_name().split("_", 1)[-1] | |||
There was a problem hiding this comment.
I'd recommend leaving this line as-is, and replacing the _ by .
I've done some changes to the naming conventions following the suggestions of @tarinduj. This includes removing the single/doubleOperand parent classes (issue #2466), writing the class names in the format [Result]-[Operands]-Operation, and changing the individual names of the operations to prepare for the different configurations those instructions can exist in. The prefix signifies the type of the operand passed into the assembly instruction and is removed during assembly emission.