From 34f7a990b752c2af77f612cd6359533fea8b9f64 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Wed, 22 Nov 2023 15:13:30 +0000 Subject: [PATCH 1/3] Disallow blockparams on branches with multiple successors Since the input CFG is required to not have critical edges, such blockparams are useless. The incoming blockparams can simply be replaced with the vreg from the (unique) predecessor. It's better to let the client's lowering code handle this so regalloc2 doesn't need to. --- src/checker.rs | 45 ++++++++++++++++++++----------------------- src/fuzzing/func.rs | 36 +++++++++++++++------------------- src/ion/liveranges.rs | 42 ++++++++++++++++++---------------------- src/lib.rs | 13 +++++++++---- src/serialize.rs | 21 +++++--------------- src/ssa.rs | 25 ++++++++++++++++++++---- 6 files changed, 90 insertions(+), 92 deletions(-) diff --git a/src/checker.rs b/src/checker.rs index 33035615..23572bc0 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -272,10 +272,8 @@ fn visit_all_vregs(f: &F, mut v: V) { v(op.vreg()); } if f.is_branch(inst) { - for succ_idx in 0..f.block_succs(block).len() { - for ¶m in f.branch_blockparams(block, inst, succ_idx) { - v(param); - } + for ¶m in f.branch_blockparams(block, inst) { + v(param); } } } @@ -869,28 +867,27 @@ impl<'a, F: Function> Checker<'a, F> { 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 + // Instead, 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 }); - } + let succ = *self.f.block_succs(block).first().unwrap(); + let args = self.f.branch_blockparams(block, inst); + 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 }); } } } diff --git a/src/fuzzing/func.rs b/src/fuzzing/func.rs index c6063e06..8fde0b22 100644 --- a/src/fuzzing/func.rs +++ b/src/fuzzing/func.rs @@ -55,7 +55,7 @@ pub struct Func { block_preds: Vec>, block_succs: Vec>, block_params_in: Vec>, - block_params_out: Vec>>, + block_params_out: Vec>, num_vregs: usize, reftype_vregs: Vec, 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, _: Inst) -> &[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>) { + pub fn set_block_params_out(&mut self, block: Block, params: Vec) { 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,11 +569,10 @@ impl Func { .copied() .collect(); let vreg = u.choose(&suitable_vregs)?; - args.push(*vreg); + params.push(*vreg); } - params.push(args); + builder.set_block_params_out(Block::new(block), params); } - 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 +607,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::>() - .join(", "); - format!("block{}({})", succ.index(), params) - }) + .map(|v| format!("v{}", v.vreg())) .collect::>() .join(", "); write!( diff --git a/src/ion/liveranges.rs b/src/ion/liveranges.rs index 09644a24..fb59fea3 100644 --- a/src/ion/liveranges.rs +++ b/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 ¶m in self.func.branch_blockparams(block, insns.last(), i) { - live.set(param.vreg(), true); - self.observe_vreg_class(param); - } + for ¶m in self.func.branch_blockparams(block, insns.last()) { + 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, insns.last()); + 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); } } diff --git a/src/lib.rs b/src/lib.rs index 3ce07863..8597c7d2 100644 --- a/src/lib.rs +++ b/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 { @@ -1043,10 +1048,10 @@ pub trait Function { 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]; + /// 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, insn: Inst) -> &[VReg]; /// Determine whether an instruction requires all reference-typed /// values to be placed onto the stack. For these instructions, diff --git a/src/serialize.rs b/src/serialize.rs index a74a60ec..68ec2ab3 100644 --- a/src/serialize.rs +++ b/src/serialize.rs @@ -45,7 +45,7 @@ pub struct SerializableFunction { block_preds: Vec>, block_succs: Vec>, block_params_in: Vec>, - block_params_out: Vec>>, + block_params_out: Vec>, num_vregs: usize, reftype_vregs: Vec, debug_value_labels: Vec<(VReg, Inst, Inst, u32)>, @@ -107,9 +107,7 @@ impl SerializableFunction { .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, inst).to_vec() }) .collect(), num_vregs: func.num_vregs(), @@ -169,8 +167,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, _: Inst) -> &[VReg] { + &self.block_params_out[block.index()][..] } fn requires_refs_on_stack(&self, insn: Inst) -> bool { @@ -258,16 +256,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::>() - .join(", "); - format!("block{}({})", succ.index(), params) - }) + .map(|v| format!("v{}", v.vreg())) .collect::>() .join(", "); write!( diff --git a/src/ssa.rs b/src/ssa.rs index ac6263ef..679711b6 100644 --- a/src/ssa.rs +++ b/src/ssa.rs @@ -88,8 +88,8 @@ pub fn validate_ssa(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: &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, insn); + 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 {}", From 09433ff8507819ebfa63791bed14a1ca10fc6451 Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Wed, 6 Dec 2023 00:33:09 +0000 Subject: [PATCH 2/3] Also remove the `inst` argument on `branch_blockparams` It isn't needed when there is only one successor. --- src/checker.rs | 4 ++-- src/fuzzing/func.rs | 2 +- src/ion/liveranges.rs | 4 ++-- src/lib.rs | 4 ++-- src/serialize.rs | 5 ++--- src/ssa.rs | 2 +- 6 files changed, 10 insertions(+), 11 deletions(-) diff --git a/src/checker.rs b/src/checker.rs index 23572bc0..a2bec447 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -272,7 +272,7 @@ fn visit_all_vregs(f: &F, mut v: V) { v(op.vreg()); } if f.is_branch(inst) { - for ¶m in f.branch_blockparams(block, inst) { + for ¶m in f.branch_blockparams(block) { v(param); } } @@ -871,7 +871,7 @@ impl<'a, F: Function> Checker<'a, F> { // outgoing edge as necessary to handle blockparams. else { let succ = *self.f.block_succs(block).first().unwrap(); - let args = self.f.branch_blockparams(block, inst); + let args = self.f.branch_blockparams(block); let params = self.f.block_params(succ); assert_eq!( args.len(), diff --git a/src/fuzzing/func.rs b/src/fuzzing/func.rs index 8fde0b22..eb89eeb3 100644 --- a/src/fuzzing/func.rs +++ b/src/fuzzing/func.rs @@ -99,7 +99,7 @@ impl Function for Func { self.insts[insn.index()].op == InstOpcode::Branch } - fn branch_blockparams(&self, block: Block, _: Inst) -> &[VReg] { + fn branch_blockparams(&self, block: Block) -> &[VReg] { &self.block_params_out[block.index()][..] } diff --git a/src/ion/liveranges.rs b/src/ion/liveranges.rs index fb59fea3..290b6631 100644 --- a/src/ion/liveranges.rs +++ b/src/ion/liveranges.rs @@ -307,7 +307,7 @@ impl<'a, F: Function> Env<'a, F> { // Include outgoing blockparams in the initial live set. if self.func.is_branch(insns.last()) { - for ¶m in self.func.branch_blockparams(block, insns.last()) { + for ¶m in self.func.branch_blockparams(block) { live.set(param.vreg(), true); self.observe_vreg_class(param); } @@ -399,7 +399,7 @@ impl<'a, F: Function> Env<'a, F> { if self.func.is_branch(insns.last()) { 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, insns.last()); + 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()); diff --git a/src/lib.rs b/src/lib.rs index 8597c7d2..031ddc49 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1047,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 + /// 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, insn: Inst) -> &[VReg]; + 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, diff --git a/src/serialize.rs b/src/serialize.rs index 68ec2ab3..c63a9981 100644 --- a/src/serialize.rs +++ b/src/serialize.rs @@ -106,8 +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(); - func.branch_blockparams(block, inst).to_vec() + func.branch_blockparams(block).to_vec() }) .collect(), num_vregs: func.num_vregs(), @@ -167,7 +166,7 @@ impl Function for SerializableFunction { self.insts[insn.index()].op == InstOpcode::Branch } - fn branch_blockparams(&self, block: Block, _: Inst) -> &[VReg] { + fn branch_blockparams(&self, block: Block) -> &[VReg] { &self.block_params_out[block.index()][..] } diff --git a/src/ssa.rs b/src/ssa.rs index 679711b6..cd5a21d7 100644 --- a/src/ssa.rs +++ b/src/ssa.rs @@ -102,7 +102,7 @@ pub fn validate_ssa(f: &F, cfginfo: &CFGInfo) -> Result<(), RegAllo return Err(RegAllocError::BB(block)); } if f.is_branch(insn) { - let blockparams_out = f.branch_blockparams(block, insn); + 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) { From 22a5791188707703445cb10fa80458282d04dcea Mon Sep 17 00:00:00 2001 From: Amanieu d'Antras Date: Tue, 12 Dec 2023 01:23:20 +0000 Subject: [PATCH 3/3] Verify that branches with operands are properly supported This extends the fuzzer and checker to verify that branches that produce outputs are properly supported and no moves are inserted after a branch. This also checks the one edge case where a branch instruction may have both operands and blockparams: when there is only one successor and that successor only has one predecessor. --- src/checker.rs | 38 ++++++++++++++++++++------------------ src/fuzzing/func.rs | 24 +++++++++++++++++++++++- 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/src/checker.rs b/src/checker.rs index a2bec447..a1c97459 100644 --- a/src/checker.rs +++ b/src/checker.rs @@ -822,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); } } @@ -851,25 +857,21 @@ 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 the + 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 { + 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); diff --git a/src/fuzzing/func.rs b/src/fuzzing/func.rs index eb89eeb3..814b4745 100644 --- a/src/fuzzing/func.rs +++ b/src/fuzzing/func.rs @@ -572,8 +572,30 @@ impl Func { 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()); + } } - builder.add_inst(Block::new(block), InstData::branch()); } else { builder.add_inst(Block::new(block), InstData::ret()); }