Repository: bytecodealliance/wasmtime
Pull request #5727 (2023-02-06 22:36:52)

Cranelift DFG: make inst clone deep-clone varargs lists. 
When investigating #5716, I found that rematerialization of a `call`, in
addition to blowing up for other reasons, caused aliasing of the varargs
list (the `EntityList` in the `ListPool`), such that editing the args of
the second copy of the call instruction inadvertently updated the first
as well.

This PR modifies `DataFlowGraph::clone_inst` so that it always clones
the varargs list if present. This shouldn't have any functional impact
on Cranelift today, because we don't rematerialize any instructions with
varargs; but it's important to get it right to avoid a bug later!

cfallin: (Rebased to unstack from #5726; there's no need to couple these actually)
jameysharp: Can we generate an `InstructionData::deep_clone` method in the meta crate by looking at which formats have value-list operands? I'm worried this won't get updated as new formats are added.

The same issue applies to `BlockCall`, right?

Aside from bugs like #5726 the egraphs code currently can never hit any of these cases, right? I think all the instructions that use value-lists (just block terminators and calls I think) are impure and so are not currently duplicated.
cfallin: > Can we generate an InstructionData::deep_clone method in the meta crate by looking at which formats have value-list operands? I'm worried this won't get updated as new formats are added.

Yep, that's the better approach; I'll build that out.

> The same issue applies to BlockCall, right?

Ah, yes, indeed, I'll need to fix that too!

> Aside from bugs like #5726 the egraphs code currently can never hit any of these cases, right? I think all the instructions that use value-lists (just block terminators and calls I think) are impure and so are not currently duplicated.

Yep, there's no functional impact today; all pure insts happen to have fixed args.

Patch:
diff --git a/cranelift/codegen/meta/src/gen_inst.rs b/cranelift/codegen/meta/src/gen_inst.rs
index 25b413c7d..db43caef6 100644
--- a/cranelift/codegen/meta/src/gen_inst.rs
+++ b/cranelift/codegen/meta/src/gen_inst.rs
@@ -383,6 +383,83 @@ fn gen_instruction_data_impl(formats: &[&InstructionFormat], fmt: &mut Formatter
             });
             fmt.line("}");
         });
+        fmt.line("}");
+
+                fmt.empty_line();
+
+        fmt.doc_comment(r#"
+            Deep-clone an `InstructionData`, including any referenced lists.
+
+            This operation requires a reference to a `ValueListPool` to
+            clone the `ValueLists`.
+        "#);
+        fmt.line("pub fn deep_clone(&self, pool: &mut ir::ValueListPool) -> Self {");
+        fmt.indent(|fmt| {
+            fmt.line("match *self {");
+            fmt.indent(|fmt| {
+                for format in formats {
+                    let name = format!("Self::{}", format.name);
+                    let mut members = vec!["opcode"];
+
+                    if format.has_value_list {
+                        members.push("ref args");
+                    } else if format.num_value_operands == 1 {
+                        members.push("arg");
+                    } else if format.num_value_operands > 0 {
+                        members.push("args");
+                    }
+
+                    match format.num_block_operands {
+                        0 => {}
+                        1 => {
+                            members.push("destination");
+                        }
+                        _ => {
+                            members.push("blocks");
+                        }
+                    };
+
+                    for field in &format.imm_fields {
+                        members.push(field.member);
+                    }
+                    let members = members.join(", ");
+
+                    fmtln!(fmt, "{}{{{}}} => {{", name, members ); // beware the moustaches
+                    fmt.indent(|fmt| {
+                        fmtln!(fmt, "Self::{} {{", format.name);
+                        fmt.indent(|fmt| {
+                            fmtln!(fmt, "opcode,");
+
+                            if format.has_value_list {
+                                fmtln!(fmt, "args: args.deep_clone(pool),");
+                            } else if format.num_value_operands == 1 {
+                                fmtln!(fmt, "arg,");
+                            } else if format.num_value_operands > 0 {
+                                fmtln!(fmt, "args,");
+                            }
+
+                            match format.num_block_operands {
+                                0 => {}
+                                1 => {
+                                    fmtln!(fmt, "destination: destination.deep_clone(pool),");
+                                }
+                                2 => {
+                                    fmtln!(fmt, "blocks: [blocks[0].deep_clone(pool), blocks[1].deep_clone(pool)],");
+                                }
+                                _ => panic!("Too many block targets in instruction"),
+                            }
+
+                            for field in &format.imm_fields {
+                                fmtln!(fmt, "{},", field.member);
+                            }
+                        });
+                        fmtln!(fmt, "}");
+                    });
+                    fmtln!(fmt, "}");
+                }
+            });
+            fmt.line("}");
+        });
         fmt.line("}");
     });
     fmt.line("}");
diff --git a/cranelift/codegen/src/dominator_tree.rs b/cranelift/codegen/src/dominator_tree.rs
index 0df595107..d70db2197 100644
--- a/cranelift/codegen/src/dominator_tree.rs
+++ b/cranelift/codegen/src/dominator_tree.rs
@@ -2,8 +2,7 @@
 
 use crate::entity::SecondaryMap;
 use crate::flowgraph::{BlockPredecessor, ControlFlowGraph};
-use crate::ir::instructions::BranchInfo;
-use crate::ir::{Block, ExpandedProgramPoint, Function, Inst, Layout, ProgramOrder, Value};
+use crate::ir::{self, Block, ExpandedProgramPoint, Function, Inst, Layout, ProgramOrder, Value};
 use crate::packed_option::PackedOption;
 use crate::timing;
 use alloc::vec::Vec;
