cranelift: Remove booleans #5031

elliottt · 2022-10-07T00:33:35Z

Remove the boolean types from cranelift, and the associated instructions breduce, bextend, bconst, and bint. Standardize on using 1/0 for the return value from instructions that produce scalar boolean results, and -1/0 for boolean vector elements.

Fixes #3205

cranelift/codegen/src/ir/extfunc.rs

bjorn3 · 2022-10-07T08:26:27Z

cranelift/codegen/src/isa/aarch64/lower/isle.rs

@@ -164,7 +164,6 @@ impl Context for IsleContext<'_, '_, MInst, Flags, IsaFlags, 6> {
    fn integral_ty(&mut self, ty: Type) -> Option<Type> {
        match ty {
            I8 | I16 | I32 | I64 | R64 => Some(ty),


This should list R32 too, right?

Probably, though I'm not sure we should address that on this branch. Would you like to add it in a separate PR?

bjorn3 · 2022-10-07T08:29:33Z

cranelift/filetests/filetests/runtests/bextend.clif

-  v1 = bextend.b32 v0
+function %bextend_i16_i32(i16) -> i32 {
+block0(v0: i16):
+  v1 = bextend.i32 v0
  return v1


Can bextend be replaced with uextend (for non-vector bools) and sextend (for vector bools)?

I'd be in favor of removing bextend and breduce. The latter can also be replaced by ireduce.

Yep, if we no longer have bools then these instructions no longer make sense; the user can zero- or sign-extend an integer (which happens to be a 0/1 or 0/-1 "truthy" value) in the usual ways.

I really like using uextend/sextend in place of bextend, and have made that refactoring. I've also removed bconst, bextend, breduce and bint, which has collapsed a lot of cases in the test suite.

afonso360 · 2022-10-07T15:35:56Z

I'm really happy that we're finally doing this! Thank you for taking this on.

Some of our lowerings depend on the fact that bools can only be represented as all ones or all zeroes, and so now that we are removing that assumption we might want to revisit them.

The AArch64 implementation for bmask is wrong if we want to accept any integer other than -1 and 0. I would expect to get -1 if I do a bmask on a 2, but that's not what's going to happen. (I haven't tested this, but that's my reading of the implementation).

bint is also assuming that inputs are all ones or all zeros and is implemented as and_imm on AArch64. (i.e. 2 will be false, but should be true)

Edit: AArch64 has instructions (cset and csetm) that represent these two operations, so we may want to partially revert b8f6d53 to get them back

cfallin

Wow -- thank you for doing the not-very-flashy but very important cleanup work here! It's surprising to me, but validating of this direction, that there are so many little corner cases that this removes.

Some scattered thoughts below; I read fairly carefully early in the diff, but my eyes started to glaze over partway through the test-output deltas, so I'll want to look at those some more in another pass. Hopefully this is good to get started though.

cfallin · 2022-10-07T23:23:33Z

cranelift/codegen/meta/src/shared/instructions.rs

@@ -685,7 +681,7 @@ pub(crate) fn define(
    let iflags: &TypeVar = &ValueType::Special(types::Flag::IFlags.into()).into();
    let fflags: &TypeVar = &ValueType::Special(types::Flag::FFlags.into()).into();

-    let b1: &TypeVar = &ValueType::from(LaneType::from(types::Bool::B1)).into();
+    let b1: &TypeVar = &ValueType::from(LaneType::from(types::Int::I8)).into();


Do we still need a typevar named b1 at all? Is this used for "truthy" things like icmp return values etc? If so, perhaps we could call it truthy or somesuch?

I renamed it to i8, as that should hopefully give good documentation for the places that it's used.

cfallin · 2022-10-07T23:25:26Z

cranelift/codegen/src/ir/types.rs

@@ -157,7 +152,7 @@ impl Type {
    /// Scalar types are all converted to `b1` which is usually what you want.
    pub fn as_bool(self) -> Self {


Perhaps we could remove the .as_bool() helper altogether?

How would you feel about renaming it to .as_truthy() instead? It does convert types like F32 to I32, so it would be convenient to keep something like it around.

cranelift/codegen/src/prelude.isle

cranelift/codegen/src/verifier/mod.rs

cranelift/filetests/filetests/isa/aarch64/condbr.clif

cfallin · 2022-10-07T23:40:48Z

Also, just to note it here: if at all possible, I'd prefer for us to wait to merge this until after my mid-end work in #4953 is merged, if only because they're both large and likely-to-conflict PRs and mine has been through ~three painful rebases already :-)

cranelift/codegen/src/isa/aarch64/inst.isle

elliottt · 2022-10-13T15:25:18Z

Following up on myself, it would be even simpler and better to use the existing value_nonzero helper, which will fold in previous comparisons if possible:
 (rule (lower (has_type ty (bmask x)))
       (lower_bool_to_mask ty (value_nonzero x)))

This is much nicer! I'll rework the changes to add lower_bool_to_mask instead.

uweigand · 2022-10-13T17:13:25Z

This is much nicer! I'll rework the changes to add lower_bool_to_mask instead.

Thanks! The s390x parts LGTM now.

Remove the boolean types from cranelift, and the associated instructions "breduce", "bextend", "bconst", and "bint". Standardize on using `1`/`0` for the return value from instructions that produce scalar boolean results, and `-1`/`0` for boolean vector elements. Co-authored-by: Afonso Bordado <afonso360@users.noreply.github.com>

Co-authored-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>

Co-authored-by: Afonso Bordado <afonso360@users.noreply.github.com>

Co-authored-by: Chris Fallin <chris@cfallin.org>

In bytecodealliance#5031, we removed `bool` types from CLIF, using integers instead for "truthy" values. This greatly simplified the IR, and was generally an improvement. However, because x86's `SETcc` instruction sets only the low 8 bits of a register, we chose to use `i8` types as the result of `icmp` and `fcmp`, to avoid the need for a masking operation when materializing the result. Unfortunately this means that uses of truthy values often now have `uextend` operations, especially when coming from Wasm (where truthy values are naturally `i32`-typed). For example, where we previously had `(brz (icmp ...))`, we now have `(brz (uextend (icmp ...)))`. It's arguable whether or not we should switch to `i32` truthy values -- in most cases we can avoid materializing a value that's immediately used for a branch or select, so a mask would in most cases be unnecessary, and it would be a win at the IR level -- but irrespective of that, this change *did* regress our generated code quality: our backends had patterns for e.g. `(brz (icmp ...))` but not with the `uextend`, so we were *always* materializing truthy values. Many blocks thus ended with "cmp; setcc; cmp; test; branch" rather than "cmp; branch". In bytecodealliance#5391 we noticed this and fixed it on x64, but it was a general problem on aarch64 and riscv64 as well. This PR introduces a `maybe_uextend` extractor that "looks through" uextends, and uses it where we consume truthy values, thus fixing the regression. This PR also adds compile filetests to ensure we don't regress again.

In bytecodealliance#5031, we removed `bool` types from CLIF, using integers instead for "truthy" values. This greatly simplified the IR, and was generally an improvement. However, because x86's `SETcc` instruction sets only the low 8 bits of a register, we chose to use `i8` types as the result of `icmp` and `fcmp`, to avoid the need for a masking operation when materializing the result. Unfortunately this means that uses of truthy values often now have `uextend` operations, especially when coming from Wasm (where truthy values are naturally `i32`-typed). For example, where we previously had `(brz (icmp ...))`, we now have `(brz (uextend (icmp ...)))`. It's arguable whether or not we should switch to `i32` truthy values -- in most cases we can avoid materializing a value that's immediately used for a branch or select, so a mask would in most cases be unnecessary, and it would be a win at the IR level -- but irrespective of that, this change *did* regress our generated code quality: our backends had patterns for e.g. `(brz (icmp ...))` but not with the `uextend`, so we were *always* materializing truthy values. Many blocks thus ended with "cmp; setcc; cmp; test; branch" rather than "cmp; branch". In bytecodealliance#5391 we noticed this and fixed it on x64, but it was a general problem on aarch64 and riscv64 as well. This PR introduces a `maybe_uextend` extractor that "looks through" uextends, and uses it where we consume truthy values, thus fixing the regression. This PR also adds compile filetests to ensure we don't regress again. The riscv64 backend has not been updated here because doing so appears to trigger another issue in its branch handling; fixing that is TBD.

#5487) In #5031, we removed `bool` types from CLIF, using integers instead for "truthy" values. This greatly simplified the IR, and was generally an improvement. However, because x86's `SETcc` instruction sets only the low 8 bits of a register, we chose to use `i8` types as the result of `icmp` and `fcmp`, to avoid the need for a masking operation when materializing the result. Unfortunately this means that uses of truthy values often now have `uextend` operations, especially when coming from Wasm (where truthy values are naturally `i32`-typed). For example, where we previously had `(brz (icmp ...))`, we now have `(brz (uextend (icmp ...)))`. It's arguable whether or not we should switch to `i32` truthy values -- in most cases we can avoid materializing a value that's immediately used for a branch or select, so a mask would in most cases be unnecessary, and it would be a win at the IR level -- but irrespective of that, this change *did* regress our generated code quality: our backends had patterns for e.g. `(brz (icmp ...))` but not with the `uextend`, so we were *always* materializing truthy values. Many blocks thus ended with "cmp; setcc; cmp; test; branch" rather than "cmp; branch". In #5391 we noticed this and fixed it on x64, but it was a general problem on aarch64 and riscv64 as well. This PR introduces a `maybe_uextend` extractor that "looks through" uextends, and uses it where we consume truthy values, thus fixing the regression. This PR also adds compile filetests to ensure we don't regress again. The riscv64 backend has not been updated here because doing so appears to trigger another issue in its branch handling; fixing that is TBD.

The B1 type was removed in: bytecodealliance/wasmtime#5031 This is nice; it removes some weird special-cases having to do with converting booleans to bytes when loading/storing. Now booleans are just I8s.

It was removed in bytecodealliance#5031

It was removed in #5031

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. labels Oct 7, 2022

bjorn3 reviewed Oct 7, 2022

View reviewed changes

cranelift/codegen/src/ir/extfunc.rs Show resolved Hide resolved

bjorn3 reviewed Oct 7, 2022

View reviewed changes

elliottt force-pushed the trevor/remove-booleans branch 6 times, most recently from f1c278a to 0ff1557 Compare October 7, 2022 23:21

cfallin reviewed Oct 7, 2022

View reviewed changes

elliottt force-pushed the trevor/remove-booleans branch 13 times, most recently from 78b3062 to 863b94e Compare October 12, 2022 01:40

afonso360 reviewed Oct 12, 2022

View reviewed changes

cranelift/codegen/src/isa/aarch64/inst.isle Outdated Show resolved Hide resolved

elliottt force-pushed the trevor/remove-booleans branch from 9e89306 to f95ed16 Compare October 13, 2022 16:29

elliottt force-pushed the trevor/remove-booleans branch 2 times, most recently from 9162e71 to 498215a Compare October 17, 2022 21:25

elliottt and others added 12 commits October 17, 2022 14:39

Fix bmask on s390x

a48c884

Update filetests

88f0448

Review feedback

6ab0942

Improve the select translation with i8 arguments on aarch64

07d3027

Fix the select implementation on x86_64

6de56b0

Simplify the translation of bmask on s390x

00127d3

Co-authored-by: Ulrich Weigand <ulrich.weigand@de.ibm.com>

Update cranelift/codegen/src/isa/aarch64/inst.isle

1549b28

Co-authored-by: Afonso Bordado <afonso360@users.noreply.github.com>

Fix overflow behavior with brz

a949160

Co-authored-by: Chris Fallin <chris@cfallin.org>

Fix inconsistencies with brz, brnz, and select on riscv64

558b9a2

Add some more overflow tests

76f210f

Fix some instruction docs

fb49863

elliottt force-pushed the trevor/remove-booleans branch from 498215a to fb49863 Compare October 17, 2022 21:39

elliottt merged commit 32a7593 into bytecodealliance:main Oct 17, 2022

afonso360 mentioned this pull request Oct 24, 2022

Cranelift: bmask not implemented for any type on x86_64 #5106

Closed

cfallin mentioned this pull request Dec 22, 2022

Cranelift: fix branch-of-icmp/fcmp regression: look through uextend. #5487

Merged

elliottt mentioned this pull request Mar 15, 2023

Rename as_bool to as_truthy, and fix TypeSet::as_bool #6027

Merged

aDotInTheVoid added a commit to aDotInTheVoid/wasmtime that referenced this pull request Apr 18, 2024

ir.md: Remove references to bconst

65d0541

It was removed in bytecodealliance#5031

aDotInTheVoid mentioned this pull request Apr 18, 2024

ir.md: Remove references to bconst #8400

Merged

github-merge-queue bot pushed a commit that referenced this pull request Apr 18, 2024

ir.md: Remove references to bconst (#8400)

4e0ba3b

It was removed in #5031

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cranelift: Remove booleans #5031

cranelift: Remove booleans #5031

elliottt commented Oct 7, 2022 •

edited

Loading

bjorn3 Oct 7, 2022

elliottt Oct 7, 2022

bjorn3 Oct 7, 2022

afonso360 Oct 7, 2022 •

edited

Loading

cfallin Oct 7, 2022

elliottt Oct 11, 2022

afonso360 commented Oct 7, 2022 •

edited

Loading

cfallin left a comment

cfallin Oct 7, 2022

elliottt Oct 8, 2022

cfallin Oct 7, 2022

elliottt Oct 8, 2022

cfallin commented Oct 7, 2022

elliottt commented Oct 13, 2022

uweigand commented Oct 13, 2022

		@@ -157,7 +152,7 @@ impl Type {
		/// Scalar types are all converted to `b1` which is usually what you want.
		pub fn as_bool(self) -> Self {

cranelift: Remove booleans #5031

cranelift: Remove booleans #5031

Conversation

elliottt commented Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afonso360 Oct 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afonso360 commented Oct 7, 2022 • edited Loading

cfallin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfallin commented Oct 7, 2022

elliottt commented Oct 13, 2022

uweigand commented Oct 13, 2022

elliottt commented Oct 7, 2022 •

edited

Loading

afonso360 Oct 7, 2022 •

edited

Loading

afonso360 commented Oct 7, 2022 •

edited

Loading