peepmatic: Apply some review suggestions from @bjorn3

cranelift/codegen/src/peepmatic.rs

@@ -196,7 +196,7 @@ fn const_to_value<'a>(builder: impl InstBuilder<'a>, c: Constant, root: Inst) ->
         Constant::Int(x, width) => {
             let width = bit_width(builder.data_flow_graph(), width, root);
             let ty = match width {
-                1 | 8 => types::I8,
+                8 => types::I8,
                 16 => types::I16,
                 32 => types::I32,
                 64 => types::I64,
@@ -407,7 +407,7 @@ fn get_argument(dfg: &DataFlowGraph, inst: Inst, i: usize) -> Option<Value> {
 
 fn peepmatic_ty_to_ir_ty(ty: Type, dfg: &DataFlowGraph, root: Inst) -> types::Type {
     match (ty.kind, bit_width(dfg, ty.bit_width, root)) {
-        (Kind::Int, 1) | (Kind::Int, 8) => types::I8,
+        (Kind::Int, 8) => types::I8,
         (Kind::Int, 16) => types::I16,
         (Kind::Int, 32) => types::I32,
         (Kind::Int, 64) => types::I64,

cranelift/peepmatic/crates/runtime/src/linear.rs

@@ -53,6 +53,9 @@ pub fn bool_to_match_result(b: bool) -> MatchResult {
     unsafe { Ok(NonZeroU32::new_unchecked(b + 1)) }
 }
 
+/// A partial match of an optimization's LHS and partial construction of its
+/// RHS.
+///
 /// An increment is a matching operation, the expected result from that
 /// operation to continue to the next increment, and the actions to take to
 /// build up the LHS scope and RHS instructions given that we got the expected
@@ -156,17 +159,18 @@ pub struct LhsId(pub u16);
 pub struct RhsId(pub u16);
 
 /// An action to perform when transitioning between states in the automata.
+///
+/// When evaluating actions, the `i^th` action implicitly defines the
+/// `RhsId(i)`.
 #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash, Serialize, Deserialize)]
 pub enum Action {
-    /// Implicitly define the n^th built up RHS instruction as something from
+    /// Reuse something from the left-hand side.
-    /// the left-hand side.
     GetLhs {
         /// The path to the instruction or value.
         path: PathId,
     },
 
-    /// Implicitly define the n^th RHS instruction as the result of the
+    /// Perform compile-time evaluation.
-    /// compile-time evaluation off this unquote operation.
     UnaryUnquote {
         /// The unquote operator.
         operator: UnquoteOperator,
@@ -174,8 +178,7 @@ pub enum Action {
         operand: RhsId,
     },
 
-    /// Implicitly define the n^th RHS instruction as the result of the
+    /// Perform compile-time evaluation.
-    /// compile-time evaluation off this unquote operation.
     BinaryUnquote {
         /// The unquote operator.
         operator: UnquoteOperator,
@@ -183,7 +186,7 @@ pub enum Action {
         operands: [RhsId; 2],
     },
 
-    /// Implicitly define the n^th RHS as an integer constant.
+    /// Create an integer constant.
     MakeIntegerConst {
         /// The constant integer value.
         value: IntegerId,
@@ -191,7 +194,7 @@ pub enum Action {
         bit_width: BitWidth,
     },
 
-    /// Implicitly define the n^th RHS as a boolean constant.
+    /// Create a boolean constant.
     MakeBooleanConst {
         /// The constant boolean value.
         value: bool,
@@ -199,14 +202,13 @@ pub enum Action {
         bit_width: BitWidth,
     },
 
-    /// Implicitly defint the n^th RHS as a condition code.
+    /// Create a condition code.
     MakeConditionCode {
         /// The condition code.
         cc: ConditionCode,
     },
 
-    /// Implicitly define the n^th RHS instruction by making a unary
+    /// Make a unary instruction.
-    /// instruction.
     MakeUnaryInst {
         /// The operand for this instruction.
         operand: RhsId,
@@ -216,8 +218,7 @@ pub enum Action {
         operator: Operator,
     },
 
-    /// Implicitly define the n^th RHS instruction by making a binary
+    /// Make a binary instruction.
-    /// instruction.
     MakeBinaryInst {
         /// The opcode for this instruction.
         operator: Operator,
@@ -227,8 +228,7 @@ pub enum Action {
         operands: [RhsId; 2],
     },
 
-    /// Implicitly define the n^th RHS instruction by making a ternary
+    /// Make a ternary instruction.
-    /// instruction.
     MakeTernaryInst {
         /// The opcode for this instruction.
         operator: Operator,

cranelift/peepmatic/crates/runtime/src/paths.rs

@@ -54,17 +54,17 @@ pub struct PathId(u16);
 /// path mapping.
 #[derive(Debug, Default)]
 pub struct PathInterner {
-    // A map from a path (whose owned data is inside `arena`) to the canonical
+    /// A map from a path (whose owned data is inside `arena`) to the canonical
-    // `PathId` we assigned it when interning it.
+    /// `PathId` we assigned it when interning it.
     map: HashMap<UnsafePath, PathId>,
 
-    // A map from a `PathId` index to an unsafe, self-borrowed path pointing
+    /// A map from a `PathId` index to an unsafe, self-borrowed path pointing
-    // into `arena`. It is safe to given these out as safe `Path`s, as long as
+    /// into `arena`. It is safe to given these out as safe `Path`s, as long as
-    // the lifetime is not longer than this `PathInterner`'s lifetime.
+    /// the lifetime is not longer than this `PathInterner`'s lifetime.
     paths: Vec<UnsafePath>,
 
-    // Bump allocation arena for path data. The bump arena ensures that these
+    /// Bump allocation arena for path data. The bump arena ensures that these
-    // allocations never move, and are therefore safe for self-references.
+    /// allocations never move, and are therefore safe for self-references.
     arena: bumpalo::Bump,
 }
 

cranelift/peepmatic/src/linearize.rs

@@ -599,13 +599,17 @@ mod tests {
                 let mut i = |i: u64| integers.intern(i);
 
                 #[allow(unused_variables)]
-                let expected = $make_expected(&mut p, &mut i);
+                let make_expected: fn(
+                    &mut dyn FnMut(&[u8]) -> PathId,
+                    &mut dyn FnMut(u64) -> IntegerId,
+                ) -> Vec<linear::Increment> = $make_expected;
+                let expected = make_expected(&mut p, &mut i);
                 dbg!(&expected);
 
                 let actual = linearize_optimization(&mut paths, &mut integers, &opts.optimizations[0]);
-                dbg!(&actual);
+                dbg!(&actual.increments);
 
-                assert_eq!(expected, actual);
+                assert_eq!(expected, actual.increments);
             }
         };
     }
@@ -617,9 +621,7 @@ mod tests {
           (is-power-of-two $C))
     (ishl $x $C))
         ",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![
-            linear::Optimization {
-                increments: vec![
             linear::Increment {
                 operation: Opcode { path: p(&[0]) },
                 expected: Ok(NonZeroU32::new(Operator::Imul as _).unwrap()),
@@ -652,78 +654,52 @@ mod tests {
                 actions: vec![],
             },
         ],
-            }
-        },
     );
 
-    linearizes_to!(
+    linearizes_to!(variable_pattern_id_optimization, "(=> $x $x)", |p, i| vec![
-        variable_pattern_id_optimization,
+        linear::Increment {
-        "(=> $x $x)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
-            linear::Optimization {
-                increments: vec![linear::Increment {
             operation: Nop,
             expected: Err(Else),
             actions: vec![GetLhs { path: p(&[0]) }],
-                }],
         }
-        },
+    ]);
-    );
 
-    linearizes_to!(
+    linearizes_to!(constant_pattern_id_optimization, "(=> $C $C)", |p, i| vec![
-        constant_pattern_id_optimization,
+        linear::Increment {
-        "(=> $C $C)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
-            linear::Optimization {
-                increments: vec![linear::Increment {
             operation: IsConst { path: p(&[0]) },
             expected: bool_to_match_result(true),
             actions: vec![GetLhs { path: p(&[0]) }],
-                }],
         }
-        },
+    ]);
-    );
 
     linearizes_to!(
         boolean_literal_id_optimization,
         "(=> true true)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![linear::Increment {
-            linear::Optimization {
-                increments: vec![linear::Increment {
             operation: BooleanValue { path: p(&[0]) },
             expected: bool_to_match_result(true),
             actions: vec![MakeBooleanConst {
                 value: true,
                 bit_width: BitWidth::Polymorphic,
             }],
-                }],
+        }]
-            }
-        },
     );
 
-    linearizes_to!(
+    linearizes_to!(number_literal_id_optimization, "(=> 5 5)", |p, i| vec![
-        number_literal_id_optimization,
+        linear::Increment {
-        "(=> 5 5)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
-            linear::Optimization {
-                increments: vec![linear::Increment {
             operation: IntegerValue { path: p(&[0]) },
             expected: Ok(i(5).into()),
             actions: vec![MakeIntegerConst {
                 value: i(5),
                 bit_width: BitWidth::Polymorphic,
             }],
-                }],
         }
-        },
+    ]);
-    );
 
     linearizes_to!(
         operation_id_optimization,
         "(=> (iconst $C) (iconst $C))",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![
-            linear::Optimization {
-                increments: vec![
             linear::Increment {
                 operation: Opcode { path: p(&[0]) },
                 expected: Ok(NonZeroU32::new(Operator::Iconst as _).unwrap()),
@@ -744,17 +720,13 @@ mod tests {
                 expected: bool_to_match_result(true),
                 actions: vec![],
             },
-                ],
+        ]
-            }
-        },
     );
 
     linearizes_to!(
         redundant_bor,
         "(=> (bor $x (bor $x $y)) (bor $x $y))",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![
-            linear::Optimization {
-                increments: vec![
             linear::Increment {
                 operation: Opcode { path: p(&[0]) },
                 expected: Ok(NonZeroU32::new(Operator::Bor as _).unwrap()),
@@ -796,35 +768,27 @@ mod tests {
                 expected: Err(Else),
                 actions: vec![],
             },
-                ],
+        ]
-            }
-        },
     );
 
     linearizes_to!(
         large_integers,
         // u64::MAX
         "(=> 18446744073709551615 0)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![linear::Increment {
-            linear::Optimization {
-                increments: vec![linear::Increment {
             operation: IntegerValue { path: p(&[0]) },
             expected: Ok(i(std::u64::MAX).into()),
             actions: vec![MakeIntegerConst {
                 value: i(0),
                 bit_width: BitWidth::Polymorphic,
             }],
-                }],
+        }]
-            }
-        }
     );
 
     linearizes_to!(
         ireduce_with_type_ascription,
         "(=> (ireduce{i32} $x) 0)",
-        |p: &mut dyn FnMut(&[u8]) -> PathId, i: &mut dyn FnMut(u64) -> IntegerId| {
+        |p, i| vec![
-            linear::Optimization {
-                increments: vec![
             linear::Increment {
                 operation: Opcode { path: p(&[0]) },
                 expected: Ok(NonZeroU32::new(Operator::Ireduce as _).unwrap()),
@@ -843,8 +807,6 @@ mod tests {
                 expected: Err(Else),
                 actions: vec![],
             },
-                ],
+        ]
-            }
-        }
     );
 }