cranelift: Use GPR newtypes extensively in x64 lowering #3798
Conversation
We already defined the `Gpr` newtype and used it in a few places, and we already defined the `Xmm` newtype and used it extensively. This finishes the transition to using the newtypes extensively in lowering by making use of `Gpr` in more places. Fixes bytecodealliance#3685
| dst_quotient: Writable::from_reg(regs::rax()), | ||
| dst_remainder: Writable::from_reg(regs::rdx()), | ||
| divisor: GprMem::new(divisor).unwrap(), | ||
| dividend: Gpr::new(regs::rax()).unwrap(), |
There was a problem hiding this comment.
Would it make sense to make regs::rax() and friends return a Gpr directly, and likewise regs::xmm0() and friends return an Xmm directly? This callsite gets simpler but others maybe gain a .to_reg(), so it's not necessarily any less verbose, but it is a bit more typesafe.
There was a problem hiding this comment.
I've avoided pushing the newtypes beyond the ISLE-glue boundary thus far. I think it would make sense to have one more PR (maybe I'll get to it while waiting on reviews for my components PRs) where we push the newtypes to all the Inst::add/etc methods and I think pushing that to regs::rax/etc in that same PR would make sense.
But I kinda don't want to do all that right now.
Does that sound good?
There was a problem hiding this comment.
Yep, that's reasonable, any incremental increase in typed-ness is good but no need to block on any part of it!
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:x64", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
…ance#3798) We already defined the `Gpr` newtype and used it in a few places, and we already defined the `Xmm` newtype and used it extensively. This finishes the transition to using the newtypes extensively in lowering by making use of `Gpr` in more places. Fixes bytecodealliance#3685
We already defined the
Gprnewtype and used it in a few places, and we alreadydefined the
Xmmnewtype and used it extensively. This finishes the transitionto using the newtypes extensively in lowering by making use of
Gprin moreplaces.
Fixes #3685