Disallow blockparams on branches with multiple successors

src/checker.rs

@@ -272,10 +272,8 @@ fn visit_all_vregs<F: Function, V: FnMut(VReg)>(f: &F, mut v: V) {
                 v(op.vreg());
             }
             if f.is_branch(inst) {
-                for succ_idx in 0..f.block_succs(block).len() {
-                    for &param in f.branch_blockparams(block, inst, succ_idx) {
-                        v(param);
-                    }
+                for &param in f.branch_blockparams(block) {
+                    v(param);
                 }
             }
         }
@@ -824,16 +822,22 @@ impl<'a, F: Function> Checker<'a, F> {
         let mut last_inst = None;
         for block in 0..self.f.num_blocks() {
             let block = Block::new(block);
+            let mut terminated = false;
             for inst_or_edit in out.block_insts_and_edits(self.f, block) {
+                assert!(!terminated);
                 match inst_or_edit {
                     InstOrEdit::Inst(inst) => {
                         debug_assert!(last_inst.is_none() || inst > last_inst.unwrap());
                         last_inst = Some(inst);
                         self.handle_inst(block, inst, &mut safepoint_slots, out);
+                        if self.f.is_branch(inst) || self.f.is_ret(inst) {
+                            terminated = true;
+                        }
                     }
                     InstOrEdit::Edit(edit) => self.handle_edit(block, edit),
                 }
             }
+            assert!(terminated);
         }
     }
 
@@ -853,44 +857,39 @@ impl<'a, F: Function> Checker<'a, F> {
             self.bb_insts.get_mut(&block).unwrap().push(checkinst);
         }
 
-        // Skip normal checks if this is a branch: the blockparams do
-        // not exist in post-regalloc code, and the edge-moves have to
-        // be inserted before the branch rather than after.
-        if !self.f.is_branch(inst) {
-            let operands: Vec<_> = self.f.inst_operands(inst).iter().cloned().collect();
-            let allocs: Vec<_> = out.inst_allocs(inst).iter().cloned().collect();
-            let clobbers: Vec<_> = self.f.inst_clobbers(inst).into_iter().collect();
-            let checkinst = CheckerInst::Op {
-                inst,
-                operands,
-                allocs,
-                clobbers,
-            };
-            trace!("checker: adding inst {:?}", checkinst);
-            self.bb_insts.get_mut(&block).unwrap().push(checkinst);
-        }
-        // Instead, if this is a branch, emit a ParallelMove on each
+        let operands: Vec<_> = self.f.inst_operands(inst).iter().cloned().collect();
+        let allocs: Vec<_> = out.inst_allocs(inst).iter().cloned().collect();
+        let clobbers: Vec<_> = self.f.inst_clobbers(inst).into_iter().collect();
+        let checkinst = CheckerInst::Op {
+            inst,
+            operands,
+            allocs,
+            clobbers,
+        };
+        trace!("checker: adding inst {:?}", checkinst);
+        self.bb_insts.get_mut(&block).unwrap().push(checkinst);
+
+        // If this is a branch, emit a ParallelMove on the
         // outgoing edge as necessary to handle blockparams.
