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<ValueDef> for ProgramPoint impl

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<A, B>(&self, a: A, b: B, layout: &Layout) -> Ordering
     where
-        A: Into<ExpandedProgramPoint>,
+        A: Into<ProgramPoint>,
-        B: Into<ExpandedProgramPoint>,
+        B: Into<ProgramPoint>,
     {
         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<A, B>(&self, a: A, b: B, layout: &Layout) -> bool
     where
-        A: Into<ExpandedProgramPoint>,
+        A: Into<ProgramPoint>,
-        B: Into<ExpandedProgramPoint>,
+        B: Into<ProgramPoint>,
     {
         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<B>(&self, a: Block, b: B, layout: &Layout) -> Option<Inst>
     where
-        B: Into<ExpandedProgramPoint>,
+        B: Into<ProgramPoint>,
     {
         let (mut block_b, mut inst_b) = match b.into() {
-            ExpandedProgramPoint::Block(block) => (block, None),
+            ProgramPoint::Block(block) => (block, None),
-            ExpandedProgramPoint::Inst(inst) => (
+            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<A, B>(&self, a: A, b: B, layout: &Layout) -> Ordering
     where
-        A: Into<ExpandedProgramPoint>,
+        A: Into<ProgramPoint>,
-        B: Into<ExpandedProgramPoint>,
+        B: Into<ProgramPoint>,
     {
         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))
+            .then(layout.pp_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()))
     }
 }
 

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

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 {
+impl Layout {
-    fn cmp<A, B>(&self, a: A, b: B) -> cmp::Ordering
+    /// 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<A, B>(&self, a: A, b: B) -> cmp::Ordering
     where
-        A: Into<ExpandedProgramPoint>,
+        A: Into<ProgramPoint>,
-        B: Into<ExpandedProgramPoint>,
+        B: Into<ProgramPoint>,
     {
         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<PP: Into<ExpandedProgramPoint>>(&self, pp: PP) -> SequenceNumber {
+    fn seq<PP: Into<ProgramPoint>>(&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,
+            ProgramPoint::Block(block) => self.blocks[block].seq,
-            ExpandedProgramPoint::Inst(inst) => self.insts[inst].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<PP>(&self, pp: PP) -> Block
+    pub fn pp_block(&self, pp: ProgramPoint) -> Block {
-    where
+        match pp {
-        PP: Into<ExpandedProgramPoint>,
+            ProgramPoint::Block(block) => block,
-    {
+            ProgramPoint::Inst(inst) => self.inst_block(inst).expect("Program point not in layout"),
-        match pp.into() {
-            ExpandedProgramPoint::Block(block) => block,
-            ExpandedProgramPoint::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.pp_cmp(e2, e2), Ordering::Equal);
-        assert_eq!(layout.cmp(e2, i2), Ordering::Less);
+        assert_eq!(layout.pp_cmp(e2, i2), Ordering::Less);
-        assert_eq!(layout.cmp(i3, i2), Ordering::Greater);
+        assert_eq!(layout.pp_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);
     }
 }

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::{

cranelift/codegen/src/ir/progpoint.rs

@@ -1,10 +1,7 @@
 //! Program points.
 
-use crate::entity::EntityRef;
+use crate::ir::{Block, Inst};
-use crate::ir::{Block, Inst, ValueDef};
-use core::cmp;
 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);
+pub enum ProgramPoint {
-
-impl From<Inst> 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<Block> 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<ValueDef> 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 {
     /// 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<Inst> for ExpandedProgramPoint {
+impl From<Inst> for ProgramPoint {
     fn from(inst: Inst) -> Self {
         Self::Inst(inst)
     }
 }
 
-impl From<Block> for ExpandedProgramPoint {
+impl From<Block> for ProgramPoint {
     fn from(block: Block) -> Self {
         Self::Block(block)
     }
 }
 
-impl From<ValueDef> for ExpandedProgramPoint {
+impl fmt::Display 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"),
-        }
-    }
-}
-
-impl From<ProgramPoint> 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 {
     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<A, B>(&self, a: A, b: B) -> cmp::Ordering
-    where
-        A: Into<ExpandedProgramPoint>,
-        B: Into<ExpandedProgramPoint>;
-
-    /// 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::*;

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();
 

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
+            if self.expected_domtree.rpo_cmp_block(prev_block, next_block) != Ordering::Greater {
-                .expected_domtree
-                .rpo_cmp(prev_block, next_block, &self.func.layout)
-                != 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
                     ),
                 ));