dialects: (x86) PR18 - m_pop + added rsp dataflow to push #2560
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2560 +/- ##
==========================================
- Coverage 89.77% 89.75% -0.03%
==========================================
Files 354 354
Lines 44663 44771 +108
Branches 6698 6717 +19
==========================================
+ Hits 40095 40182 +87
- Misses 3591 3606 +15
- Partials 977 983 +6 ☔ View full report in Codecov by Sentry. |
xdsl/dialects/x86/ops.py
Outdated
|
|
||
| rsp_input = operand_def(GeneralRegisterType("rsp")) | ||
| destination = operand_def( | ||
| R1InvT |
There was a problem hiding this comment.
This seems off, shouldn't it be a Generic parameter?
There was a problem hiding this comment.
Not sure what you mean? I define operands the same way in all the other instructions inheriting directly from IRDLOperation. It wouldn't let me pass Generic arguments iirc.
There was a problem hiding this comment.
Hmm, not sure how I missed this. This is a TypeVar, it's designed to be passed to Generic at the type level. When used like this, it's not any different to just passing the bound of the TypeVar, and possibly technically invalid Python, in my understanding. I'd say we should either make the base classes generic in their register types, or switch the operand constraints to pass the bound instead.
xdsl/dialects/x86/ops.py
Outdated
| rsp_input = operand_def(GeneralRegisterType("rsp")) | ||
| destination = operand_def( | ||
| R1InvT | ||
| ) # not an operand, but couldn't think of a better way to make it part of the IR |
There was a problem hiding this comment.
Can you please describe what's happening and what you're aming for a bit more?
There was a problem hiding this comment.
So the problem is that the value within that memory location doesn't matter so it's not really an operand. We did it in a similar way in the register version of pop. In this case however the destination is a memory access, not a register so no SSA value is created either. As such, I didn't see any way to include the information of what the memory access actually is in the IR because it doesn't fit neither operand_def nor result_def.
There was a problem hiding this comment.
The register holds a pointer, not a value, right? My understanding right now is that an operand is clearly the way to model this, although maybe I'm missing something. Could you maybe explain the relevant values in registers and memory before and after this instruction is executed? The link in the docstring is not very clear.
xdsl/dialects/x86/ops.py
Outdated
| @@ -300,59 +300,61 @@ class RR_MovOp(R_RR_Operation[GeneralRegisterType, GeneralRegisterType]): | |||
| name = "x86.rr.mov" | |||
|
|
|||
|
|
|||
| class M_R_Operation(Generic[R1InvT], IRDLOperation, X86Instruction, ABC): | |||
| @irdl_op_definition | |||
| class R_PushOp(IRDLOperation, X86Instruction, ABC): | |||
There was a problem hiding this comment.
This seems to not be an ABC any more
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Added the memory variant of the pop function and the dataflow of the rsp register for the push operations. Also reformatted a bit to remove unncecessary parent functions as im pretty sure pop and push are unique in their workings.
Do let me know if you know of a better way to do m.pop because the current way is a bit hacked