Stream: git-wasmtime

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


view this post on Zulip 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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 00:59):

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

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 00:59):

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

view this post on Zulip 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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

nit: debug_assert_eq! (gives better debug output)

view this post on Zulip 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.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Same remark about using test in both branches here.

view this post on Zulip 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:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 14:20):

bnjbvr created PR Review Comment:

Do you mind duplicating this test for brz please?

view this post on Zulip 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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:34):

cfallin submitted PR Review.

view this post on Zulip 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}.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin created PR Review Comment:

Yes, that is a bit clearer; updated!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:35):

cfallin created PR Review Comment:

Done.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin submitted PR Review.

view this post on Zulip 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).

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:37):

cfallin created PR Review Comment:

Done!

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:38):

cfallin submitted PR Review.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:38):

cfallin created PR Review Comment:

Done!

view this post on Zulip 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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:43):

bjorn3 submitted PR Review.

view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:45):

cfallin submitted PR Review.

view this post on Zulip 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.

view this post on Zulip 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

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

Please ensure all communication adheres to the code of conduct.
-->

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 22:46):

cfallin submitted PR Review.

view this post on Zulip 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.)

view this post on Zulip Wasmtime GitHub notifications bot (Jan 05 2021 at 23:32):

cfallin merged PR #2546.


Last updated: Dec 23 2024 at 12:05 UTC