-        else {
-            for (i, &succ) in self.f.block_succs(block).iter().enumerate() {
-                let args = self.f.branch_blockparams(block, inst, i);
-                let params = self.f.block_params(succ);
-                assert_eq!(
-                    args.len(),
-                    params.len(),
-                    "block{} has succ block{}; gave {} args for {} params",
-                    block.index(),
-                    succ.index(),
-                    args.len(),
-                    params.len()
-                );
-                if args.len() > 0 {
-                    let moves = params.iter().cloned().zip(args.iter().cloned()).collect();
-                    self.edge_insts
-                        .get_mut(&(block, succ))
-                        .unwrap()
-                        .push(CheckerInst::ParallelMove { moves });
-                }
+        if self.f.is_branch(inst) {
+            let succ = *self.f.block_succs(block).first().unwrap();
+            let args = self.f.branch_blockparams(block);
+            let params = self.f.block_params(succ);
+            assert_eq!(
+                args.len(),
+                params.len(),
+                "block{} has succ block{}; gave {} args for {} params",
+                block.index(),
+                succ.index(),
+                args.len(),
+                params.len()
+            );
+            if args.len() > 0 {
+                let moves = params.iter().cloned().zip(args.iter().cloned()).collect();
+                self.edge_insts
+                    .get_mut(&(block, succ))
+                    .unwrap()
+                    .push(CheckerInst::ParallelMove { moves });
             }
         }
     }

src/fuzzing/func.rs

@@ -55,7 +55,7 @@ pub struct Func {
     block_preds: Vec<Vec<Block>>,
     block_succs: Vec<Vec<Block>>,
     block_params_in: Vec<Vec<VReg>>,
-    block_params_out: Vec<Vec<Vec<VReg>>>,
+    block_params_out: Vec<Vec<VReg>>,
     num_vregs: usize,
     reftype_vregs: Vec<VReg>,
     debug_value_labels: Vec<(VReg, Inst, Inst, u32)>,
@@ -99,8 +99,8 @@ impl Function for Func {
         self.insts[insn.index()].op == InstOpcode::Branch
     }
 
-    fn branch_blockparams(&self, block: Block, _: Inst, succ: usize) -> &[VReg] {
-        &self.block_params_out[block.index()][succ][..]
+    fn branch_blockparams(&self, block: Block) -> &[VReg] {
+        &self.block_params_out[block.index()][..]
     }
 
     fn requires_refs_on_stack(&self, insn: Inst) -> bool {
@@ -194,7 +194,7 @@ impl FuncBuilder {
         self.f.block_params_in[block.index()] = params.iter().cloned().collect();
     }
 
-    pub fn set_block_params_out(&mut self, block: Block, params: Vec<Vec<VReg>>) {
+    pub fn set_block_params_out(&mut self, block: Block, params: Vec<VReg>) {
         self.f.block_params_out[block.index()] = params;
     }
 
@@ -383,7 +383,11 @@ impl Func {
             let mut vregs_to_be_defined = vec![];
             let mut max_block_params = u.int_in_range(0..=core::cmp::min(3, vregs.len() / 3))?;
             for &vreg in &vregs {
-                if block > 0 && bool::arbitrary(u)? && max_block_params > 0 {
+                if block > 0
+                    && bool::arbitrary(u)?
+                    && max_block_params > 0
+                    && builder.f.block_preds[block].len() > 1
+                {
                     block_params[block].push(vreg);
                     max_block_params -= 1;
                 } else {
@@ -538,9 +542,9 @@ impl Func {
             // Define the branch with blockparam args that must end
             // the block.
             if builder.f.block_succs[block].len() > 0 {
-                let mut params = vec![];
-                for &succ in &builder.f.block_succs[block] {
-                    let mut args = vec![];
+                // Only add blockparams if there is a single successor.
+                if let [succ] = builder.f.block_succs[block][..] {
+                    let mut params = vec![];
                     for i in 0..builder.f.block_params_in[succ.index()].len() {
                         let dom_block = choose_dominating_block(
                             &builder.idom[..],
@@ -565,12 +569,33 @@ impl Func {
                             .copied()
                             .collect();
                         let vreg = u.choose(&suitable_vregs)?;
-                        args.push(*vreg);
+                        params.push(*vreg);
+                    }
+                    builder.set_block_params_out(Block::new(block), params);
+
+                    // If the unique successor only has a single predecessor,
+                    // we can have both blockparams and operands. Turn the last
+                    // instruction into a branch.
+                    let succ_preds = builder.f.block_preds[succ.index()].len()
+                        + if succ == Block(0) { 1 } else { 0 };
+                    if succ_preds == 1 {
+                        if let Some(last) = builder.insts_per_block[block].last_mut() {
+                            last.op = InstOpcode::Branch;
+                        } else {
+                            builder.add_inst(Block::new(block), InstData::branch());
+                        }
+                    } else {
+                        builder.add_inst(Block::new(block), InstData::branch());
+                    }
+                } else {
+                    // If a branch doesn't have blockparams then it is allowed
+                    // have operands. Turn the last instruction into a branch.
+                    if let Some(last) = builder.insts_per_block[block].last_mut() {
+                        last.op = InstOpcode::Branch;
+                    } else {
+                        builder.add_inst(Block::new(block), InstData::branch());
                     }
-                    params.push(args);
                 }
-                builder.set_block_params_out(Block::new(block), params);
-                builder.add_inst(Block::new(block), InstData::branch());
             } else {
                 builder.add_inst(Block::new(block), InstData::ret());
             }
@@ -604,16 +629,7 @@ impl core::fmt::Debug for Func {
                 .join(", ");
             let params_out = self.block_params_out[i]
                 .iter()
-                .enumerate()
-                .map(|(succ_idx, vec)| {
-                    let succ = self.block_succs[i][succ_idx];
-                    let params = vec
-                        .iter()
-                        .map(|v| format!("v{}", v.vreg()))
-                        .collect::<Vec<_>>()
-                        .join(", ");
-                    format!("block{}({})", succ.index(), params)
-                })
+                .map(|v| format!("v{}", v.vreg()))
                 .collect::<Vec<_>>()
                 .join(", ");
             write!(

src/ion/liveranges.rs

@@ -307,11 +307,9 @@ impl<'a, F: Function> Env<'a, F> {
 
             // Include outgoing blockparams in the initial live set.
             if self.func.is_branch(insns.last()) {
-                for i in 0..self.func.block_succs(block).len() {
-                    for &param in self.func.branch_blockparams(block, insns.last(), i) {
-                        live.set(param.vreg(), true);
-                        self.observe_vreg_class(param);
-                    }
+                for &param in self.func.branch_blockparams(block) {
+                    live.set(param.vreg(), true);
+                    self.observe_vreg_class(param);
                 }
             }
 
@@ -399,24 +397,22 @@ impl<'a, F: Function> Env<'a, F> {
             // If the last instruction is a branch (rather than
             // return), create blockparam_out entries.
             if self.func.is_branch(insns.last()) {
-                for (i, &succ) in self.func.block_succs(block).iter().enumerate() {
-                    let blockparams_in = self.func.block_params(succ);
-                    let blockparams_out = self.func.branch_blockparams(block, insns.last(), i);
-                    for (&blockparam_in, &blockparam_out) in
-                        blockparams_in.iter().zip(blockparams_out)
-                    {
-                        let blockparam_out = VRegIndex::new(blockparam_out.vreg());
-                        let blockparam_in = VRegIndex::new(blockparam_in.vreg());
-                        self.blockparam_outs.push(BlockparamOut {
-                            to_vreg: blockparam_in,
-                            to_block: succ,
-                            from_block: block,
-                            from_vreg: blockparam_out,
-                        });
-
-                        // Include outgoing blockparams in the initial live set.
-                        live.set(blockparam_out.index(), true);
-                    }
+                let succ = *self.func.block_succs(block).first().unwrap();
+                let blockparams_in = self.func.block_params(succ);
+                let blockparams_out = self.func.branch_blockparams(block);
+                for (&blockparam_in, &blockparam_out) in blockparams_in.iter().zip(blockparams_out)
+                {
+                    let blockparam_out = VRegIndex::new(blockparam_out.vreg());
+                    let blockparam_in = VRegIndex::new(blockparam_in.vreg());
+                    self.blockparam_outs.push(BlockparamOut {
+                        to_vreg: blockparam_in,
+                        to_block: succ,
+                        from_block: block,
+                        from_vreg: blockparam_out,
+                    });
+
+                    // Include outgoing blockparams in the initial live set.
+                    live.set(blockparam_out.index(), true);
                 }
             }
 

src/lib.rs

@@ -240,6 +240,11 @@ impl PRegSet {
         self.bits[0] |= other.bits[0];
         self.bits[1] |= other.bits[1];
     }
+
+    /// Checks whether the `PRegSet` is empty.
+    fn is_empty(self) -> bool {
+        self == Self::empty()
+    }
 }
 
 impl IntoIterator for PRegSet {
@@ -1042,11 +1047,11 @@ pub trait Function {
     /// branch.
     fn is_branch(&self, insn: Inst) -> bool;
 
-    /// If `insn` is a branch at the end of `block`, returns the
-    /// outgoing blockparam arguments for the given successor. The
-    /// number of arguments must match the number incoming blockparams
-    /// for each respective successor block.
-    fn branch_blockparams(&self, block: Block, insn: Inst, succ_idx: usize) -> &[VReg];
+    /// If `block` ends with a branch instruction, returns the
+    /// outgoing blockparam arguments. Branch arguments are only
+    /// allowed on blocks with a single successor. The number of
+    /// arguments must match the number incoming blockparams.
+    fn branch_blockparams(&self, block: Block) -> &[VReg];
 
     /// Determine whether an instruction requires all reference-typed
     /// values to be placed onto the stack. For these instructions,

src/serialize.rs

@@ -45,7 +45,7 @@ pub struct SerializableFunction {
     block_preds: Vec<Vec<Block>>,
     block_succs: Vec<Vec<Block>>,
     block_params_in: Vec<Vec<VReg>>,
-    block_params_out: Vec<Vec<Vec<VReg>>>,
+    block_params_out: Vec<Vec<VReg>>,
     num_vregs: usize,
     reftype_vregs: Vec<VReg>,
     debug_value_labels: Vec<(VReg, Inst, Inst, u32)>,
@@ -106,10 +106,7 @@ impl SerializableFunction {
             block_params_out: (0..func.num_blocks())
                 .map(|i| {
                     let block = Block::new(i);
-                    let inst = func.block_insns(block).last();
-                    (0..func.block_succs(block).len())
-                        .map(|succ_idx| func.branch_blockparams(block, inst, succ_idx).to_vec())
-                        .collect()
+                    func.branch_blockparams(block).to_vec()
                 })
                 .collect(),
             num_vregs: func.num_vregs(),
@@ -169,8 +166,8 @@ impl Function for SerializableFunction {
         self.insts[insn.index()].op == InstOpcode::Branch
     }
 
-    fn branch_blockparams(&self, block: Block, _: Inst, succ: usize) -> &[VReg] {
-        &self.block_params_out[block.index()][succ][..]
+    fn branch_blockparams(&self, block: Block) -> &[VReg] {
+        &self.block_params_out[block.index()][..]
     }
 
     fn requires_refs_on_stack(&self, insn: Inst) -> bool {
@@ -258,16 +255,7 @@ impl fmt::Debug for SerializableFunction {
                 .join(", ");
             let params_out = self.block_params_out[i]
                 .iter()
-                .enumerate()
-                .map(|(succ_idx, vec)| {
-                    let succ = self.block_succs[i][succ_idx];
-                    let params = vec
-                        .iter()
-                        .map(|v| format!("v{}", v.vreg()))
-                        .collect::<Vec<_>>()
-                        .join(", ");
-                    format!("block{}({})", succ.index(), params)
-                })
+                .map(|v| format!("v{}", v.vreg()))
                 .collect::<Vec<_>>()
                 .join(", ");
             write!(

src/ssa.rs

@@ -88,8 +88,8 @@ pub fn validate_ssa<F: Function>(f: &F, cfginfo: &CFGInfo) -> Result<(), RegAllo
         }
     }
 
-    // Check that the length of branch args matches the sum of the
-    // number of blockparams in their succs, and that the end of every
+    // Check that the length of branch args matches the number of
+    // blockparams in their succs, and that the end of every
     // block ends in this branch or in a ret, and that there are no
     // other branches or rets in the middle of the block.
     for block in 0..f.num_blocks() {
@@ -102,9 +102,26 @@ pub fn validate_ssa<F: Function>(f: &F, cfginfo: &CFGInfo) -> Result<(), RegAllo
                     return Err(RegAllocError::BB(block));
                 }
                 if f.is_branch(insn) {
-                    for (i, &succ) in f.block_succs(block).iter().enumerate() {
+                    let blockparams_out = f.branch_blockparams(block);
+                    if !blockparams_out.is_empty() {
+                        // Branches with blockparams can only have 1 successor.
+                        let succ = match f.block_succs(block) {
+                            &[succ] => succ,
+                            _ => {
+                                trace!(
+                                    "Block {:?} with outgoing blockparams \
+                                    can only have one successor",
+                                    block
+                                );
+                                return Err(RegAllocError::Branch(insn));
+                            }
+                        };
+                        // ... and aren't allowed to have operands.
+                        if !f.inst_operands(insn).is_empty() || !f.inst_clobbers(insn).is_empty() {
+                            trace!("Branch {:?} with blockparams can't have operands", insn);
+                            return Err(RegAllocError::Branch(insn));
+                        }
                         let blockparams_in = f.block_params(succ);
-                        let blockparams_out = f.branch_blockparams(block, insn, i);
                         if blockparams_in.len() != blockparams_out.len() {
                             trace!(
                                 "Mismatch on block params, found {} expected {}",