Wasmtime GitHub notifications bot (Aug 23 2022 at 14:29): Wasmtime GitHub notifications bot (Aug 23 2022 at 14:29):afonso360 opened PR #4752 from x64-fix-shifts to main:
This was reported by @bjorn3 in https://github.com/bjorn3/rustc_codegen_cranelift/pull/1268#issuecomment-1223916783 when we tried to remove the explicit shift amount masking done by
cg_clif.As a consequence of this fix, #4699 is also fixed, which is nice!
This PR also does some other housekeeping:
- Enables
i128shifts in the fuzzer- Enables the
i128-shifts-small-types.clifruntests and moves them toi128-shifts.clif- Adds run tests that trigger the issue above (which is similar, but not the same as the
i128one)- Adds compile tests for all forms of
ishl/sshr/ushrFixes: #4699
cc: @elliottt
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:28):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:28):elliottt submitted PR review.
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:28):elliottt created PR review comment:
Was this priority necessary for correct output? I think it should be fine to omit it.
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:40):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:40):afonso360 created PR review comment:
Not really, but I've had issues with priorities, so now I'm a bit paranoid about it. I'll remove it
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:44):afonso360 updated PR #4752 from x64-fix-shifts to main.
Wasmtime GitHub notifications bot (Aug 23 2022 at 16:48):afonso360 updated PR #4752 from x64-fix-shifts to main.
Wasmtime GitHub notifications bot (Aug 23 2022 at 17:07):jameysharp has enabled auto merge for PR #4752.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:39):jameysharp submitted PR review.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:39):jameysharp created PR review comment:
I want to see if I understand why this PR fixes this issue. Could you confirm or correct my interpretation?
It looks like there are two bugs in
put_masked_in_imm8_gpr, and I gather that these bugs aren't present in the similar-looking ISLE rules.If the shift amount is not defined by an
iconstinstruction, then this function doesn't mask it at all...? The only thing it does is, if the shift amount needs two registers because it's an i128, then it only takes the lower 64-bit register.If it is a constant, then it's masked by
(1 << ty.bits()) - 1. So for an 8-bit LHS, the shift amount is used modulo 256, but it's actually supposed to be modulo 8. For a 64-bit LHS the shift amount is effectively not masked at all. By contrast, theshift_maskfunction just returnsty.lane_bits() - 1so it gets this right. (And also handles vector types correctly, I guess?)I guess the reason these bugs weren't obvious sooner is that wasm only uses i32 and i64 types, and x86 already masks 32-bit and 64-bit shift amounts to 5 and 6 bits, respectively. But narrower 8-bit and 16-bit shifts are also masked to 5 bits on x86, so those were wrong.
Now that I think I understand this, I'm wondering how hard it is in ISLE to avoid emitting the
andinstruction when the type is i32 or i64.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:40):jameysharp has disabled auto merge for PR #4752.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:55):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 23 2022 at 18:55):afonso360 created PR review comment:
If the shift amount is not defined by an iconst instruction, then this function doesn't mask it at all...? The only thing it does is, if the shift amount needs two registers because it's an i128, then it only takes the lower 64-bit register.
Yes, the source of the issue I was trying to fix was exactly this.
It looks like there are two bugs in put_masked_in_imm8_gpr, and I gather that these bugs aren't present in the similar-looking ISLE rules.
Wow! I was just trying to fix the unmasked non const one, didn't actually realize that the const case didn't do the right thing either!
We should probably add some
shift_immtest cases as well.Now that I think I understand this, I'm wondering how hard it is in ISLE to avoid emitting the and instruction when the type is i32 or i64.
I think that's a good improvement, i'll push it soon.
Wasmtime GitHub notifications bot (Aug 23 2022 at 19:00):afonso360 edited PR review comment.
Wasmtime GitHub notifications bot (Aug 24 2022 at 10:05):afonso360 updated PR #4752 from x64-fix-shifts to main.
Wasmtime GitHub notifications bot (Aug 24 2022 at 10:06):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 24 2022 at 10:06):afonso360 created PR review comment:
And it turns out the const case was also wrong. Great Catch!
Wasmtime GitHub notifications bot (Aug 24 2022 at 10:21):afonso360 updated PR #4752 from x64-fix-shifts to main.
Wasmtime GitHub notifications bot (Aug 24 2022 at 17:31):jameysharp merged PR #4752.
Last updated: Dec 23 2024 at 12:05 UTC