riscv64: Refactor constant emission #6915
Conversation
This better fits the existing naming convention of constructors being `x_to_y` and extractors being `y_from_x` See `u64_to_uimm5` or `uimm5_from_u8` as examples.
It is no longer used.
Instead of taking a closure, return an Option with the split values
This moves the constant emission rules into isle and cleans up some of the existing constant emission code. Further work is still required
Subscribe to Label ActionDetailsThis issue or pull request has been labeled: "cranelift", "cranelift:area:machinst", "cranelift:area:riscv64", "isle"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
jameysharp
left a comment
There was a problem hiding this comment.
This looks great, for the most part! I feel much better about this infrastructure with these changes.
I think Imm20::maybe_from_u64 is incorrect and also unused, although I'd suggest using it from generate_imm so I think it's worth fixing it. However if you want to just delete it in this PR and maybe add a corrected version in a later PR, that's fine with me.
| (rv_vwadd_vx x y (unmasked) (vstate_mf2 (ty_half_lanes in_ty)))) | ||
|
|
||
| (rule 14 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (sextend x @ (value_type sext_ty))) | ||
| (rule 15 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (sextend x @ (value_type sext_ty))) |
There was a problem hiding this comment.
I can't see from the diff of the "Sign extend replicated_imm5 immediates" commit why these rules needed a higher priority; I don't see another rule at priority 14 that these rules could conflict with.
There was a problem hiding this comment.
I don't know why that overlap showed up either. But it does show up when I switch the definition of replicated_imm5. The overlaps that ISLE reports are:
src/isa/riscv64/lower.isle:139:7: (rule 14 (lower (has_type (ty_vec_fits_in_register ty) (iadd (replicated_imm5 x) y)))
src/isa/riscv64/lower.isle:204:7: (rule 14 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (uextend x @ (value_type uext_ty)))
src/isa/riscv64/lower.isle:159:7: (rule 14 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (sextend x @ (value_type sext_ty)))
src/isa/riscv64/lower.isle:182:7: (rule 14 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (sextend x @ (value_type sext_ty)))
src/isa/riscv64/lower.isle:227:7: (rule 14 (lower (has_type (ty_vec_fits_in_register _) (iadd (splat (uextend y @ (value_type uext_ty)))
There was a problem hiding this comment.
Ah, now I understand. Previously replicated_imm5 matched iconst and ISLE could tell that's a different enum variant than splat. Now it matches an external constructor, which ISLE has to assume could match splat, even though we know it doesn't. I don't see a reasonable way to rewrite that to give ISLE more information, so changing the priorities like you've done here is the only option.
| Some(( | ||
| Imm20::from_bits(imm20 as i32), | ||
| Imm12::from_bits(imm12 as i16), | ||
| )) |
There was a problem hiding this comment.
I appreciate that this is a pretty minimal change, but looking at the original function I kind of really want to rewrite the whole thing. Something along these lines:
// sign-extend the low 12 bits: this is the value we can represent with an `addi` immediate operand
const SIGN_BITS: u8 = 64 - 12;
let imm12 = (value as i64) << SIGN_BITS >> SIGN_BITS;
// compute the inverse of `addi` to see what part we need to use `lui` for;
// note that by construction the low 12 bits are always 0 so it's safe to shift them out
let imm20 = value.wrapping_sub(imm12 as u64) >> 12;
// check that the imm20 part fits; note that by construction the imm12 always fits
Imm20::maybe_from_u64(imm20).map(|imm20| (imm20, Imm12::from_bits(imm12 as i16)))I think that should correctly replace everything, including the check for imm_min/imm_max and the check for whether an imm12 is enough.
There was a problem hiding this comment.
Yeah, I was trying to avoid changing that function since it ends up being slightly complicated to write. And that version is a lot cleaner.
I think it might be failing in a couple edge cases since some tests stop working with it, and I don't really feel like looking into it now, but I'm going to file an issue to follow up on it later (#6922).
Use `Imm12::maybe_from_i64` where it is more ergonomic
* riscv64: Sign extend imm12 values * riscv64: Rename `imm5_to_i8` constructor This better fits the existing naming convention of constructors being `x_to_y` and extractors being `y_from_x` See `u64_to_uimm5` or `uimm5_from_u8` as examples. * riscv64: Sign extend `replicated_imm5` immediates * riscv64: Delete `neg_imm12` helper * riscv64: Delete `construct_imm_sub_rs` It is no longer used. * riscv64: Refactor `Inst::generate_imm` Instead of taking a closure, return an Option with the split values * riscv64: Rework constant emission This moves the constant emission rules into isle and cleans up some of the existing constant emission code. Further work is still required * riscv64: Fix imm matching for `insertlane` * riscv64: Misc cleanups * riscv64: Refactor uses of `Imm12::maybe_from_u64` Use `Imm12::maybe_from_i64` where it is more ergonomic * riscv64: Delete `Imm20::maybe_from_u64`
👋 Hey,
This PR fixes the RISC-V issues that #6850 exposed. It does a bit of a refactor in our constant emission infrastructure, and also moves some of it into ISLE.
The issues that that PR exposed are that we sometimes would match or not match a particular constant generation strategy based on the upper bits that shouldn't really matter. The main fix is to sign extend constants before trying to match them.
Moving the constant emission into ISLE also means that we have a bunch of neutral regalloc changes. There are a few cases where we have actual changes in the instructions emitted.
This PR contains a lot of stuff, and I would recommend reviewing it commit by commit.