x64: Migrate mulx to the new assembler
#10887
Conversation
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "cranelift:meta", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
| // opposed to when the operands are different to calculate both the high | ||
| // and low halves. | ||
| inst("mulxl", fmt("RVM", [w(r32a), w(r32b), r(rm32), r(implicit(edx))]), vex(LZ)._f2()._0f38().w0().op(0xF6), _64b | compat | bmi2).custom_visit(), | ||
| inst("mulxq", fmt("RVM", [w(r64a), w(r64b), r(rm64), r(implicit(rdx))]), vex(LZ)._f2()._0f38().w1().op(0xF6), _64b | bmi2).custom_visit(), |
There was a problem hiding this comment.
I really like this custom* idea: I'd been mulling over how to do custom encodings for nop and custom printing for comparisons and felt we needed an escape hatch for very special instructions. And I like that we just expect custom::visit_{inst} to be present, which should be pretty clear in compile errors.
I don't know if you were thinking this way, but maybe in the future we can go to .custom(Visit | Encode | Display) which would result in an appropriate call to custom::visit::{inst}, custom::encode::{inst}, and custom::display::{inst}. Thoughts?
There was a problem hiding this comment.
I think that's reasonable yeah!
| (_ Unit (emit (MInst.MulX size src1 src2 (writable_invalid_gpr) dst)))) | ||
| dst)) | ||
| (rule (x64_mulx_hi $I32 src1 src2) (x64_mulxl_rvm_hi src2 src1)) | ||
| (rule (x64_mulx_hi $I64 src1 src2) (x64_mulxq_rvm_hi src2 src1)) |
There was a problem hiding this comment.
I guess there's no way to pass the same temporary register in... I wonder if eventually we need to allow even more fine-grained control from ISLE — a few new gen_asm.rs rules?
There was a problem hiding this comment.
I was wondering the same yeah, but I figured that if it's just mulx then there's no need to go to such lengths, so I figured I'd leave it as-is for now. If the number of instructions needing this grows though I can see how making new ISLE helpers would be reasonable
This is an interesting instruction as it has a relatively unique shape compared to many others. The VEX encoding is used to give it a 3-operand form, although it still has an implicit 4th operand as well. The other unique part about this instruction is that if the two write-only operands are the same then that has a different semantic meaning than if they are different. Modeling the two-output form of the instruction was pretty easy, the only changes needed were to add the `r32a` and `r32b` locations as previously only `r32` was available. Modeling the one-output form of the instruction led to a "hook" where these instructions specify that they use a custom regalloc function. That skips the auto-generated regalloc entirely and defers to a new `custom` submodule in the assembler crate. These custom variants handle only having a single write operand on the instruction.
This is an interesting instruction as it has a relatively unique shape compared to many others. The VEX encoding is used to give it a 3-operand form, although it still has an implicit 4th operand as well. The other unique part about this instruction is that if the two write-only operands are the same then that has a different semantic meaning than if they are different.
Modeling the two-output form of the instruction was pretty easy, the only changes needed were to add the
r32aandr32blocations as previously onlyr32was available. Modeling the one-output form of the instruction led to a "hook" where these instructions specify that they use a custom regalloc function. That skips the auto-generated regalloc entirely and defers to a newcustomsubmodule in the assembler crate. These custom variants handle only having a single write operand on the instruction.