Stream: git-wasmtime

Topic: wasmtime / PR #6915 riscv64: Refactor constant emission

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2023 at 16:55):

afonso360 opened PR #6915 from afonso360:riscv-fix-imm to bytecodealliance:main:

:wave: 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2023 at 16:55):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #6915.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 26 2023 at 16:55):

afonso360 requested elliottt for a review on PR #6915.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp submitted PR review:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp created PR review comment:

It looks like this can only return either None or Some(Imm20::from_bits(0)). Perhaps it was supposed to say (bits & !0xF_FFFF) != 0, with a ! in front of the 0x? Alternatively, (bits >> 20) != 0 should work, although I kind of like the symmetry with Imm20::from_bits.

It looks like u64_to_imm20 is the only user of this method and is not used anywhere, which would explain this slipping through unnoticed.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 28 2023 at 20:49):

jameysharp created PR review comment:

I think your maybe_from_i64 is a simpler way to express exactly the same result that the existing maybe_from_u64 would produce given the appropriate type conversion between u64 and i64. Would you mind making maybe_from_u64 just cast to i64 and call the new function? Alternatively, is maybe_from_u64 still actually used after the rest of these changes, or can it be deleted?

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 08:16):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 08:16):

afonso360 created PR review 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)))

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 11:40):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 11:40):

afonso360 created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 11:40):

afonso360 updated PR #6915.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 12:03):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 15:27):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 15:27):

jameysharp created PR review 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.

view this post on Zulip Wasmtime GitHub notifications bot (Aug 29 2023 at 19:12):

afonso360 merged PR #6915.

Last updated: Dec 23 2024 at 12:05 UTC