From a81c2068705fb565cdb469296f8201ce47dddd42 Mon Sep 17 00:00:00 2001 From: bjorn3 <17426603+bjorn3@users.noreply.github.com> Date: Fri, 17 Mar 2023 19:46:34 +0100 Subject: [PATCH] Various cleanups to Layout (#6042) * Use inst_block instead of pp_block where possible * Remove unused is_block_gap method * Remove ProgramOrder trait It only has a single implementation * Rename Layout::cmp to pp_cmp to distinguish it from Ord::cmp * Make pp_block non-generic * Use rpo_cmp_block instead of rpo_cmp in the verifier * Remove ProgramPoint * Rename ExpandedProgramPoint to ProgramPoint * Remove From for ProgramPoint impl --- cranelift/codegen/src/dominator_tree.rs | 55 ++++++------- cranelift/codegen/src/ir/dfg.rs | 5 -- cranelift/codegen/src/ir/layout.rs | 56 ++++++------- cranelift/codegen/src/ir/mod.rs | 2 +- cranelift/codegen/src/ir/progpoint.rs | 102 ++---------------------- cranelift/codegen/src/legalizer/mod.rs | 5 +- cranelift/codegen/src/verifier/mod.rs | 16 ++-- 7 files changed, 65 insertions(+), 176 deletions(-) diff --git a/cranelift/codegen/src/dominator_tree.rs b/cranelift/codegen/src/dominator_tree.rs index a4985261ae..135697497d 100644 --- a/cranelift/codegen/src/dominator_tree.rs +++ b/cranelift/codegen/src/dominator_tree.rs @@ -2,7 +2,7 @@ use crate::entity::SecondaryMap; use crate::flowgraph::{BlockPredecessor, ControlFlowGraph}; -use crate::ir::{Block, ExpandedProgramPoint, Function, Inst, Layout, ProgramOrder, Value}; +use crate::ir::{Block, Function, Inst, Layout, ProgramPoint}; use crate::packed_option::PackedOption; use crate::timing; use alloc::vec::Vec; @@ -88,7 +88,7 @@ impl DominatorTree { } /// Compare two blocks relative to the reverse post-order. - fn rpo_cmp_block(&self, a: Block, b: Block) -> Ordering { + pub fn rpo_cmp_block(&self, a: Block, b: Block) -> Ordering { self.nodes[a].rpo_number.cmp(&self.nodes[b].rpo_number) } @@ -100,13 +100,13 @@ impl DominatorTree { /// If `a` and `b` belong to the same block, compare their relative position in the block. pub fn rpo_cmp(&self, a: A, b: B, layout: &Layout) -> Ordering where - A: Into, - B: Into, + A: Into, + B: Into, { let a = a.into(); let b = b.into(); self.rpo_cmp_block(layout.pp_block(a), layout.pp_block(b)) - .then(layout.cmp(a, b)) + .then(layout.pp_cmp(a, b)) } /// Returns `true` if `a` dominates `b`. @@ -120,21 +120,21 @@ impl DominatorTree { /// An instruction is considered to dominate itself. pub fn dominates(&self, a: A, b: B, layout: &Layout) -> bool where - A: Into, - B: Into, + A: Into, + B: Into, { let a = a.into(); let b = b.into(); match a { - ExpandedProgramPoint::Block(block_a) => { + ProgramPoint::Block(block_a) => { a == b || self.last_dominator(block_a, b, layout).is_some() } - ExpandedProgramPoint::Inst(inst_a) => { + ProgramPoint::Inst(inst_a) => { let block_a = layout .inst_block(inst_a) .expect("Instruction not in layout."); match self.last_dominator(block_a, b, layout) { - Some(last) => layout.cmp(inst_a, last) != Ordering::Greater, + Some(last) => layout.pp_cmp(inst_a, last) != Ordering::Greater, None => false, } } @@ -145,11 +145,11 @@ impl DominatorTree { /// If no instructions in `a` dominate `b`, return `None`. pub fn last_dominator(&self, a: Block, b: B, layout: &Layout) -> Option where - B: Into, + B: Into, { let (mut block_b, mut inst_b) = match b.into() { - ExpandedProgramPoint::Block(block) => (block, None), - ExpandedProgramPoint::Inst(inst) => ( + ProgramPoint::Block(block) => (block, None), + ProgramPoint::Inst(inst) => ( layout.inst_block(inst).expect("Instruction not in layout."), Some(inst), ), @@ -210,7 +210,7 @@ impl DominatorTree { ); // We're in the same block. The common dominator is the earlier instruction. - if layout.cmp(a.inst, b.inst) == Ordering::Less { + if layout.pp_cmp(a.inst, b.inst) == Ordering::Less { a } else { b @@ -475,7 +475,9 @@ impl DominatorTreePreorder { // sibling lists are ordered according to the CFG reverse post-order. for &block in domtree.cfg_postorder() { if let Some(idom_inst) = domtree.idom(block) { - let idom = layout.pp_block(idom_inst); + let idom = layout + .inst_block(idom_inst) + .expect("Instruction not in layout."); let sib = mem::replace(&mut self.nodes[idom].child, block.into()); self.nodes[block].sibling = sib; } else { @@ -505,7 +507,9 @@ impl DominatorTreePreorder { // its dominator tree children. for &block in domtree.cfg_postorder() { if let Some(idom_inst) = domtree.idom(block) { - let idom = layout.pp_block(idom_inst); + let idom = layout + .inst_block(idom_inst) + .expect("Instruction not in layout."); let pre_max = cmp::max(self.nodes[block].pre_max, self.nodes[idom].pre_max); self.nodes[idom].pre_max = pre_max; } @@ -568,26 +572,13 @@ impl DominatorTreePreorder { /// program points dominated by pp follow immediately and contiguously after pp in the order. pub fn pre_cmp(&self, a: A, b: B, layout: &Layout) -> Ordering where - A: Into, - B: Into, + A: Into, + B: Into, { let a = a.into(); let b = b.into(); self.pre_cmp_block(layout.pp_block(a), layout.pp_block(b)) - .then(layout.cmp(a, b)) - } - - /// Compare two value defs according to the dominator tree pre-order. - /// - /// Two values defined at the same program point are compared according to their parameter or - /// result order. - /// - /// This is a total ordering of the values in the function. - pub fn pre_cmp_def(&self, a: Value, b: Value, func: &Function) -> Ordering { - let da = func.dfg.value_def(a); - let db = func.dfg.value_def(b); - self.pre_cmp(da, db, &func.layout) - .then_with(|| da.num().cmp(&db.num())) + .then(layout.pp_cmp(a, b)) } } diff --git a/cranelift/codegen/src/ir/dfg.rs b/cranelift/codegen/src/ir/dfg.rs index 1718921617..5784812cff 100644 --- a/cranelift/codegen/src/ir/dfg.rs +++ b/cranelift/codegen/src/ir/dfg.rs @@ -492,11 +492,6 @@ impl ValueDef { } } - /// Get the program point where the value was defined. - pub fn pp(self) -> ir::ExpandedProgramPoint { - self.into() - } - /// Get the number component of this definition. /// /// When multiple values are defined at the same program point, this indicates the index of diff --git a/cranelift/codegen/src/ir/layout.rs b/cranelift/codegen/src/ir/layout.rs index 644665bab5..f8bbc12fe2 100644 --- a/cranelift/codegen/src/ir/layout.rs +++ b/cranelift/codegen/src/ir/layout.rs @@ -4,7 +4,7 @@ //! determined by the `Layout` data structure defined in this module. use crate::entity::SecondaryMap; -use crate::ir::progpoint::{ExpandedProgramPoint, ProgramOrder}; +use crate::ir::progpoint::ProgramPoint; use crate::ir::{Block, Inst}; use crate::packed_option::PackedOption; use crate::{timing, trace}; @@ -114,33 +114,33 @@ fn test_midpoint() { assert_eq!(midpoint(3, 4), None); } -impl ProgramOrder for Layout { - fn cmp(&self, a: A, b: B) -> cmp::Ordering +impl Layout { + /// Compare the program points `a` and `b` relative to this program order. + /// + /// Return `Less` if `a` appears in the program before `b`. + /// + /// This is declared as a generic such that it can be called with `Inst` and `Block` arguments + /// directly. Depending on the implementation, there is a good chance performance will be + /// improved for those cases where the type of either argument is known statically. + pub fn pp_cmp(&self, a: A, b: B) -> cmp::Ordering where - A: Into, - B: Into, + A: Into, + B: Into, { let a_seq = self.seq(a); let b_seq = self.seq(b); a_seq.cmp(&b_seq) } - - fn is_block_gap(&self, inst: Inst, block: Block) -> bool { - let i = &self.insts[inst]; - let e = &self.blocks[block]; - - i.next.is_none() && i.block == e.prev - } } // Private methods for dealing with sequence numbers. impl Layout { /// Get the sequence number of a program point that must correspond to an entity in the layout. - fn seq>(&self, pp: PP) -> SequenceNumber { + fn seq>(&self, pp: PP) -> SequenceNumber { // When `PP = Inst` or `PP = Block`, we expect this dynamic type check to be optimized out. match pp.into() { - ExpandedProgramPoint::Block(block) => self.blocks[block].seq, - ExpandedProgramPoint::Inst(inst) => self.insts[inst].seq, + ProgramPoint::Block(block) => self.blocks[block].seq, + ProgramPoint::Inst(inst) => self.insts[inst].seq, } } @@ -536,15 +536,10 @@ impl Layout { } /// Get the block containing the program point `pp`. Panic if `pp` is not in the layout. - pub fn pp_block(&self, pp: PP) -> Block - where - PP: Into, - { - match pp.into() { - ExpandedProgramPoint::Block(block) => block, - ExpandedProgramPoint::Inst(inst) => { - self.inst_block(inst).expect("Program point not in layout") - } + pub fn pp_block(&self, pp: ProgramPoint) -> Block { + match pp { + ProgramPoint::Block(block) => block, + ProgramPoint::Inst(inst) => self.inst_block(inst).expect("Program point not in layout"), } } @@ -867,7 +862,7 @@ mod tests { use super::Layout; use crate::cursor::{Cursor, CursorPosition}; use crate::entity::EntityRef; - use crate::ir::{Block, Inst, ProgramOrder, SourceLoc}; + use crate::ir::{Block, Inst, SourceLoc}; use alloc::vec::Vec; use core::cmp::Ordering; @@ -1289,13 +1284,8 @@ mod tests { } // Check `ProgramOrder`. - assert_eq!(layout.cmp(e2, e2), Ordering::Equal); - assert_eq!(layout.cmp(e2, i2), Ordering::Less); - assert_eq!(layout.cmp(i3, i2), Ordering::Greater); - - assert_eq!(layout.is_block_gap(i1, e2), true); - assert_eq!(layout.is_block_gap(i3, e1), true); - assert_eq!(layout.is_block_gap(i1, e1), false); - assert_eq!(layout.is_block_gap(i2, e1), false); + assert_eq!(layout.pp_cmp(e2, e2), Ordering::Equal); + assert_eq!(layout.pp_cmp(e2, i2), Ordering::Less); + assert_eq!(layout.pp_cmp(i3, i2), Ordering::Greater) } } diff --git a/cranelift/codegen/src/ir/mod.rs b/cranelift/codegen/src/ir/mod.rs index 7b000c8e72..3a5936fcb9 100644 --- a/cranelift/codegen/src/ir/mod.rs +++ b/cranelift/codegen/src/ir/mod.rs @@ -53,7 +53,7 @@ pub use crate::ir::known_symbol::KnownSymbol; pub use crate::ir::layout::Layout; pub use crate::ir::libcall::{get_probestack_funcref, LibCall}; pub use crate::ir::memflags::{Endianness, MemFlags}; -pub use crate::ir::progpoint::{ExpandedProgramPoint, ProgramOrder, ProgramPoint}; +pub use crate::ir::progpoint::ProgramPoint; pub use crate::ir::sourceloc::RelSourceLoc; pub use crate::ir::sourceloc::SourceLoc; pub use crate::ir::stackslot::{ diff --git a/cranelift/codegen/src/ir/progpoint.rs b/cranelift/codegen/src/ir/progpoint.rs index 39c4d98fbe..7800547767 100644 --- a/cranelift/codegen/src/ir/progpoint.rs +++ b/cranelift/codegen/src/ir/progpoint.rs @@ -1,10 +1,7 @@ //! Program points. -use crate::entity::EntityRef; -use crate::ir::{Block, Inst, ValueDef}; -use core::cmp; +use crate::ir::{Block, Inst}; use core::fmt; -use core::u32; /// A `ProgramPoint` represents a position in a function where the live range of an SSA value can /// begin or end. It can be either: @@ -14,45 +11,14 @@ use core::u32; /// /// This corresponds more or less to the lines in the textual form of Cranelift IR. #[derive(PartialEq, Eq, Clone, Copy)] -pub struct ProgramPoint(u32); - -impl From for ProgramPoint { - fn from(inst: Inst) -> Self { - let idx = inst.index(); - debug_assert!(idx < (u32::MAX / 2) as usize); - Self((idx * 2) as u32) - } -} - -impl From for ProgramPoint { - fn from(block: Block) -> Self { - let idx = block.index(); - debug_assert!(idx < (u32::MAX / 2) as usize); - Self((idx * 2 + 1) as u32) - } -} - -impl From for ProgramPoint { - fn from(def: ValueDef) -> Self { - match def { - ValueDef::Result(inst, _) => inst.into(), - ValueDef::Param(block, _) => block.into(), - ValueDef::Union(_, _) => panic!("Union does not have a single program point"), - } - } -} - -/// An expanded program point directly exposes the variants, but takes twice the space to -/// represent. -#[derive(PartialEq, Eq, Clone, Copy)] -pub enum ExpandedProgramPoint { +pub enum ProgramPoint { /// An instruction in the function. Inst(Inst), /// A block header. Block(Block), } -impl ExpandedProgramPoint { +impl ProgramPoint { /// Get the instruction we know is inside. pub fn unwrap_inst(self) -> Inst { match self { @@ -62,39 +28,19 @@ impl ExpandedProgramPoint { } } -impl From for ExpandedProgramPoint { +impl From for ProgramPoint { fn from(inst: Inst) -> Self { Self::Inst(inst) } } -impl From for ExpandedProgramPoint { +impl From for ProgramPoint { fn from(block: Block) -> Self { Self::Block(block) } } -impl From for ExpandedProgramPoint { - fn from(def: ValueDef) -> Self { - match def { - ValueDef::Result(inst, _) => inst.into(), - ValueDef::Param(block, _) => block.into(), - ValueDef::Union(_, _) => panic!("Union does not have a single program point"), - } - } -} - -impl From for ExpandedProgramPoint { - fn from(pp: ProgramPoint) -> Self { - if pp.0 & 1 == 0 { - Self::Inst(Inst::from_u32(pp.0 / 2)) - } else { - Self::Block(Block::from_u32(pp.0 / 2)) - } - } -} - -impl fmt::Display for ExpandedProgramPoint { +impl fmt::Display for ProgramPoint { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { Self::Inst(x) => write!(f, "{}", x), @@ -103,48 +49,12 @@ impl fmt::Display for ExpandedProgramPoint { } } -impl fmt::Display for ProgramPoint { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - let epp: ExpandedProgramPoint = (*self).into(); - epp.fmt(f) - } -} - -impl fmt::Debug for ExpandedProgramPoint { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "ExpandedProgramPoint({})", self) - } -} - impl fmt::Debug for ProgramPoint { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { write!(f, "ProgramPoint({})", self) } } -/// Context for ordering program points. -/// -/// `ProgramPoint` objects don't carry enough information to be ordered independently, they need a -/// context providing the program order. -pub trait ProgramOrder { - /// Compare the program points `a` and `b` relative to this program order. - /// - /// Return `Less` if `a` appears in the program before `b`. - /// - /// This is declared as a generic such that it can be called with `Inst` and `Block` arguments - /// directly. Depending on the implementation, there is a good chance performance will be - /// improved for those cases where the type of either argument is known statically. - fn cmp(&self, a: A, b: B) -> cmp::Ordering - where - A: Into, - B: Into; - - /// Is the range from `inst` to `block` just the gap between consecutive blocks? - /// - /// This returns true if `inst` is the terminator in the block immediately before `block`. - fn is_block_gap(&self, inst: Inst, block: Block) -> bool; -} - #[cfg(test)] mod tests { use super::*; diff --git a/cranelift/codegen/src/legalizer/mod.rs b/cranelift/codegen/src/legalizer/mod.rs index c226f0cb5d..3b09ed1fc7 100644 --- a/cranelift/codegen/src/legalizer/mod.rs +++ b/cranelift/codegen/src/legalizer/mod.rs @@ -299,7 +299,10 @@ fn expand_cond_trap( // // new_block_resume: // .. - let old_block = func.layout.pp_block(inst); + let old_block = func + .layout + .inst_block(inst) + .expect("Instruction not in layout."); let new_block_trap = func.dfg.make_block(); let new_block_resume = func.dfg.make_block(); diff --git a/cranelift/codegen/src/verifier/mod.rs b/cranelift/codegen/src/verifier/mod.rs index 016abdbf70..4f1e8a3dfd 100644 --- a/cranelift/codegen/src/verifier/mod.rs +++ b/cranelift/codegen/src/verifier/mod.rs @@ -884,7 +884,11 @@ impl<'a> Verifier<'a> { self.verify_value(loc_inst, v, errors)?; let dfg = &self.func.dfg; - let loc_block = self.func.layout.pp_block(loc_inst); + let loc_block = self + .func + .layout + .inst_block(loc_inst) + .expect("Instruction not in layout."); let is_reachable = self.expected_domtree.is_reachable(loc_block); // SSA form @@ -1101,17 +1105,13 @@ impl<'a> Verifier<'a> { )); } } - // We verify rpo_cmp on pairs of adjacent blocks in the postorder + // We verify rpo_cmp_block on pairs of adjacent blocks in the postorder for (&prev_block, &next_block) in domtree.cfg_postorder().iter().adjacent_pairs() { - if self - .expected_domtree - .rpo_cmp(prev_block, next_block, &self.func.layout) - != Ordering::Greater - { + if self.expected_domtree.rpo_cmp_block(prev_block, next_block) != Ordering::Greater { return errors.fatal(( next_block, format!( - "invalid domtree, rpo_cmp does not says {} is greater than {}", + "invalid domtree, rpo_cmp_block does not says {} is greater than {}", prev_block, next_block ), ));