Merge pull request #2293 from fitzgen/souper-no-assign-constant

souper-harvest: Do not generate assignments of constants

Cargo.lock

@@ -1990,9 +1990,9 @@ checksum = "fbee7696b84bbf3d89a1c2eccff0850e3047ed46bfcd2e92c29a2d074d57e252"
 
 [[package]]
 name = "souper-ir"
-version = "1.0.0"
+version = "2.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "163cc2bdd8a66cbaccdf06a6b476689a97e928883e09bffbe06fd5945842a83f"
+checksum = "a50c18ce33988e1973003afbaa66e6a465ad7a614dc33f246879ccc209c2c044"
 dependencies = [
  "id-arena",
 ]

cranelift/codegen/Cargo.toml

@@ -29,7 +29,7 @@ peepmatic = { path = "../peepmatic", optional = true, version = "0.68.0" }
 peepmatic-traits = { path = "../peepmatic/crates/traits", optional = true, version = "0.68.0" }
 peepmatic-runtime = { path = "../peepmatic/crates/runtime", optional = true, version = "0.68.0" }
 regalloc = { version = "0.0.31" }
-souper-ir = { version = "1", optional = true }
+souper-ir = { version = "2.1.0", optional = true }
 wast = { version = "28.0.0", optional = true }
 # It is a goal of the cranelift-codegen crate to have minimal external dependencies.
 # Please don't add any unless they are essential to the task of creating binary

cranelift/codegen/src/souper_harvest.rs

