From 30aeb5708342de0e913699eba3281e7eb4b7639d Mon Sep 17 00:00:00 2001 From: Jakob Stoklund Olesen Date: Thu, 5 Oct 2017 12:42:19 -0700 Subject: [PATCH] Add a value location verifier. This is a verification pass that can be run after register allocation. It verifies that value locations are consistent with constraints on their uses, and that the register diversions are consistent. Make it clear that register diversions are local to an EBB only. This affects what branch relaxation is allowed to do. The verify_locations() takes an optional Liveness parameter which is used to check that no diverted values are live across CFG edges. --- cranelift/src/filetest/binemit.rs | 2 +- lib/cretonne/src/binemit/relaxation.rs | 21 +- lib/cretonne/src/regalloc/coloring.rs | 2 +- lib/cretonne/src/regalloc/context.rs | 3 +- lib/cretonne/src/regalloc/liverange.rs | 7 + lib/cretonne/src/verifier/locations.rs | 316 +++++++++++++++++++++++++ lib/cretonne/src/verifier/mod.rs | 4 +- 7 files changed, 350 insertions(+), 5 deletions(-) create mode 100644 lib/cretonne/src/verifier/locations.rs diff --git a/cranelift/src/filetest/binemit.rs b/cranelift/src/filetest/binemit.rs index da0e2012a0..ae1c741d13 100644 --- a/cranelift/src/filetest/binemit.rs +++ b/cranelift/src/filetest/binemit.rs @@ -116,6 +116,7 @@ impl SubTest for TestBinEmit { // Give an encoding to any instruction that doesn't already have one. let mut divert = RegDiversions::new(); for ebb in func.layout.ebbs() { + divert.clear(); for inst in func.layout.ebb_insts(ebb) { if !func.encodings[inst].is_legal() { let mut legal_encodings = isa.legal_encodings( @@ -180,7 +181,6 @@ impl SubTest for TestBinEmit { } // Now emit all instructions. - divert.clear(); let mut sink = TextSink::new(isa); for ebb in func.layout.ebbs() { divert.clear(); diff --git a/lib/cretonne/src/binemit/relaxation.rs b/lib/cretonne/src/binemit/relaxation.rs index 4e6d1a2408..94835b736e 100644 --- a/lib/cretonne/src/binemit/relaxation.rs +++ b/lib/cretonne/src/binemit/relaxation.rs @@ -166,5 +166,24 @@ fn relax_branch( return encinfo.bytes(enc); } - unimplemented!(); + // Note: On some RISC ISAs, conditional branches have shorter range than unconditional + // branches, so one way of extending the range of a conditional branch is to invert its + // condition and make it branch over an unconditional jump which has the larger range. + // + // Splitting the EBB is problematic this late because there may be register diversions in + // effect across the conditional branch, and they can't survive the control flow edge to a new + // EBB. We have two options for handling that: + // + // 1. Set a flag on the new EBB that indicates it wants the preserve the register diversions of + // its layout predecessor, or + // 2. Use an encoding macro for the branch-over-jump pattern so we don't need to split the EBB. + // + // It seems that 1. would allow us to share code among RISC ISAs that need this. + // + // We can't allow register diversions to survive from the layout predecessor because the layout + // predecessor could contain kill points for some values that are live in this EBB, and + // diversions are not automatically cancelled when the live range of a value ends. + + // This assumes solution 2. above: + panic!("No branch in range for {:#x}-{:#x}", offset, dest_offset); } diff --git a/lib/cretonne/src/regalloc/coloring.rs b/lib/cretonne/src/regalloc/coloring.rs index 9a92df0270..69e1086416 100644 --- a/lib/cretonne/src/regalloc/coloring.rs +++ b/lib/cretonne/src/regalloc/coloring.rs @@ -468,7 +468,7 @@ impl<'a> Context<'a> { // // Values with a global live range that are not live in to `dest` could appear as branch // arguments, so they can't always be un-diverted. - self.undivert_regs(|lr, func| lr.livein_local_end(dest, &func.layout).is_some()); + self.undivert_regs(|lr, func| lr.is_livein(dest, &func.layout)); // Now handle the EBB arguments. let br_args = self.cur.func.dfg.inst_variable_args(inst); diff --git a/lib/cretonne/src/regalloc/context.rs b/lib/cretonne/src/regalloc/context.rs index 5e5c17c113..06d27ff325 100644 --- a/lib/cretonne/src/regalloc/context.rs +++ b/lib/cretonne/src/regalloc/context.rs @@ -17,7 +17,7 @@ use regalloc::spilling::Spilling; use regalloc::virtregs::VirtRegs; use result::CtonResult; use topo_order::TopoOrder; -use verifier::{verify_context, verify_liveness, verify_cssa}; +use verifier::{verify_context, verify_liveness, verify_cssa, verify_locations}; /// Persistent memory allocations for register allocation. pub struct Context { @@ -138,6 +138,7 @@ impl Context { if isa.flags().enable_verifier() { verify_context(func, cfg, domtree, isa)?; verify_liveness(isa, func, cfg, &self.liveness)?; + verify_locations(isa, func, Some(&self.liveness))?; verify_cssa(func, cfg, domtree, &self.liveness, &self.virtregs)?; } Ok(()) diff --git a/lib/cretonne/src/regalloc/liverange.rs b/lib/cretonne/src/regalloc/liverange.rs index 3da125ef3a..7b94fce280 100644 --- a/lib/cretonne/src/regalloc/liverange.rs +++ b/lib/cretonne/src/regalloc/liverange.rs @@ -383,6 +383,13 @@ impl LiveRange { }) } + /// Is this value live-in to `ebb`? + /// + /// An EBB argument is not considered to be live in. + pub fn is_livein(&self, ebb: Ebb, order: &PO) -> bool { + self.livein_local_end(ebb, order).is_some() + } + /// Get all the live-in intervals. pub fn liveins(&self) -> &[Interval] { &self.liveins diff --git a/lib/cretonne/src/verifier/locations.rs b/lib/cretonne/src/verifier/locations.rs new file mode 100644 index 0000000000..5f84f0403d --- /dev/null +++ b/lib/cretonne/src/verifier/locations.rs @@ -0,0 +1,316 @@ +//! Verify value locations. + +use ir; +use isa; +use regalloc::RegDiversions; +use regalloc::liveness::Liveness; +use verifier::Result; + +/// Verify value locations for `func`. +/// +/// After register allocation, every value must be assigned to a location - either a register or a +/// stack slot. These locations must be compatible with the constraints described by the +/// instruction encoding recipes. +/// +/// Values can be temporarily diverted to a different location by using the `regmove`, `regspill`, +/// and `regfill` instructions, but only inside an EBB. +/// +/// If a liveness analysis is provided, it is used to verify that there are no active register +/// diversions across control flow edges. +pub fn verify_locations( + isa: &isa::TargetIsa, + func: &ir::Function, + liveness: Option<&Liveness>, +) -> Result { + let verifier = LocationVerifier { + isa, + func, + reginfo: isa.register_info(), + encinfo: isa.encoding_info(), + liveness, + }; + verifier.check_constraints()?; + Ok(()) +} + +struct LocationVerifier<'a> { + isa: &'a isa::TargetIsa, + func: &'a ir::Function, + reginfo: isa::RegInfo, + encinfo: isa::EncInfo, + liveness: Option<&'a Liveness>, +} + +impl<'a> LocationVerifier<'a> { + /// Check that the assigned value locations match the operand constraints of their uses. + fn check_constraints(&self) -> Result { + let dfg = &self.func.dfg; + let mut divert = RegDiversions::new(); + + for ebb in self.func.layout.ebbs() { + // Diversions are reset at the top of each EBB. No diversions can exist across control + // flow edges. + divert.clear(); + for inst in self.func.layout.ebb_insts(ebb) { + let enc = self.func.encodings[inst]; + + if enc.is_legal() { + self.check_enc_constraints(inst, enc, &divert)? + } else { + self.check_ghost_results(inst)?; + } + + if let Some(sig) = dfg.call_signature(inst) { + self.check_call_abi(inst, sig, &divert)?; + } + + let opcode = dfg[inst].opcode(); + if opcode.is_return() { + self.check_return_abi(inst, &divert)?; + } + + if opcode.is_branch() && !divert.is_empty() { + self.check_cfg_edges(inst, &divert)?; + } + + self.update_diversions(inst, &mut divert)?; + } + } + + Ok(()) + } + + /// Check encoding constraints against the current value locations. + fn check_enc_constraints( + &self, + inst: ir::Inst, + enc: isa::Encoding, + divert: &RegDiversions, + ) -> Result { + let constraints = self.encinfo.operand_constraints(enc).expect( + "check_enc_constraints requires a legal encoding", + ); + + if constraints.satisfied(inst, divert, self.func) { + return Ok(()); + } + + // TODO: We could give a better error message here. + err!( + inst, + "{} constraints not satisfied", + self.encinfo.display(enc) + ) + } + + /// Check that the result values produced by a ghost instruction are not assigned a value + /// location. + fn check_ghost_results(&self, inst: ir::Inst) -> Result { + let results = self.func.dfg.inst_results(inst); + + for &res in results { + let loc = self.func.locations[res]; + if loc.is_assigned() { + return err!( + inst, + "ghost result {} value must not have a location ({}).", + res, + loc.display(&self.reginfo) + ); + } + } + + Ok(()) + } + + /// Check the ABI argument and result locations for a call. + fn check_call_abi(&self, inst: ir::Inst, sig: ir::SigRef, divert: &RegDiversions) -> Result { + let sig = &self.func.dfg.signatures[sig]; + let varargs = self.func.dfg.inst_variable_args(inst); + let results = self.func.dfg.inst_results(inst); + + for (abi, &value) in sig.argument_types.iter().zip(varargs) { + self.check_abi_location( + inst, + value, + abi, + divert.get(value, &self.func.locations), + ir::StackSlotKind::OutgoingArg, + )?; + } + + for (abi, &value) in sig.return_types.iter().zip(results) { + self.check_abi_location( + inst, + value, + abi, + self.func.locations[value], + ir::StackSlotKind::OutgoingArg, + )?; + } + + Ok(()) + } + + /// Check the ABI argument locations for a return. + fn check_return_abi(&self, inst: ir::Inst, divert: &RegDiversions) -> Result { + let sig = &self.func.signature; + let varargs = self.func.dfg.inst_variable_args(inst); + + for (abi, &value) in sig.return_types.iter().zip(varargs) { + self.check_abi_location( + inst, + value, + abi, + divert.get(value, &self.func.locations), + ir::StackSlotKind::IncomingArg, + )?; + } + + Ok(()) + } + + /// Check a single ABI location. + fn check_abi_location( + &self, + inst: ir::Inst, + value: ir::Value, + abi: &ir::ArgumentType, + loc: ir::ValueLoc, + want_kind: ir::StackSlotKind, + ) -> Result { + match abi.location { + ir::ArgumentLoc::Unassigned => {} + ir::ArgumentLoc::Reg(reg) => { + if loc != ir::ValueLoc::Reg(reg) { + return err!( + inst, + "ABI expects {} in {}, got {}", + value, + abi.location.display(&self.reginfo), + loc.display(&self.reginfo) + ); + } + } + ir::ArgumentLoc::Stack(offset) => { + if let ir::ValueLoc::Stack(ss) = loc { + let slot = &self.func.stack_slots[ss]; + if slot.kind != want_kind { + return err!( + inst, + "call argument {} should be in a {} slot, but {} is {}", + value, + want_kind, + ss, + slot.kind + ); + } + if slot.offset != offset { + return err!( + inst, + "ABI expects {} at stack offset {}, but {} is at {}", + value, + offset, + ss, + slot.offset + ); + } + } else { + return err!( + inst, + "ABI expects {} at stack offset {}, got {}", + value, + offset, + loc.display(&self.reginfo) + ); + } + } + } + + Ok(()) + } + + /// Update diversions to reflect the current instruction and check their consistency. + fn update_diversions(&self, inst: ir::Inst, divert: &mut RegDiversions) -> Result { + let (arg, src) = match self.func.dfg[inst] { + ir::InstructionData::RegMove { arg, src, .. } => (arg, ir::ValueLoc::Reg(src)), + ir::InstructionData::RegSpill { arg, src, .. } => (arg, ir::ValueLoc::Reg(src)), + ir::InstructionData::RegFill { arg, src, .. } => (arg, ir::ValueLoc::Stack(src)), + _ => return Ok(()), + }; + + if let Some(d) = divert.diversion(arg) { + if d.to != src { + return err!( + inst, + "inconsistent with current diversion to {}", + d.to.display(&self.reginfo) + ); + } + } else if self.func.locations[arg] != src { + return err!( + inst, + "inconsistent with global location {}", + self.func.locations[arg].display(&self.reginfo) + ); + } + + divert.apply(&self.func.dfg[inst]); + + Ok(()) + } + + /// We have active diversions before a branch. Make sure none of the diverted values are live + /// on the outgoing CFG edges. + fn check_cfg_edges(&self, inst: ir::Inst, divert: &RegDiversions) -> Result { + use ir::instructions::BranchInfo::*; + + // We can only check CFG edges if we have a liveness analysis. + let liveness = match self.liveness { + Some(l) => l, + None => return Ok(()), + }; + let dfg = &self.func.dfg; + + match dfg[inst].analyze_branch(&dfg.value_lists) { + NotABranch => { + panic!( + "No branch information for {}", + dfg.display_inst(inst, self.isa) + ) + } + SingleDest(ebb, _) => { + for d in divert.all() { + let lr = &liveness[d.value]; + if lr.is_livein(ebb, &self.func.layout) { + return err!( + inst, + "{} is diverted to {} and live in to {}", + d.value, + d.to.display(&self.reginfo), + ebb + ); + } + } + } + Table(jt) => { + for d in divert.all() { + let lr = &liveness[d.value]; + for (_, ebb) in self.func.jump_tables[jt].entries() { + if lr.is_livein(ebb, &self.func.layout) { + return err!( + inst, + "{} is diverted to {} and live in to {}", + d.value, + d.to.display(&self.reginfo), + ebb + ); + } + } + } + } + } + + Ok(()) + } +} diff --git a/lib/cretonne/src/verifier/mod.rs b/lib/cretonne/src/verifier/mod.rs index 735f78a754..196054bec3 100644 --- a/lib/cretonne/src/verifier/mod.rs +++ b/lib/cretonne/src/verifier/mod.rs @@ -72,8 +72,9 @@ use std::collections::BTreeSet; use std::cmp::Ordering; use iterators::IteratorExtras; -pub use self::liveness::verify_liveness; pub use self::cssa::verify_cssa; +pub use self::liveness::verify_liveness; +pub use self::locations::verify_locations; // Create an `Err` variant of `Result` from a location and `format!` arguments. macro_rules! err { @@ -94,6 +95,7 @@ macro_rules! err { mod cssa; mod liveness; +mod locations; /// A verifier error. #[derive(Debug, PartialEq, Eq)]