@@ -352,21 +351,28 @@ impl DominatorTree {
     /// post-order except for the insertion of the new block header at the split point.
     fn push_successors(&mut self, func: &Function, block: Block) {
         if let Some(inst) = func.layout.last_inst(block) {
-            match func.dfg.analyze_branch(inst) {
-                BranchInfo::SingleDest(succ) => {
-                    self.push_if_unseen(succ.block(&func.dfg.value_lists))
-                }
-                BranchInfo::Conditional(block_then, block_else) => {
+            match func.dfg.insts[inst] {
+                ir::InstructionData::Jump {
+                    destination: succ, ..
+                } => self.push_if_unseen(succ.block(&func.dfg.value_lists)),
+                ir::InstructionData::Brif {
+                    blocks: [block_then, block_else],
+                    ..
+                } => {
                     self.push_if_unseen(block_then.block(&func.dfg.value_lists));
                     self.push_if_unseen(block_else.block(&func.dfg.value_lists));
                 }
-                BranchInfo::Table(jt, dest) => {
+                ir::InstructionData::BranchTable {
+                    table: jt,
+                    destination: dest,
+                    ..
+                } => {
                     for succ in func.jump_tables[jt].iter() {
                         self.push_if_unseen(*succ);
                     }
                     self.push_if_unseen(dest);
                 }
-                BranchInfo::NotABranch => {}
+                _ => {}
             }
         }
     }
diff --git a/cranelift/codegen/src/egraph/elaborate.rs b/cranelift/codegen/src/egraph/elaborate.rs
index 68f4238cf..d52927ffe 100644
--- a/cranelift/codegen/src/egraph/elaborate.rs
+++ b/cranelift/codegen/src/egraph/elaborate.rs
@@ -278,6 +278,14 @@ impl<'a> Elaborator<'a> {
                         before
                     );
 
+                    // Get the best option; we use `value` (latest
+                    // value) here so we have a full view of the
+                    // eclass.
+                    trace!("looking up best value for {}", value);
+                    let (_, best_value) = self.value_to_best_value[value];
+                    debug_assert_ne!(best_value, Value::reserved_value());
+                    trace!("elaborate: value {} -> best {}", value, best_value);
+
                     let remat = if let Some(elab_val) =
                         self.value_to_elaborated_value.get(&canonical_value)
                     {
@@ -287,7 +295,7 @@ impl<'a> Elaborator<'a> {
                         // from another block. If so, ignore the hit
                         // and recompute below.
                         let remat = elab_val.in_block != self.cur_block
-                            && self.remat_values.contains(&canonical_value);
+                            && self.remat_values.contains(&best_value);
                         if !remat {
                             trace!("elaborate: value {} -> {:?}", value, elab_val);
                             self.stats.elaborate_memoize_hit += 1;
@@ -304,20 +312,12 @@ impl<'a> Elaborator<'a> {
                         // Value not available; but still look up
                         // whether it's been flagged for remat because
                         // this affects placement.
-                        let remat = self.remat_values.contains(&canonical_value);
+                        let remat = self.remat_values.contains(&best_value);
                         trace!(" -> not present in map; remat = {}", remat);
                         remat
                     };
                     self.stats.elaborate_memoize_miss += 1;
 
-                    // Get the best option; we use `value` (latest
-                    // value) here so we have a full view of the
-                    // eclass.
-                    trace!("looking up best value for {}", value);
-                    let (_, best_value) = self.value_to_best_value[value];
-                    debug_assert_ne!(best_value, Value::reserved_value());
-                    trace!("elaborate: value {} -> best {}", value, best_value,);
-
                     // Now resolve the value to its definition to see
                     // how we can compute it.
                     let (inst, result_idx) = match self.func.dfg.value_def(best_value) {
diff --git a/cranelift/codegen/src/flowgraph.rs b/cranelift/codegen/src/flowgraph.rs
index ead9cba2f..9b04a5abc 100644
--- a/cranelift/codegen/src/flowgraph.rs
+++ b/cranelift/codegen/src/flowgraph.rs
@@ -22,8 +22,7 @@
 
 use crate::bforest;
 use crate::entity::SecondaryMap;
-use crate::ir::instructions::BranchInfo;
-use crate::ir::{Block, Function, Inst};
+use crate::ir::{self, Block, Function, Inst};
 use crate::timing;
 use core::mem;
 
@@ -118,22 +117,31 @@ impl ControlFlowGraph {
 
     fn compute_block(&mut self, func: &Function, block: Block) {
         if let Some(inst) = func.layout.last_inst(block) {
-            match func.dfg.analyze_branch(inst) {
-                BranchInfo::SingleDest(dest) => {
+            match func.dfg.insts[inst] {
+                ir::InstructionData::Jump {
+                    destination: dest, ..
+                } => {
                     self.add_edge(block, inst, dest.block(&func.dfg.value_lists));
                 }
-                BranchInfo::Conditional(block_then, block_else) => {
+                ir::InstructionData::Brif {
+                    blocks: [block_then, block_else],
+                    ..
+                } => {
                     self.add_edge(block, inst, block_then.block(&func.dfg.value_lists));
                     self.add_edge(block, inst, block_else.block(&func.dfg.value_lists));
                 }
-                BranchInfo::Table(jt, dest) => {
+                ir::InstructionData::BranchTable {
+                    table: jt,
+                    destination: dest,
+                    ..
+                } => {
                     self.add_edge(block, inst, dest);
 
                     for dest in func.jump_tables[jt].iter() {
                         self.add_edge(block, inst, *dest);
                     }
                 }
-                BranchInfo::NotABranch => {}
+                _ => {}
             }
         }
     }
diff --git a/cranelift/codegen/src/inst_predicates.rs b/cranelift/codegen/src/inst_predicates.rs
index eb8a53202..9e9280675 100644
--- a/cranelift/codegen/src/inst_predicates.rs
+++ b/cranelift/codegen/src/inst_predicates.rs
@@ -1,7 +1,6 @@
 //! Instruction predicates/properties, shared by various analyses.
 use crate::ir::immediates::Offset32;
-use crate::ir::instructions::BranchInfo;
-use crate::ir::{Block, DataFlowGraph, Function, Inst, InstructionData, Opcode, Type, Value};
+use crate::ir::{self, Block, DataFlowGraph, Function, Inst, InstructionData, Opcode, Type, Value};
 use cranelift_entity::EntityRef;
 
 /// Preserve instructions with used result values.
@@ -176,16 +175,26 @@ pub(crate) fn visit_block_succs<F: FnMut(Inst, Block, bool)>(
     mut visit: F,
 ) {
     if let Some(inst) = f.layout.last_inst(block) {
-        match f.dfg.insts[inst].analyze_branch() {
-            BranchInfo::NotABranch => {}
-            BranchInfo::SingleDest(dest) => {
+        match f.dfg.insts[inst] {
+            ir::InstructionData::Jump {
+                destination: dest, ..
+            } => {
                 visit(inst, dest.block(&f.dfg.value_lists), false);
             }
-            BranchInfo::Conditional(block_then, block_else) => {
+
+            ir::InstructionData::Brif {
+                blocks: [block_then, block_else],
+                ..
+            } => {
                 visit(inst, block_then.block(&f.dfg.value_lists), false);
                 visit(inst, block_else.block(&f.dfg.value_lists), false);
             }
-            BranchInfo::Table(table, dest) => {
+
+            ir::InstructionData::BranchTable {
+                table,
+                destination: dest,
+                ..
+            } => {
                 // The default block is reached via a direct conditional branch,
                 // so it is not part of the table.
                 visit(inst, dest, false);
@@ -194,6 +203,8 @@ pub(crate) fn visit_block_succs<F: FnMut(Inst, Block, bool)>(
                     visit(inst, dest, true);
                 }
             }
+
+            _ => {}
         }
     }
 }
diff --git a/cranelift/codegen/src/ir/dfg.rs b/cranelift/codegen/src/ir/dfg.rs
index a1f4c34f4..7b3246187 100644
--- a/cranelift/codegen/src/ir/dfg.rs
+++ b/cranelift/codegen/src/ir/dfg.rs
@@ -4,7 +4,7 @@ use crate::entity::{self, PrimaryMap, SecondaryMap};
 use crate::ir;
 use crate::ir::builder::ReplaceBuilder;
 use crate::ir::dynamic_type::{DynamicTypeData, DynamicTypes};
-use crate::ir::instructions::{BranchInfo, CallInfo, InstructionData};
+use crate::ir::instructions::{CallInfo, InstructionData};
 use crate::ir::{types, ConstantData, ConstantPool, Immediate};
 use crate::ir::{
     Block, BlockCall, DynamicType, FuncRef, Inst, SigRef, Signature, Type, Value,
@@ -907,6 +907,10 @@ impl DataFlowGraph {
     pub fn clone_inst(&mut self, inst: Inst) -> Inst {
         // First, add a clone of the InstructionData.
         let inst_data = self.insts[inst].clone();
+        // If the `inst_data` has a reference to a ValueList, clone it
+        // as well, because we can't share these (otherwise mutating
+        // one would affect the other).
+        let inst_data = inst_data.deep_clone(&mut self.value_lists);
         let new_inst = self.make_inst(inst_data);
         // Get the controlling type variable.
         let ctrl_typevar = self.ctrl_typevar(inst);
@@ -1035,11 +1039,6 @@ impl DataFlowGraph {
         impl ExactSizeIterator for InstResultTypes<'_> {}
     }
 
-    /// Check if `inst` is a branch.
-    pub fn analyze_branch(&self, inst: Inst) -> BranchInfo {
-        self.insts[inst].analyze_branch()
-    }
-
     /// Compute the type of an instruction result from opcode constraints and call signatures.
     ///
     /// This computes the same sequence of result types that `make_inst_results()` above would
@@ -1620,4 +1619,25 @@ mod tests {
         assert_eq!(pos.func.dfg.resolve_aliases(c2), c2);
         assert_eq!(pos.func.dfg.resolve_aliases(c), c2);
     }
+
+    #[test]
+    fn cloning() {
+        use crate::ir::InstBuilder;
+
+        let mut func = Function::new();
+        let mut sig = Signature::new(crate::isa::CallConv::SystemV);
+        sig.params.push(ir::AbiParam::new(types::I32));
+        let sig = func.import_signature(sig);
+        let block0 = func.dfg.make_block();
+        let mut pos = FuncCursor::new(&mut func);
+        pos.insert_block(block0);
+        let v1 = pos.ins().iconst(types::I32, 0);
+        let v2 = pos.ins().iconst(types::I32, 1);
+        let call_inst = pos.ins().call_indirect(sig, v1, &[v1]);
+        let func = pos.func;
+
+        let call_inst_dup = func.dfg.clone_inst(call_inst);
+        func.dfg.inst_args_mut(call_inst)[0] = v2;
+        assert_eq!(v1, func.dfg.inst_args(call_inst_dup)[0]);
+    }
 }
diff --git a/cranelift/codegen/src/ir/entities.rs b/cranelift/codegen/src/ir/entities.rs
index 8573b4cd7..51c763320 100644
--- a/cranelift/codegen/src/ir/entities.rs
+++ b/cranelift/codegen/src/ir/entities.rs
@@ -92,11 +92,6 @@ impl Value {
 /// the `Value` of such instructions, use [`inst_results`](super::DataFlowGraph::inst_results) or
 /// its analogue in `cranelift_frontend::FuncBuilder`.
 ///
-/// If you look around the API, you can find many inventive uses for `Inst`,
-/// such as [annotating specific instructions with a comment][inst_comment]
-/// or [performing reflection at compile time](super::DataFlowGraph::analyze_branch)
-/// on the type of instruction.
-///
 /// [inst_comment]: https://github.com/bjorn3/rustc_codegen_cranelift/blob/0f8814fd6da3d436a90549d4bb19b94034f2b19c/src/pretty_clif.rs
 ///
 /// While the order is stable, it is arbitrary and does not necessarily resemble the layout order.
diff --git a/cranelift/codegen/src/ir/function.rs b/cranelift/codegen/src/ir/function.rs
index 541483107..b0a14d426 100644
--- a/cranelift/codegen/src/ir/function.rs
+++ b/cranelift/codegen/src/ir/function.rs
@@ -4,15 +4,12 @@
 //! instructions.
 
 use crate::entity::{PrimaryMap, SecondaryMap};
-use crate::ir;
-use crate::ir::JumpTables;
 use crate::ir::{
-    instructions::BranchInfo, Block, DynamicStackSlot, DynamicStackSlotData, DynamicType,
-    ExtFuncData, FuncRef, GlobalValue, GlobalValueData, Inst, InstructionData, JumpTable,
-    JumpTableData, Opcode, SigRef, StackSlot, StackSlotData, Table, TableData, Type,
+    self, Block, DataFlowGraph, DynamicStackSlot, DynamicStackSlotData, DynamicStackSlots,
+    DynamicType, ExtFuncData, FuncRef, GlobalValue, GlobalValueData, Inst, InstructionData,
+    JumpTable, JumpTableData, JumpTables, Layout, Opcode, SigRef, Signature, SourceLocs, StackSlot,
+    StackSlotData, StackSlots, Table, TableData, Type,
 };
-use crate::ir::{DataFlowGraph, Layout, Signature};
-use crate::ir::{DynamicStackSlots, SourceLocs, StackSlots};
 use crate::isa::CallConv;
 use crate::value_label::ValueLabelsRanges;
 use crate::write::write_function;
@@ -280,8 +277,10 @@ impl FunctionStencil {
     /// Rewrite the branch destination to `new_dest` if the destination matches `old_dest`.
     /// Does nothing if called with a non-jump or non-branch instruction.
     pub fn rewrite_branch_destination(&mut self, inst: Inst, old_dest: Block, new_dest: Block) {
-        match self.dfg.analyze_branch(inst) {
-            BranchInfo::SingleDest(dest) => {
+        match self.dfg.insts[inst] {
+            InstructionData::Jump {
+                destination: dest, ..
+            } => {
                 if dest.block(&self.dfg.value_lists) == old_dest {
                     for block in self.dfg.insts[inst].branch_destination_mut() {
                         block.set_block(new_dest, &mut self.dfg.value_lists)
@@ -289,7 +288,10 @@ impl FunctionStencil {
                 }
             }
 
-            BranchInfo::Conditional(block_then, block_else) => {
+            InstructionData::Brif {
+                blocks: [block_then, block_else],
+                ..
+            } => {
                 if block_then.block(&self.dfg.value_lists) == old_dest {
                     if let InstructionData::Brif {
                         blocks: [block_then, _],
@@ -315,7 +317,11 @@ impl FunctionStencil {
                 }
             }
 
-            BranchInfo::Table(table, default_dest) => {
+            InstructionData::BranchTable {
+                table,
+                destination: default_dest,
+                ..
+            } => {
                 self.jump_tables[table].iter_mut().for_each(|entry| {
                     if *entry == old_dest {
                         *entry = new_dest;
@@ -335,7 +341,7 @@ impl FunctionStencil {
                 }
             }
 
-            BranchInfo::NotABranch => {}
+            _ => {}
         }
     }
 
diff --git a/cranelift/codegen/src/ir/instructions.rs b/cranelift/codegen/src/ir/instructions.rs
index dec1e8728..9513949a6 100644
--- a/cranelift/codegen/src/ir/instructions.rs
+++ b/cranelift/codegen/src/ir/instructions.rs
@@ -20,7 +20,7 @@ use crate::ir::{
     self,
     condcodes::{FloatCC, IntCC},
     trapcode::TrapCode,
-    types, Block, FuncRef, JumpTable, MemFlags, SigRef, StackSlot, Type, Value,
+    types, Block, FuncRef, MemFlags, SigRef, StackSlot, Type, Value,
 };
 
 /// Some instructions use an external list of argument values because there is not enough space in
@@ -123,6 +123,15 @@ impl BlockCall {
     pub fn display<'a>(&self, pool: &'a ValueListPool) -> DisplayBlockCall<'a> {
         DisplayBlockCall { block: *self, pool }
     }
+
+    /// Deep-clone the underlying list in the same pool. The returned
+    /// list will have identical contents but changes to this list
+    /// will not change its contents or vice-versa.
+    pub fn deep_clone(&self, pool: &mut ValueListPool) -> Self {
+        Self {
+            values: self.values.deep_clone(pool),
+        }
+    }
 }
 
 /// Wrapper for the context needed to display a [BlockCall] value.
@@ -292,27 +301,6 @@ impl Default for VariableArgs {
 /// Avoid large matches on instruction formats by using the methods defined here to examine
 /// instructions.
 impl InstructionData {
-    /// Return information about the destination of a branch or jump instruction.
-    ///
-    /// Any instruction that can transfer control to another block reveals its possible destinations
-    /// here.
-    pub fn analyze_branch(&self) -> BranchInfo {
-        match *self {
-            Self::Jump { destination, .. } => BranchInfo::SingleDest(destination),
-            Self::Brif {
-                blocks: [block_then, block_else],
-                ..
-            } => BranchInfo::Conditional(block_then, block_else),
-            Self::BranchTable {
-                table, destination, ..
-            } => BranchInfo::Table(table, destination),
-            _ => {
-                debug_assert!(!self.opcode().is_branch());
-                BranchInfo::NotABranch
-            }
-        }
-    }
-
     /// Get the destinations of this instruction, if it's a branch.
     ///
     /// `br_table` returns the empty slice.
@@ -476,23 +464,6 @@ impl InstructionData {
     }
 }
 
-/// Information about branch and jump instructions.
-pub enum BranchInfo {
-    /// This is not a branch or jump instruction.
-    /// This instruction will not transfer control to another block in the function, but it may still
-    /// affect control flow by returning or trapping.
-    NotABranch,
-
-    /// This is a branch or jump to a single destination block, possibly taking value arguments.
-    SingleDest(BlockCall),
-
-    /// This is a conditional branch
-    Conditional(BlockCall, BlockCall),
-
-    /// This is a jump table branch which can have many destination blocks and one default block.
-    Table(JumpTable, Block),
-}
-
 /// Information about call instructions.
 pub enum CallInfo<'a> {
     /// This is not a call instruction.
diff --git a/cranelift/codegen/src/machinst/lower.rs b/cranelift/codegen/src/machinst/lower.rs
index ea29cb346..250bfa078 100644
--- a/cranelift/codegen/src/machinst/lower.rs
+++ b/cranelift/codegen/src/machinst/lower.rs
@@ -9,9 +9,9 @@ use crate::entity::SecondaryMap;
 use crate::fx::{FxHashMap, FxHashSet};
 use crate::inst_predicates::{has_lowering_side_effect, is_constant_64bit};
 use crate::ir::{
-    instructions, ArgumentPurpose, Block, Constant, ConstantData, DataFlowGraph, ExternalName,
-    Function, GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, Opcode,
-    RelSourceLoc, Type, Value, ValueDef, ValueLabelAssignments, ValueLabelStart,
+    ArgumentPurpose, Block, Constant, ConstantData, DataFlowGraph, ExternalName, Function,
+    GlobalValue, GlobalValueData, Immediate, Inst, InstructionData, MemFlags, Opcode, RelSourceLoc,
+    Type, Value, ValueDef, ValueLabelAssignments, ValueLabelStart,
 };
 use crate::machinst::{
     writable_value_regs, BlockIndex, BlockLoweringOrder, Callee, LoweredBlock, MachLabel, Reg,
@@ -943,17 +943,18 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
             let (inst, succ) = self.vcode.block_order().succ_indices(block)[succ_idx];
 
             // Get branch args and convert to Regs.
-            let branch_args = match self.f.dfg.analyze_branch(inst) {
-                instructions::BranchInfo::NotABranch => unreachable!(),
-                instructions::BranchInfo::SingleDest(block) => {
-                    block.args_slice(&self.f.dfg.value_lists)
-                }
-                instructions::BranchInfo::Conditional(then_block, else_block) => {
+            let branch_args = match self.f.dfg.insts[inst] {
+                InstructionData::Jump {
+                    destination: block, ..
+                } => block.args_slice(&self.f.dfg.value_lists),
+                InstructionData::Brif {
+                    blocks: [then_block, else_block],
+                    ..
+                } => {
                     // NOTE: `succ_idx == 0` implying that we're traversing the `then_block` is
                     // enforced by the traversal order defined in `visit_block_succs`. Eventually
-                    // we should traverse the `branch_destination` slice instead of the result of
-                    // analyze_branch there, which would simplify computing the branch args
-                    // significantly.
+                    // we should traverse the `branch_destination` slice there, which would
+                    // simplify computing the branch args significantly.
                     if succ_idx == 0 {
                         then_block.args_slice(&self.f.dfg.value_lists)
                     } else {
@@ -961,7 +962,8 @@ impl<'func, I: VCodeInst> Lower<'func, I> {
                         else_block.args_slice(&self.f.dfg.value_lists)
                     }
                 }
-                instructions::BranchInfo::Table(_, _) => &[],
+                InstructionData::BranchTable { .. } => &[],
+                _ => unreachable!(),
             };
             let mut branch_arg_vregs: SmallVec<[Reg; 16]> = smallvec![];
             for &arg in branch_args {
diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs
index b71ccf41e..523e32d7f 100644
--- a/cranelift/codegen/src/verifier/mod.rs
+++ b/cranelift/codegen/src/verifier/mod.rs
@@ -62,7 +62,7 @@ use crate::dominator_tree::DominatorTree;
 use crate::entity::SparseSet;
 use crate::flowgraph::{BlockPredecessor, ControlFlowGraph};
 use crate::ir::entities::AnyEntity;
-use crate::ir::instructions::{BranchInfo, CallInfo, InstructionFormat, ResolvedConstraint};
+use crate::ir::instructions::{CallInfo, InstructionFormat, ResolvedConstraint};
 use crate::ir::{self, ArgumentExtension};
 use crate::ir::{
     types, ArgumentPurpose, Block, Constant, DynamicStackSlot, FuncRef, Function, GlobalValue,
@@ -1293,8 +1293,10 @@ impl<'a> Verifier<'a> {
         inst: Inst,
         errors: &mut VerifierErrors,
     ) -> VerifierStepResult<()> {
-        match self.func.dfg.analyze_branch(inst) {
-            BranchInfo::SingleDest(block) => {
+        match self.func.dfg.insts[inst] {
+            ir::InstructionData::Jump {
+                destination: block, ..
+            } => {
                 let iter = self
                     .func
                     .dfg
@@ -1304,7 +1306,10 @@ impl<'a> Verifier<'a> {
                 let args = block.args_slice(&self.func.dfg.value_lists);
                 self.typecheck_variable_args_iterator(inst, iter, args, errors)?;
             }
-            BranchInfo::Conditional(block_then, block_else) => {
+            ir::InstructionData::Brif {
+                blocks: [block_then, block_else],
+                ..
+            } => {
                 let iter = self
                     .func
                     .dfg
@@ -1323,7 +1328,11 @@ impl<'a> Verifier<'a> {
                 let args_else = block_else.args_slice(&self.func.dfg.value_lists);
                 self.typecheck_variable_args_iterator(inst, iter, args_else, errors)?;
             }
-            BranchInfo::Table(table, block) => {
+            ir::InstructionData::BranchTable {
+                table,
+                destination: block,
+                ..
+            } => {
                 let arg_count = self.func.dfg.num_block_params(block);
                 if arg_count != 0 {
                     return errors.nonfatal((
@@ -1349,7 +1358,7 @@ impl<'a> Verifier<'a> {
                     }
                 }
             }
-            BranchInfo::NotABranch => {}
+            _ => {}
         }
 
         match self.func.dfg.insts[inst].analyze_call(&self.func.dfg.value_lists) {
diff --git a/cranelift/frontend/src/frontend.rs b/cranelift/frontend/src/frontend.rs
index dff1601cc..a0b5e2e29 100644
--- a/cranelift/frontend/src/frontend.rs
+++ b/cranelift/frontend/src/frontend.rs
@@ -106,17 +106,20 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> {
             self.builder.func.set_srcloc(inst, self.builder.srcloc);
         }
 
-        match self.builder.func.dfg.analyze_branch(inst) {
-            ir::instructions::BranchInfo::NotABranch => (),
-
-            ir::instructions::BranchInfo::SingleDest(dest) => {
+        match self.builder.func.dfg.insts[inst] {
+            ir::InstructionData::Jump {
+                destination: dest, ..
+            } => {
                 // If the user has supplied jump arguments we must adapt the arguments of
                 // the destination block
                 let block = dest.block(&self.builder.func.dfg.value_lists);
                 self.builder.declare_successor(block, inst);
             }
 
-            ir::instructions::BranchInfo::Conditional(branch_then, branch_else) => {
+            ir::InstructionData::Brif {
+                blocks: [branch_then, branch_else],
+                ..
+            } => {
                 let block_then = branch_then.block(&self.builder.func.dfg.value_lists);
                 let block_else = branch_else.block(&self.builder.func.dfg.value_lists);
 
@@ -126,7 +129,9 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> {
                 }
             }
 
-            ir::instructions::BranchInfo::Table(table, destination) => {
+            ir::InstructionData::BranchTable {
+                table, destination, ..
+            } => {
                 // Unlike all other jumps/branches, jump tables are
                 // capable of having the same successor appear
                 // multiple times, so we must deduplicate.
@@ -149,6 +154,8 @@ impl<'short, 'long> InstBuilderBase<'short> for FuncInstBuilder<'short, 'long> {
                 }
                 self.builder.declare_successor(destination, inst);
             }
+
+            _ => {}
         }
 
         if data.opcode().is_terminator() {
diff --git a/cranelift/frontend/src/ssa.rs b/cranelift/frontend/src/ssa.rs
index ea53765e5..0623ddc74 100644
--- a/cranelift/frontend/src/ssa.rs
+++ b/cranelift/frontend/src/ssa.rs
@@ -14,7 +14,6 @@ use core::mem;
 use cranelift_codegen::cursor::{Cursor, FuncCursor};
 use cranelift_codegen::entity::{EntityList, EntitySet, ListPool, SecondaryMap};
 use cranelift_codegen::ir::immediates::{Ieee32, Ieee64};
-use cranelift_codegen::ir::instructions::BranchInfo;
 use cranelift_codegen::ir::types::{F32, F64, I128, I64};
 use cranelift_codegen::ir::{
     Block, Function, Inst, InstBuilder, InstructionData, JumpTableData, Type, Value,
@@ -578,20 +577,17 @@ impl SSABuilder {
         dest_block: Block,
         val: Value,
     ) -> Option<(Block, Inst)> {
-        match func.dfg.analyze_branch(branch) {
-            BranchInfo::NotABranch => {
-                panic!("you have declared a non-branch instruction as a predecessor to a block");
-            }
+        match func.dfg.insts[branch] {
             // For a single destination appending a jump argument to the instruction
             // is sufficient.
-            BranchInfo::SingleDest(_) => {
+            InstructionData::Jump { .. } => {
                 let dfg = &mut func.dfg;
                 for dest in dfg.insts[branch].branch_destination_mut() {
                     dest.append_argument(val, &mut dfg.value_lists);
                 }
                 None
             }
-            BranchInfo::Conditional(_, _) => {
+            InstructionData::Brif { .. } => {
                 let dfg = &mut func.dfg;
                 for block in dfg.insts[branch].branch_destination_mut() {
                     if block.block(&dfg.value_lists) == dest_block {
@@ -600,7 +596,7 @@ impl SSABuilder {
                 }
                 None
             }
-            BranchInfo::Table(mut jt, _default_block) => {
+            InstructionData::BranchTable { table: mut jt, .. } => {
                 // In the case of a jump table, the situation is tricky because br_table doesn't
                 // support arguments. We have to split the critical edge.
                 let middle_block = func.dfg.make_block();
@@ -622,7 +618,7 @@ impl SSABuilder {
                     jt = func.create_jump_table(copied);
                 }
 
-                // Redo the match from `analyze_branch` but this time capture mutable references
+                // Redo the match from above, but this time capture mutable references
                 match &mut func.dfg.insts[branch] {
                     InstructionData::BranchTable {
                         destination, table, ..
@@ -639,6 +635,9 @@ impl SSABuilder {
                 let middle_jump_inst = cur.ins().jump(dest_block, &[val]);
                 Some((middle_block, middle_jump_inst))
             }
+            _ => {
+                panic!("you have declared a non-branch instruction as a predecessor to a block");
+            }
         }
     }
 
@@ -689,7 +688,7 @@ mod tests {
     use crate::Variable;
     use cranelift_codegen::cursor::{Cursor, FuncCursor};
     use cranelift_codegen::entity::EntityRef;
-    use cranelift_codegen::ir::instructions::BranchInfo;
+    use cranelift_codegen::ir;
     use cranelift_codegen::ir::types::*;
     use cranelift_codegen::ir::{Function, Inst, InstBuilder, JumpTableData, Opcode};
     use cranelift_codegen::settings;
@@ -838,8 +837,11 @@ mod tests {
 
         assert_eq!(x_ssa, x_use3);
         assert_eq!(y_ssa, y_use3);
-        match func.dfg.analyze_branch(brif_block0_block2_block1) {
-            BranchInfo::Conditional(block_then, block_else) => {
+        match func.dfg.insts[brif_block0_block2_block1] {
+            ir::InstructionData::Brif {
+                blocks: [block_then, block_else],
+                ..
+            } => {
                 assert_eq!(block_then.block(&func.dfg.value_lists), block2);
                 assert_eq!(block_then.args_slice(&func.dfg.value_lists).len(), 0);
                 assert_eq!(block_else.block(&func.dfg.value_lists), block1);
@@ -847,8 +849,11 @@ mod tests {
             }
             _ => assert!(false),
         };
-        match func.dfg.analyze_branch(brif_block0_block2_block1) {
-            BranchInfo::Conditional(block_then, block_else) => {
+        match func.dfg.insts[brif_block0_block2_block1] {
+            ir::InstructionData::Brif {
+                blocks: [block_then, block_else],
+                ..
+            } => {
                 assert_eq!(block_then.block(&func.dfg.value_lists), block2);
                 assert_eq!(block_then.args_slice(&func.dfg.value_lists).len(), 0);
                 assert_eq!(block_else.block(&func.dfg.value_lists), block1);
@@ -856,8 +861,10 @@ mod tests {
             }
             _ => assert!(false),
         };
-        match func.dfg.analyze_branch(jump_block1_block2) {
-            BranchInfo::SingleDest(dest) => {
+        match func.dfg.insts[jump_block1_block2] {
+            ir::InstructionData::Jump {
+                destination: dest, ..
+            } => {
                 assert_eq!(dest.block(&func.dfg.value_lists), block2);
                 assert_eq!(dest.args_slice(&func.dfg.value_lists).len(), 0);
             }
diff --git a/cranelift/isle/isle/src/lexer.rs b/cranelift/isle/isle/src/lexer.rs
index d76b1e16a..a69894449 100644
--- a/cranelift/isle/isle/src/lexer.rs
+++ b/cranelift/isle/isle/src/lexer.rs
@@ -232,7 +232,7 @@ impl<'a> Lexer<'a> {
                 debug_assert!(!s.is_empty());
                 Ok(Some((start_pos, Token::Symbol(s.to_string()))))
             }
-            c if (c >= b'0' && c <= b'9') || c == b'-' => {
+            c @ (b'0'..=b'9' | b'-') => {
                 let start_pos = self.pos();
                 let neg = if c == b'-' {
                     self.advance_pos();
@@ -258,16 +258,8 @@ impl<'a> Lexer<'a> {
                 // string-to-integer conversion.
                 let mut s = vec![];
                 while self.pos.offset < self.buf.len()
-                    && ((radix == 10
-                        && self.buf[self.pos.offset] >= b'0'
-                        && self.buf[self.pos.offset] <= b'9')
-                        || (radix == 16
-                            && ((self.buf[self.pos.offset] >= b'0'
-                                && self.buf[self.pos.offset] <= b'9')
-                                || (self.buf[self.pos.offset] >= b'a'
-                                    && self.buf[self.pos.offset] <= b'f')
-                                || (self.buf[self.pos.offset] >= b'A'
-                                    && self.buf[self.pos.offset] <= b'F')))
+                    && ((radix == 10 && self.buf[self.pos.offset].is_ascii_digit())
+                        || (radix == 16 && self.buf[self.pos.offset].is_ascii_hexdigit())
                         || self.buf[self.pos.offset] == b'_')
                 {
                     if self.buf[self.pos.offset] != b'_' {
diff --git a/cranelift/isle/isle/src/parser.rs b/cranelift/isle/isle/src/parser.rs
index a305d6ce9..583f952d7 100644
--- a/cranelift/isle/isle/src/parser.rs
+++ b/cranelift/isle/isle/src/parser.rs
@@ -45,7 +45,7 @@ impl<'a> Parser<'a> {
         }
     }
 
-    fn take<F: Fn(&Token) -> bool>(&mut self, f: F) -> Result<Token> {
+    fn expect<F: Fn(&Token) -> bool>(&mut self, f: F) -> Result<Token> {
         if let Some(&(pos, ref peek)) = self.lexer.peek() {
             if !f(peek) {
                 return Err(self.error(pos, format!("Unexpected token {:?}", peek)));
@@ -56,6 +56,17 @@ impl<'a> Parser<'a> {
         }
     }
 
+    fn eat<F: Fn(&Token) -> bool>(&mut self, f: F) -> Result<Option<Token>> {
+        if let Some(&(_pos, ref peek)) = self.lexer.peek() {
+            if !f(peek) {
+                return Ok(None);
+            }
+            Ok(Some(self.lexer.next()?.unwrap().1))
+        } else {
+            Ok(None) // EOF
+        }
+    }
+
     fn is<F: Fn(&Token) -> bool>(&self, f: F) -> bool {
         if let Some(&(_, ref peek)) = self.lexer.peek() {
             f(peek)
@@ -80,16 +91,10 @@ impl<'a> Parser<'a> {
         self.is(|tok| *tok == Token::At)
     }
     fn is_sym(&self) -> bool {
-        self.is(|tok| tok.is_sym())
+        self.is(Token::is_sym)
     }
     fn is_int(&self) -> bool {
-        self.is(|tok| tok.is_int())
-    }
-    fn is_sym_str(&self, s: &str) -> bool {
-        self.is(|tok| match tok {
-            &Token::Symbol(ref tok_s) if tok_s == s => true,
-            _ => false,
-        })
+        self.is(Token::is_int)
     }
 
     fn is_const(&self) -> bool {
@@ -99,25 +104,33 @@ impl<'a> Parser<'a> {
         })
     }
 
-    fn lparen(&mut self) -> Result<()> {
-        self.take(|tok| *tok == Token::LParen).map(|_| ())
+    fn expect_lparen(&mut self) -> Result<()> {
+        self.expect(|tok| *tok == Token::LParen).map(|_| ())
     }
-    fn rparen(&mut self) -> Result<()> {
-        self.take(|tok| *tok == Token::RParen).map(|_| ())
+    fn expect_rparen(&mut self) -> Result<()> {
+        self.expect(|tok| *tok == Token::RParen).map(|_| ())
     }
-    fn at(&mut self) -> Result<()> {
-        self.take(|tok| *tok == Token::At).map(|_| ())
+    fn expect_at(&mut self) -> Result<()> {
+        self.expect(|tok| *tok == Token::At).map(|_| ())
     }
 
-    fn symbol(&mut self) -> Result<String> {
-        match self.take(|tok| tok.is_sym())? {
+    fn expect_symbol(&mut self) -> Result<String> {
+        match self.expect(Token::is_sym)? {
             Token::Symbol(s) => Ok(s),
             _ => unreachable!(),
         }
     }
 
-    fn int(&mut self) -> Result<i128> {
-        match self.take(|tok| tok.is_int())? {
+    fn eat_sym_str(&mut self, s: &str) -> Result<bool> {
+        self.eat(|tok| match tok {
+            &Token::Symbol(ref tok_s) if tok_s == s => true,
+            _ => false,
+        })
+        .map(|token| token.is_some())
+    }
+
+    fn expect_int(&mut self) -> Result<i128> {
+        match self.expect(Token::is_int)? {
             Token::Int(i) => Ok(i),
             _ => unreachable!(),
         }
@@ -136,9 +149,9 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_def(&mut self) -> Result<Def> {
-        self.lparen()?;
+        self.expect_lparen()?;
         let pos = self.pos();
-        let def = match &self.symbol()?[..] {
+        let def = match &self.expect_symbol()?[..] {
             "pragma" => Def::Pragma(self.parse_pragma()?),
             "type" => Def::Type(self.parse_type()?),
             "decl" => Def::Decl(self.parse_decl()?),
@@ -150,7 +163,7 @@ impl<'a> Parser<'a> {
                 return Err(self.error(pos, format!("Unexpected identifier: {}", s)));
             }
         };
-        self.rparen()?;
+        self.expect_rparen()?;
         Ok(def)
     }
 
@@ -182,7 +195,7 @@ impl<'a> Parser<'a> {
 
     fn parse_ident(&mut self) -> Result<Ident> {
         let pos = self.pos();
-        let s = self.symbol()?;
+        let s = self.expect_symbol()?;
         self.str_to_ident(pos, &s)
     }
 
@@ -216,7 +229,7 @@ impl<'a> Parser<'a> {
         let mut is_nodebug = false;
 
         while self.lexer.peek().map_or(false, |(_pos, tok)| tok.is_sym()) {
-            let sym = self.symbol()?;
+            let sym = self.expect_symbol()?;
             if sym == "extern" {
                 is_extern = true;
             } else if sym == "nodebug" {
@@ -241,20 +254,18 @@ impl<'a> Parser<'a> {
 
     fn parse_typevalue(&mut self) -> Result<TypeValue> {
         let pos = self.pos();
-        self.lparen()?;
-        if self.is_sym_str("primitive") {
-            self.symbol()?;
+        self.expect_lparen()?;
+        if self.eat_sym_str("primitive")? {
             let primitive_ident = self.parse_ident()?;
-            self.rparen()?;
+            self.expect_rparen()?;
             Ok(TypeValue::Primitive(primitive_ident, pos))
-        } else if self.is_sym_str("enum") {
-            self.symbol()?;
+        } else if self.eat_sym_str("enum")? {
             let mut variants = vec![];
             while !self.is_rparen() {
                 let variant = self.parse_type_variant()?;
                 variants.push(variant);
             }
-            self.rparen()?;
+            self.expect_rparen()?;
             Ok(TypeValue::Enum(variants, pos))
         } else {
             Err(self.error(pos, "Unknown type definition".to_string()))
@@ -272,56 +283,41 @@ impl<'a> Parser<'a> {
             })
         } else {
             let pos = self.pos();
-            self.lparen()?;
+            self.expect_lparen()?;
             let name = self.parse_ident()?;
             let mut fields = vec![];
             while !self.is_rparen() {
                 fields.push(self.parse_type_field()?);
             }
-            self.rparen()?;
+            self.expect_rparen()?;
             Ok(Variant { name, fields, pos })
         }
     }
 
     fn parse_type_field(&mut self) -> Result<Field> {
         let pos = self.pos();
-        self.lparen()?;
+        self.expect_lparen()?;
         let name = self.parse_ident()?;
         let ty = self.parse_ident()?;
-        self.rparen()?;
+        self.expect_rparen()?;
         Ok(Field { name, ty, pos })
     }
 
     fn parse_decl(&mut self) -> Result<Decl> {
         let pos = self.pos();
 
-        let pure = if self.is_sym_str("pure") {
-            self.symbol()?;
-            true
-        } else {
-            false
-        };
-        let multi = if self.is_sym_str("multi") {
-            self.symbol()?;
-            true
-        } else {
-            false
-        };
-        let partial = if self.is_sym_str("partial") {
-            self.symbol()?;
-            true
-        } else {
-            false
-        };
+        let pure = self.eat_sym_str("pure")?;
+        let multi = self.eat_sym_str("multi")?;
+        let partial = self.eat_sym_str("partial")?;
 
         let term = self.parse_ident()?;
 
-        self.lparen()?;
+        self.expect_lparen()?;
         let mut arg_tys = vec![];
         while !self.is_rparen() {
             arg_tys.push(self.parse_ident()?);
         }
-        self.rparen()?;
+        self.expect_rparen()?;
 
         let ret_ty = self.parse_ident()?;
 
@@ -338,21 +334,12 @@ impl<'a> Parser<'a> {
 
     fn parse_extern(&mut self) -> Result<Extern> {
         let pos = self.pos();
-        if self.is_sym_str("constructor") {
-            self.symbol()?;
-
+        if self.eat_sym_str("constructor")? {
             let term = self.parse_ident()?;
             let func = self.parse_ident()?;
             Ok(Extern::Constructor { term, func, pos })
-        } else if self.is_sym_str("extractor") {
-            self.symbol()?;
-
-            let infallible = if self.is_sym_str("infallible") {
-                self.symbol()?;
-                true
-            } else {
-                false
-            };
+        } else if self.eat_sym_str("extractor")? {
+            let infallible = self.eat_sym_str("infallible")?;
 
             let term = self.parse_ident()?;
             let func = self.parse_ident()?;
@@ -363,8 +350,7 @@ impl<'a> Parser<'a> {
                 pos,
                 infallible,
             })
-        } else if self.is_sym_str("const") {
-            self.symbol()?;
+        } else if self.eat_sym_str("const")? {
             let pos = self.pos();
             let name = self.parse_const()?;
             let ty = self.parse_ident()?;
@@ -380,13 +366,13 @@ impl<'a> Parser<'a> {
 
     fn parse_etor(&mut self) -> Result<Extractor> {
         let pos = self.pos();
-        self.lparen()?;
+        self.expect_lparen()?;
         let term = self.parse_ident()?;
         let mut args = vec![];
         while !self.is_rparen() {
             args.push(self.parse_ident()?);
         }
-        self.rparen()?;
+        self.expect_rparen()?;
         let template = self.parse_pattern()?;
         Ok(Extractor {
             term,
@@ -400,7 +386,7 @@ impl<'a> Parser<'a> {
         let pos = self.pos();
         let prio = if self.is_int() {
             Some(
-                i64::try_from(self.int()?)
+                i64::try_from(self.expect_int()?)
                     .map_err(|err| self.error(pos, format!("Invalid rule priority: {}", err)))?,
             )
         } else {
@@ -430,34 +416,31 @@ impl<'a> Parser<'a> {
         let pos = self.pos();
         if self.is_int() {
             Ok(Pattern::ConstInt {
-                val: self.int()?,
+                val: self.expect_int()?,
                 pos,
             })
         } else if self.is_const() {
             let val = self.parse_const()?;
             Ok(Pattern::ConstPrim { val, pos })
-        } else if self.is_sym_str("_") {
-            self.symbol()?;
+        } else if self.eat_sym_str("_")? {
             Ok(Pattern::Wildcard { pos })
         } else if self.is_sym() {
-            let s = self.symbol()?;
-            let var = self.str_to_ident(pos, &s)?;
+            let var = self.parse_ident()?;
             if self.is_at() {
-                self.at()?;
+                self.expect_at()?;
                 let subpat = Box::new(self.parse_pattern()?);
                 Ok(Pattern::BindPattern { var, subpat, pos })
             } else {
                 Ok(Pattern::Var { var, pos })
             }
         } else if self.is_lparen() {
-            self.lparen()?;
-            if self.is_sym_str("and") {
-                self.symbol()?;
+            self.expect_lparen()?;
+            if self.eat_sym_str("and")? {
                 let mut subpats = vec![];
                 while !self.is_rparen() {
                     subpats.push(self.parse_pattern()?);
                 }
-                self.rparen()?;
+                self.expect_rparen()?;
                 Ok(Pattern::And { subpats, pos })
             } else {
                 let sym = self.parse_ident()?;
@@ -465,7 +448,7 @@ impl<'a> Parser<'a> {
                 while !self.is_rparen() {
                     args.push(self.parse_pattern()?);
                 }
-                self.rparen()?;
+                self.expect_rparen()?;
                 Ok(Pattern::Term { sym, args, pos })
             }
         } else {
@@ -476,19 +459,17 @@ impl<'a> Parser<'a> {
     fn parse_iflet_or_expr(&mut self) -> Result<IfLetOrExpr> {
         let pos = self.pos();
         if self.is_lparen() {
-            self.lparen()?;
-            let ret = if self.is_sym_str("if-let") {
-                self.symbol()?;
+            self.expect_lparen()?;
+            let ret = if self.eat_sym_str("if-let")? {
                 IfLetOrExpr::IfLet(self.parse_iflet()?)
-            } else if self.is_sym_str("if") {
+            } else if self.eat_sym_str("if")? {
                 // Shorthand form: `(if (x))` desugars to `(if-let _
                 // (x))`.
-                self.symbol()?;
                 IfLetOrExpr::IfLet(self.parse_iflet_if()?)
             } else {
                 IfLetOrExpr::Expr(self.parse_expr_inner_parens(pos)?)
             };
-            self.rparen()?;
+            self.expect_rparen()?;
             Ok(ret)
         } else {
             self.parse_expr().map(|expr| IfLetOrExpr::Expr(expr))
@@ -515,15 +496,13 @@ impl<'a> Parser<'a> {
     fn parse_expr(&mut self) -> Result<Expr> {
         let pos = self.pos();
         if self.is_lparen() {
-            self.lparen()?;
+            self.expect_lparen()?;
             let ret = self.parse_expr_inner_parens(pos)?;
-            self.rparen()?;
+            self.expect_rparen()?;
             Ok(ret)
-        } else if self.is_sym_str("#t") {
-            self.symbol()?;
+        } else if self.eat_sym_str("#t")? {
             Ok(Expr::ConstInt { val: 1, pos })
-        } else if self.is_sym_str("#f") {
-            self.symbol()?;
+        } else if self.eat_sym_str("#f")? {
             Ok(Expr::ConstInt { val: 0, pos })
         } else if self.is_const() {
             let val = self.parse_const()?;
@@ -532,7 +511,7 @@ impl<'a> Parser<'a> {
             let name = self.parse_ident()?;
             Ok(Expr::Var { name, pos })
         } else if self.is_int() {
-            let val = self.int()?;
+            let val = self.expect_int()?;
             Ok(Expr::ConstInt { val, pos })
         } else {
             Err(self.error(pos, "Invalid expression".into()))
@@ -540,15 +519,14 @@ impl<'a> Parser<'a> {
     }
 
     fn parse_expr_inner_parens(&mut self, pos: Pos) -> Result<Expr> {
-        if self.is_sym_str("let") {
-            self.symbol()?;
-            self.lparen()?;
+        if self.eat_sym_str("let")? {
+            self.expect_lparen()?;
             let mut defs = vec![];
             while !self.is_rparen() {
                 let def = self.parse_letdef()?;
                 defs.push(def);
             }
-            self.rparen()?;
+            self.expect_rparen()?;
             let body = Box::new(self.parse_expr()?);
             Ok(Expr::Let { defs, body, pos })
         } else {
@@ -563,11 +541,11 @@ impl<'a> Parser<'a> {
 
     fn parse_letdef(&mut self) -> Result<LetDef> {
         let pos = self.pos();
-        self.lparen()?;
+        self.expect_lparen()?;
         let var = self.parse_ident()?;
         let ty = self.parse_ident()?;
         let val = Box::new(self.parse_expr()?);
-        self.rparen()?;
+        self.expect_rparen()?;
         Ok(LetDef { var, ty, val, pos })
     }
 


Test patch:
diff --git a/cranelift/filetests/filetests/egraph/issue-5716.clif b/cranelift/filetests/filetests/egraph/issue-5716.clif
new file mode 100644
index 000000000..7763d3eac
--- /dev/null
+++ b/cranelift/filetests/filetests/egraph/issue-5716.clif
@@ -0,0 +1,41 @@
+;; This tests that a call does not get rematerialized, even if a remat flag is
+;; set on a different node in its eclass.
+;;
+;; Below, `v97` is an add of `v238` (the call's first return value) and a
+;; constant 0; a mid-end rule rewrites this to just `v238` (i.e., `v97` is unioned
+;; in). Separately, a rule states that an add of a value and a constant always
+;; gets rematerialized at use. When `v97` is used in a later block, it would have
+;; rematerialized the add; except, if we instead use the result of the call
+;; directly, we should *not* remat the call. If we do, a compile error results
+;; later.
+
+test compile
+set opt_level=speed_and_size
+target aarch64
+
+function u0:33() system_v {
+ss0 = explicit_slot 32
+sig0 = (i64, i64, i64, i64, i64) -> i64, i64 system_v
+fn0 = colocated u0:0 sig0
+jt0 = jump_table [block36, block38]
+block0:
+  v80 = iconst.i32 0
+  v91 = iconst.i64 0
+  v92 = iconst.i64 0
+  v96 = iconst.i64 0
+  v235 = iconst.i64 0
+  v236 = iconst.i64 0
+  v237 = iconst.i64 0
+  v238, v239 = call fn0(v236, v237, v91, v92, v235) ; v236 = 0, v237 = 0, v91 = 0, v92 = 0, v235 = 0
+  v97 = iadd v238, v96 ; v96 = 0
+  br_table v80, block37, jt0 ; v80 = 0
+block36:
+  trap user0
+block37:
+  trap unreachable
+block38:
+  v98 = load.i8 notrap v97
+  v99 = fcvt_from_uint.f64 v98
+  stack_store v99, ss0
+  trap user0
+}