-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cranelift: Improve codegen of store_imm on x64 #6979
Conversation
Adds a new x64 rule for directly lowering stores of immediates with a MOV instruction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
There's a few tests which need to be updated which can be done with CRANELIFT_TEST_BLESS=1 cargo run test filetests
from the cranelift
directory. Otherwise though could you additionally add some *.clif
tests in the cranelift/filetests/filetests/isa/x64/
directory specifically for these lowering rules? I think it'd be good to showcase a range of constants being stored to ensure that they call get codegen'd correctly.
;; IMM stores | ||
(rule 2 (lower (store flags (has_type (fits_in_64 ty) (iconst (u64_from_imm64 value))) address offset)) | ||
(side_effect | ||
(x64_movimm_m ty (to_amode flags address offset) value))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're up for it I think it might be good to update the MovImmM
variant itself. Looking at emit.rs
it will panic if the low 32-bits of the value
here don't sign-extend into the full 64-bits, so the MovImmM
should I think store i32
instead of i64
. With that you could add extra matching here that value
fits within a 32-bit signed integer.
Thanks for the review. I've updated the Changing |
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "winch"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for this!
I::I64(v) => match v.try_into() { | ||
Ok(v) => self.asm.mov_im(v, &dst, size), | ||
Err(_) => { | ||
panic!("Immediate-to-memory moves require immediate operand to sign-extend to 64 bits."); | ||
} | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @saulecabrera, this is making the preexisting panic more explicit but it's one where I think the Err
case will want to be handled via a move-to-register then store instruction.
(in a future PR, no need to do anything here @mchesser)
* Improve lowering of store_imm on x64 Adds a new x64 rule for directly lowering stores of immediates with a MOV instruction. * Ensure that the MovImmM operand fits in an i32 and add tests. * Update winch to handle MovImmM change
* Improve lowering of store_imm on x64 Adds a new x64 rule for directly lowering stores of immediates with a MOV instruction. * Ensure that the MovImmM operand fits in an i32 and add tests. * Update winch to handle MovImmM change
Improve code generation of store_imm on x64 by adding a new rule that directly handles stores of small immediates.
Currently storing to constant to memory is achieved by first copying the constant into a temporary register, then performing a store of the register value to memory. For example:
Is currently compiled to:
On x86 constants can be stored directly to memory, e.g., after this patch, the constant is inlined into the store instruction removing the extra temporary register.
The overall performance impact is probably very minimal, but it could help with frontend/decoder pressure in cases where this pattern is common.
(Draft PR since this was partially an exercise in understanding ISLE so I could be missing something here).