Fixed width immediates #437
Conversation
kddnewton
requested review from
maximecb,
XrXr,
tenderlove and
k0kubun
as code owners
| /// Shorten a signed immediate to fit into a compile-time known width. | ||
| pub fn fix_signed_immediate<T: Into<i32>, const WIDTH: usize>(value: T) -> u32 { | ||
| let value: i32 = value.into(); | ||
| (value as u32) & ((1 << WIDTH) - 1) | ||
| } | ||
|
|
||
| /// Shorten an unsigned immediate to fit into a compile-time known width. | ||
| pub fn fix_unsigned_immediate<T: Into<u32>, const WIDTH: usize>(value: T) -> u32 { | ||
| let value: u32 = value.into(); | ||
| (value & ((1 << WIDTH) - 1)) | ||
| } |
There was a problem hiding this comment.
IIUC what these functions are doing is dropping the top bits, truncating integer values.
Maybe it would be better to name them truncate_imm() and truncate_uimm() ? Currently the name of these functions doesn't clearly indicate that they do IMO.
The other thing is, for an unsigned value, if the input already fits into the required number of bits, this should be a noop. For a signed value that is negative, it will change the value though.
It might be smart to add some assertions to make sure that:
- For an unsigned immediate, the top bits that we're not masking are all zeros (so basically, masked_output_value == input_value)
- For a signed immediate, if the value is positive, we're not dropping any bits, and for a negative value, the masked bits were all ones. There might be a simpler way to write that by saying, if you pad the negative masked value with all ones, you get the input value back.
There was a problem hiding this comment.
Yeah this makes a lot of sense, thanks for the detailed feedback. I updated per your suggestions.
kddnewton
force-pushed
the
fixed-width
branch
2 times, most recently
from
9b9d93c
to
4080a18
Compare
There are a lot of times when encoding AArch64 instructions that we need to represent an integer value with a custom fixed width. For example, the offset for a B instruction is 26 bits, so we store an i32 on the instruction struct and then mask it when we encode. We've been doing this masking everywhere, which has worked, but it's getting a bit copy-pasty all over the place. This commit centralizes that logic to make sure we stay consistent.
|
@noahgibbs @k0kubun we should cherry-pick this into the upstream merge as well |
|
Looks like this has already been cherry-picked into yjit_backend_ir. For the commit message, we do need to change the PR number in parens to a URL, but that's easy. The yjit_backend_ir commit (b5ae488) cherry-picks cleanly onto the PR branch (https://github.com/Shopify/ruby/tree/yjit_backend_ir_upstream) and builds cleanly. I'll wait until @k0kubun logs in before pushing anything to the branch. But this looks very straightforward. |
|
will do, thanks for the heads up! |
There are a lot of times when encoding AArch64 instructions that we need to represent an integer value with a custom fixed width. For example, the offset for a B instruction is 26 bits, so we store an i32 on the instruction struct and then mask it when we encode. We've been doing this masking everywhere, which has worked, but it's getting a bit copy-pasty all over the place. This commit centralizes that logic to make sure we stay consistent.
There are a lot of times when encoding AArch64 instructions that we need to represent an integer value with a custom fixed width. For example, the offset for a B instruction is 26 bits, so we store an i32 on the instruction struct and then mask it when we encode. We've been doing this masking everywhere, which has worked, but it's getting a bit copy-pasty all over the place. This commit centralizes that logic to make sure we stay consistent.
There are a lot of times when encoding AArch64 instructions that we need to represent an integer value with a custom fixed width. For example, the offset for a B instruction is 26 bits, so we store an i32 on the instruction struct and then mask it when we encode. We've been doing this masking everywhere, which has worked, but it's getting a bit copy-pasty all over the place. This commit centralizes that logic to make sure we stay consistent.
There are a lot of times when encoding AArch64 instructions that we
need to represent an integer value with a custom fixed width. For
example, the offset for a B instruction is 26 bits, so we store an
i32 on the instruction struct and then mask it when we encode.
We've been doing this masking everywhere, which has worked, but
it's getting a bit copy-pasty all over the place. This commit
centralizes that logic to make sure we stay consistent.
I also changed a couple of the tests, because I think I was handling
the negative signed values incorrectly in a couple of very very edge
cases.