[Cranelift] add type-aware rotate operations#12764
[Cranelift] add type-aware rotate operations#12764bongjunj wants to merge 1 commit intobytecodealliance:mainfrom
Conversation
github-actions
bot
added
cranelift
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
cfallin
left a comment
cfallin
left a comment
There was a problem hiding this comment.
Thanks! A few thoughts below but the logic generally looks good to me.
| let x = (x.bits() as u64) & ty_mask; | ||
|
|
||
| // Mask off any excess rotate bits so the rotate stays within `ty`. | ||
| let shift_mask = bits - 1; |
There was a problem hiding this comment.
Can we put a debug_assert here that bits is a power of two? This is true for all of our types today but I don't want to bake the assumption in without something guarding it.
| let x = (x.bits() as u64) & ty_mask; | ||
|
|
||
| // Mask off any excess rotate bits so the rotate stays within `ty`. | ||
| let shift_mask = bits - 1; |
There was a problem hiding this comment.
Likewise here (debug-assert power-of-two).
| } | ||
|
|
||
| #[inline] | ||
| fn imm64_rotl(&mut self, ty: Type, x: Imm64, y: Imm64) -> Imm64 { |
There was a problem hiding this comment.
Now that we have these helpers in the prelude, we should be able to implement rotl/rotr constant propagation rules too, right? It seems odd to have just the rule in this PR (for rotr-of-select-of-constants) -- can we add simple cprop as well?
2 participants
Related to #12759. Rotation operations are sensitive to type width; therefore their semantics should change per the operands' type rather than being fixed to
u64,u32, and so on.