Cranelift: bnot.b1 is illegal on x86

[whitequark]

While looking into another issue I minimized the following testcase, which produces invalid IR:

target x86_64

function u0:323() system_v {
block0:
    v221 = bconst.b1 false
    v222 = bconst.b1 false
    v223 = bnot v221
    v224 = band v223, v222
    trap unreachable
}

[-]                                 v223 = bnot v221
;~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
; error: inst2: v223 is a real GPR value defined by a ghost instruction

It seems to me that it should work but I don't understand what's the missing legalization or selection rule is in this case. @iximeow?

[github-actions]

Subscribe to Label Action

cc @bnjbvr

This issue or pull request has been labeled: "cranelift"

Thus the following users have been cc'd because of the following labels:

• bnjbvr: cranelift

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[whitequark]

I've tried legalizing it like this but it doesn't do anything:

--- a/cranelift/codegen/meta/src/shared/legalize.rs
+++ b/cranelift/codegen/meta/src/shared/legalize.rs
@@ -5,6 +5,7 @@ use crate::cdsl::xform::{TransformGroupBuilder, TransformGroups};
 use crate::shared::immediates::Immediates;
 use crate::shared::types::Float::{F32, F64};
 use crate::shared::types::Int::{I128, I16, I32, I64, I8};
+use crate::shared::types::Bool::B1;
 use cranelift_codegen_shared::condcodes::{CondCode, IntCC};
 
 #[allow(clippy::many_single_char_names, clippy::cognitive_complexity)]
@@ -396,6 +397,14 @@ pub(crate) fn define(insts: &InstructionGroup, imm: &Immediates) -> TransformGro
         );
     }
 
+    widen.legalize(
+        def!(a = bnot.B1(b)),
+        vec![
+            def!(x = bint.I32(b)),
+            def!(a = icmp_imm.I32(intcc_eq, x, Literal::constant(&imm.uimm32, 0)))
+        ]
+    );
+
     // Widen instructions with one input operand.
     for &op in &[bnot, popcnt] {
         for &int_ty in &[I8, I16] {

[bjorn3]

You can try expand instead of widen.

[whitequark]

Tried that, it breaks with a really strange message:

error[E0425]: cannot find value `typeof_x` in this scope
   --> /home/whitequark/Projects/wasmtime/target/debug/build/cranelift-codegen-ca1b406eb4035fa6/out/legalizer.rs:195:68
    |
195 |                 let predicate = predicate && TYPE_SETS[0].contains(typeof_x);
    |                                                                    ^^^^^^^^ help: a local variable with a similar name exists: `typeof_a`

error[E0425]: cannot find value `typeof_x` in this scope
   --> /home/whitequark/Projects/wasmtime/target/debug/build/cranelift-codegen-ca1b406eb4035fa6/out/legalizer.rs:197:46
    |
197 |                     let y = pos.ins().iconst(typeof_x, -1);
    |                                              ^^^^^^^^ help: a local variable with a similar name exists: `typeof_a`

error[E0425]: cannot find value `x` in this scope
   --> /home/whitequark/Projects/wasmtime/target/debug/build/cranelift-codegen-ca1b406eb4035fa6/out/legalizer.rs:198:61
    |
198 |                     let a = pos.func.dfg.replace(inst).bxor(x, y);
    |                                                             ^ help: a local variable with a similar name exists: `a`

[iximeow]

I think that entirely confused the legalizer codegen :D I'd expect the Expand bnot using xor bit in the shared legalizer to cover this case:

    //# Expand bnot using xor.
    let minus_one = Literal::constant(&imm.imm64, -1);
    expand.legalize(
        def!(a = bnot(x)),
        vec![def!(y = iconst(minus_one)), def!(a = bxor(x, y))],
    );

I'm a little at a loss as to why this doesn't apply.

That said, adding a direct encoding for B1 to x86 does make your test pass:

diff --git a/cranelift/codegen/meta/src/isa/x86/encodings.rs b/cranelift/codegen/meta/src/isa/x86/encodings.rs
index 7863e2bd8..9f1517253 100644
--- a/cranelift/codegen/meta/src/isa/x86/encodings.rs
+++ b/cranelift/codegen/meta/src/isa/x86/encodings.rs
@@ -1454,6 +1454,7 @@ fn define_alu(
     // x86 has a bitwise not instruction NOT.
     e.enc_i32_i64(bnot, rec_ur.opcodes(&NOT).rrr(2));
     e.enc_b32_b64(bnot, rec_ur.opcodes(&NOT).rrr(2));
+    e.enc_both(bnot.bind(B1), rec_ur.opcodes(&NOT).rrr(2));
 
     // Also add a `b1` encodings for the logic instructions.
     // TODO: Should this be done with 8-bit instructions? It would improve partial register

[iximeow]

I'm a little at a loss as to why this doesn't apply.

Theory: the integer immediate forbids this legalization from applying for an incompatible B1 operand?

[abrown]

I was just looking at this but because of other problems: B1s are going to look in the expand_flags transform group for legalizations, https://github.com/bytecodealliance/wasmtime/blob/master/cranelift/codegen/meta/src/isa/x86/mod.rs#L40-L59. If it can't find it there, there is no "look in the next transform group" logic so it will just fail.

[iximeow]

expand_flags ought to be chained with expand rules afterward:

wasmtime/cranelift/codegen/meta/src/shared/legalize.rs

Lines 954 to 964 in 162fcd3

	let mut expand_flags = TransformGroupBuilder::new(
	"expand_flags",
	r#"
	Instruction expansions for architectures with flags.
	Expand some instructions using CPU flags, then fall back to the normal
	expansions. Not all architectures support CPU flags, so these patterns
	are kept separate.
	"#,
	)
	.chain_with(expand_id);

x86_expand is then chained to expand_flags later downstream:

wasmtime/cranelift/codegen/meta/src/isa/x86/legalize.rs

Lines 11 to 19 in 4ec16fa

	let mut group = TransformGroupBuilder::new(
	"x86_expand",
	r#"
	Legalize instructions by expansion.
	Use x86-specific instructions if needed."#,
	)
	.isa("x86")
	.chain_with(shared.transform_groups.by_name("expand_flags").id);

so I'm actually more confused at B1 types being expand_flags rather than x86_expand :(

[abrown]

Ah, I think you're right: there's chaining for the DSL legalizations (I think?) but not for the custom functions?