github-actions[bot] commented on Issue #1954:
Subscribe to Label Action
cc @bnjbvr
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64"Thus the following users have been cc'd because of the following labels:
- bnjbvr: cranelift
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
cfallin commented on Issue #1954:
I'm concerned that this fix isn't at the right level in the system. What it does is to take an insn with (presumably) undefined semantics, eg
lsl w4, w5, #53
and emit it as some other insn that does have defined semantics, eg `lsl w4, w5, #1921I was skeptical at first, but it seems this is precisely defined both for WebAssembly and for CLIF:
in the Wasm spec, page 51, the "ishl" instruction is defined to "return the result of shifting ... left by k bits, modulo 2^N": i.e., we mask the shift amount.
in the CLIF instruction definition for ishl, the definition states "the shift amount is masked to the size of the register". That's a little ambiguous (size of the shiftee register or size of the shift-amount register?) but I take it to mean that a shift of an
i32
is masked to a 5-bit shift amount.So I think that we just define proper masking/modulo behavior at the Wasm level, and at the CLIF level, and translate it appropriately when we lower to machine instructions. (The
LSL
/LSR
instructions also appear to do the appropriate masking, so the translation for variable-amount shifts is correct as well.) So i think everything is correct as it stands, but I could be missing something!
julian-seward1 commented on Issue #1954:
So then out-of-range shifts have meaning both at the wasm and CLIR level, but not at the ARM64 level. Indeed, the ARM docs say the machine will trap on an out-of-range shift of a 32-bit value:
if sf == '0' && imm6<5> == '1' then ReservedValue(); ... ReservedValue(): if UsingAArch32() && !AArch32.GeneralExceptionsToAArch64() then AArch32.TakeUndefInstrException(); else AArch64.UndefinedFault();
I'm still of the opinion that the proposed fix is at the wrong level, though. It seems to me the right place for it is at the CLIR -> AArch64::Inst translation (lowering) point, not at the AArch64::Inst emission point. I'm sorry to be a nuisance about this. I really would prefer to avoid all "undefined semantics" in the overall pipeline, and an AArch64::Inst for an out-of-range 32-bit shift would be undefined, even though the existing patch "fixes" it at the emission point.
cfallin commented on Issue #1954:
Right, this is actually much simpler now that it's done in lowering -- thanks for persisting :-)
julian-seward1 commented on Issue #1954:
Sorry to have been a nuisance :-)
Last updated: Dec 23 2024 at 12:05 UTC