Stream: git-wasmtime

Topic: wasmtime / Issue #1954 AArch64: fix shift ops: mask shift...

Wasmtime GitHub notifications bot (Jul 01 2020 at 00:59):

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>

Wasmtime GitHub notifications bot (Jul 01 2020 at 06:27):

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, #1921

I 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!

Wasmtime GitHub notifications bot (Jul 01 2020 at 09:49):

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.

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:01):

cfallin commented on Issue #1954:

Right, this is actually much simpler now that it's done in lowering -- thanks for persisting :-)

Wasmtime GitHub notifications bot (Jul 01 2020 at 16:19):

julian-seward1 commented on Issue #1954:

Sorry to have been a nuisance :-)

Last updated: Oct 23 2024 at 20:03 UTC