wasmtime / PR #2546 x64: handle tests of b1 values correc... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2546 x64: handle tests of b1 values correc...

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:59):

cfallin opened PR #2546 from fix-b1 to main:

Previously, select and brz/brnz instructions, when given a b1
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its Extend pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:59):

cfallin requested abrown and bnjbvr for a review on PR #2546.

Wasmtime GitHub notifications bot (Jan 05 2021 at 00:59):

cfallin requested abrown and bnjbvr for a review on PR #2546.

Wasmtime GitHub notifications bot (Jan 05 2021 at 04:25):

cfallin updated PR #2546 from fix-b1 to main:

Previously, select and brz/brnz instructions, when given a b1
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its Extend pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Would this read slightly better as if is_cmp { if *size == 1 { 0x80 } else if use_imm8 { 0x83 } else { 0x81 } } else if *size == 1 { 0xf6 } else { 0xf7 } ?

/me also wonders if rustc/llvm can rearrange the order of the comparisons here, so that is_cmp is checked only once in generated code.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

nit: debug_assert_eq! (gives better debug output)

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

uber-nit: could be an int or boolean, but the size value matches the type size in this case.

We could in fact use a test instruction against zero here. If so, the whole if can select the immediate to test against, slightly reducing code duplication here.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Pre-existing: in the above inst definition for CmpRmiR, can you remove /tests from the comment? it's a bit misleading considering the new variant.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Same remark about using test in both branches here.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

In fact, do we need the new variant TestRmiR? I had in mind that we needed to add new instructions when no other instructions matched the following:

regalloc constraints are the same

instruction operands are the same

codegen is (almost) the same

In this case, it seems that CmpRmiR has the same behavior for the three above, so we could add one boolean field there meaning is_test, and have one fewer enum variant in return. We can absolutely keep the constructor test_rmi_r though, as an higher-level helper.

Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Do you mind duplicating this test for brz please?

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:30):

cfallin updated PR #2546 from fix-b1 to main:

Previously, select and brz/brnz instructions, when given a b1
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its Extend pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:34):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:34):

cfallin created PR Review Comment:

Yes, that's a better idea -- merged this in and added CmpOpcode::{Test, Cmp}.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin created PR Review Comment:

Yes, that is a bit clearer; updated!

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin created PR Review Comment:

Done.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin created PR Review Comment:

Re: the nit -- yes, good point; it could be a wider-than-1-bit bool. I'm not sure what that would mean as an input to a select or brz/brnz, so I've just added an assert that we do not see another bool type in the else-branch.

Re: using test always -- yes, good point; actually test rN, rN is one byte shorter than cmp rN, 0 so this is a code-size win too. (rN, rN and not rN, 0 because this is an and-op; and 0xff...ff is much larger in emitted code than the R-R form).

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:38):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:38):

cfallin created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:40):

cfallin updated PR #2546 from fix-b1 to main:

Previously, select and brz/brnz instructions, when given a b1
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its Extend pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:43):

bjorn3 submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:43):

bjorn3 created PR Review Comment:

Please revert this. I am pretty sure that cranelif-module doesn't disable default features and as such it would become impossible to use the old backend.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:45):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:45):

cfallin created PR Review Comment:

Eek, this slipped in from local testing. Thanks for catching this.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:45):

cfallin updated PR #2546 from fix-b1 to main:

Previously, select and brz/brnz instructions, when given a b1
boolean argument, would test whether that boolean argument was nonzero,
rather than whether its LSB was nonzero. Since our invariant for mapping
CLIF state to machine state is that bits beyond the width of a value are
undefined, the proper lowering is to test only the LSB.

(aarch64 does not have the same issue because its Extend pseudoinst
already properly handles masking of b1 values when a zero-extend is
requested, as it is for select/brz/brnz.)

Found by Nathan Ringo on Zulip [1] (thanks!).

[1]
https://bytecodealliance.zulipchat.com/#narrow/stream/217117-cranelift/topic/bnot.20on.20b1s

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:46):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Jan 05 2021 at 22:46):

cfallin created PR Review Comment:

(I think that's why I was newly seeing a warning issue; CI doesn't otherwise check for warnings with non-default options.)

Wasmtime GitHub notifications bot (Jan 05 2021 at 23:32):

cfallin merged PR #2546.

Last updated: Oct 23 2024 at 20:03 UTC