@@ -142,7 +142,49 @@ fn harvest_candidate_lhs(
         let souper_assignment_rhs = match func.dfg.value_def(val) {
             ir::ValueDef::Result(inst, 0) => {
                 let args = func.dfg.inst_args(inst);
-                let arg = |allocs: &mut Allocs, n| allocs.ir_to_souper_val[&args[n]].into();
+
+                // Get the n^th argument as a souper operand.
+                let arg = |allocs: &mut Allocs, n| {
+                    let arg = args[n];
+                    if let Some(a) = allocs.ir_to_souper_val.get(&arg).copied() {
+                        a.into()
+                    } else {
+                        // The only arguments we get that we haven't already
+                        // converted into a souper instruction are `iconst`s and
+                        // `bconst`s. This is because souper only allows
+                        // constants as operands, and it doesn't allow assigning
+                        // constants to a variable name. So we lazily convert
+                        // `iconst`s and `bconst`s into souper operands here,
+                        // when they are actually used.
+                        match func.dfg.value_def(arg) {
+                            ir::ValueDef::Result(inst, 0) => match func.dfg[inst] {
+                                ir::InstructionData::UnaryImm { opcode, imm } => {
+                                    debug_assert_eq!(opcode, ir::Opcode::Iconst);
+                                    let imm: i64 = imm.into();
+                                    ast::Operand::Constant(ast::Constant {
+                                        value: imm.into(),
+                                        r#type: souper_type_of(&func.dfg, arg),
+                                    })
+                                }
+                                ir::InstructionData::UnaryBool { opcode, imm } => {
+                                    debug_assert_eq!(opcode, ir::Opcode::Iconst);
+                                    ast::Operand::Constant(ast::Constant {
+                                        value: imm.into(),
+                                        r#type: souper_type_of(&func.dfg, arg),
+                                    })
+                                }
+                                _ => unreachable!(
+                                    "only iconst and bconst instructions \
+                                     aren't in `ir_to_souper_val`"
+                                ),
+                            },
+                            _ => unreachable!(
+                                "only iconst and bconst instructions \
+                                 aren't in `ir_to_souper_val`"
+                            ),
+                        }
+                    }
+                };
 
                 match (func.dfg[inst].opcode(), &func.dfg[inst]) {
                     (ir::Opcode::Iadd, _) => {
@@ -350,6 +392,28 @@ fn harvest_candidate_lhs(
                     }
                     (ir::Opcode::Select, _) => {
                         let a = arg(allocs, 0);
+
+                        // While Cranelift allows any width condition for
+                        // `select`, Souper requires an `i1`.
+                        let a = match a {
+                            ast::Operand::Value(id) => match lhs.get_value(id).r#type {
+                                Some(ast::Type { width: 1 }) => a,
+                                _ => lhs
+                                    .assignment(
+                                        None,
+                                        Some(ast::Type { width: 1 }),
+                                        ast::Instruction::Trunc { a },
+                                        vec![],
+                                    )
+                                    .into(),
+                            },
+                            ast::Operand::Constant(ast::Constant { value, .. }) => ast::Constant {
+                                value: (value != 0) as _,
+                                r#type: Some(ast::Type { width: 1 }),
+                            }
+                            .into(),
+                        };
+
                         let b = arg(allocs, 1);
                         let c = arg(allocs, 2);
                         ast::Instruction::Select { a, b, c }.into()
@@ -421,23 +485,13 @@ fn harvest_candidate_lhs(
                         let b = arg(allocs, 1);
                         ast::Instruction::UsubSat { a, b }.into()
                     }
-                    (ir::Opcode::Iconst, ir::InstructionData::UnaryImm { imm, .. }) => {
-                        let value: i64 = (*imm).into();
-                        let value: i128 = value.into();
-                        ast::Constant {
-                            value,
-                            r#type: souper_type_of(&func.dfg, val),
-                        }
-                        .into()
-                    }
-                    (ir::Opcode::Bconst, ir::InstructionData::UnaryBool { imm, .. }) => {
-                        let value = *imm as i128;
-                        ast::Constant {
-                            value,
-                            r#type: souper_type_of(&func.dfg, val),
-                        }
-                        .into()
-                    }
+                    // Because Souper doesn't allow constants to be on the right
+                    // hand side of an assignment (i.e. `%0:i32 = 1234` is
+                    // disallowed) we have to ignore `iconst` and `bconst`
+                    // instructions until we process them as operands for some
+                    // other instruction. See the `arg` closure above for
+                    // details.
+                    (ir::Opcode::Iconst, _) | (ir::Opcode::Bconst, _) => return,
                     _ => ast::AssignmentRhs::Var,
                 }
             }

cranelift/peepmatic/crates/souper/Cargo.toml

@@ -10,7 +10,7 @@ description = "Converting Souper optimizations into Peepmatic DSL"
 
 [dependencies]
 anyhow = "1"
-souper-ir = { version = "1", features = ["parse"] }
+souper-ir = { version = "2.1.0", features = ["parse"] }
 log = "0.4.8"
 
 [dev-dependencies]

cranelift/peepmatic/crates/souper/src/lib.rs

@@ -174,7 +174,6 @@ fn convert_operand(
                     }
                     Some(format!("{}", convert_name(&assn.name)))
                 }
-                ast::AssignmentRhs::Constant(c) => Some(format!("{}", c.value)),
                 ast::AssignmentRhs::Instruction(inst) => match inst {
                     // Unsupported instructions.
                     ast::Instruction::Bswap { .. }
@@ -619,8 +618,7 @@ mod tests {
             "
                 %0:i64 = var
                 %1:i32 = trunc %0
-                %2:i32 = 0
-                cand %1 %2
+                cand %1 0
             ",
             "\
 (=> (when (ireduce {i32} $v0)
@@ -631,8 +629,7 @@ mod tests {
             "
                 %0:i32 = var
                 %1:i64 = sext %0
-                %2:i64 = 0
-                cand %1 %2
+                cand %1 0
             ",
             "\
 (=> (when (sextend {i64} $v0)
@@ -643,8 +640,7 @@ mod tests {
             "
                 %0:i32 = var
                 %1:i64 = zext %0
-                %2:i64 = 0
-                cand %1 %2
+                cand %1 0
             ",
             "\
 (=> (when (uextend {i64} $v0)
@@ -677,8 +673,7 @@ mod tests {
                 %1:i32 = var
                 %2:i1 = eq %0, %1
                 %3:i32 = zext %2
-                %4:i32 = 0
-                cand %3 %4
+                cand %3 0
             ",
             "\
 (=> (when (bint (icmp eq $v0 $v1))
@@ -693,8 +688,7 @@ mod tests {
             "
                 %0:i32 = var
                 %1:i32 = add %0, 1
-                %2:i32 = 0
-                cand %1 %2
+                cand %1 0
             ",
             "\
 (=> (when (iadd_imm 1 $v0)
@@ -705,8 +699,7 @@ mod tests {
             "
                 %0:i32 = var
                 %1:i32 = add 1, %0
-                %2:i32 = 0
-                cand %1 %2
+                cand %1 0
             ",
             "\
 (=> (when (iadd_imm 1 $v0)