diff --git a/src/nouveau/compiler/nak/assign_regs.rs b/src/nouveau/compiler/nak/assign_regs.rs index f84160a5e5e..05621c128f3 100644 --- a/src/nouveau/compiler/nak/assign_regs.rs +++ b/src/nouveau/compiler/nak/assign_regs.rs @@ -181,7 +181,7 @@ impl PhiWebs { let Some(phi_dsts) = f.blocks[b_idx].phi_dsts() else { continue; }; - let dsts: FxHashMap = phi_dsts + let dsts: FxHashMap = phi_dsts .dsts .iter() .map(|(idx, dst)| { @@ -225,7 +225,7 @@ impl PhiWebs { #[derive(Clone, Copy, Eq, Hash, PartialEq)] enum LiveRef { SSA(SSAValue), - Phi(u32), + Phi(Phi), } #[derive(Clone, Copy, Eq, Hash, PartialEq)] @@ -922,7 +922,7 @@ struct AssignRegsBlock { ra: PerRegFile, pcopy_tmp_gprs: u8, live_in: Vec, - phi_out: FxHashMap, + phi_out: FxHashMap, } impl AssignRegsBlock { @@ -1031,12 +1031,12 @@ impl AssignRegsBlock { Op::PhiDsts(op) => { assert!(instr.pred.is_true()); - for (id, dst) in op.dsts.iter() { + for (phi, dst) in op.dsts.iter() { if let Dst::SSA(ssa) = dst { assert!(ssa.comps() == 1); let reg = self.alloc_scalar(ip, sum, phi_webs, ssa[0]); self.live_in.push(LiveValue { - live_ref: LiveRef::Phi(*id), + live_ref: LiveRef::Phi(*phi), reg_ref: reg, }); } diff --git a/src/nouveau/compiler/nak/from_nir.rs b/src/nouveau/compiler/nak/from_nir.rs index 859f0f71128..2fe211e95d4 100644 --- a/src/nouveau/compiler/nak/from_nir.rs +++ b/src/nouveau/compiler/nak/from_nir.rs @@ -192,7 +192,7 @@ fn alloc_ssa_for_nir(b: &mut impl SSABuilder, ssa: &nir_def) -> Vec { struct PhiAllocMap<'a> { alloc: &'a mut PhiAllocator, - map: FxHashMap<(u32, u8), u32>, + map: FxHashMap<(u32, u8), Phi>, } impl<'a> PhiAllocMap<'a> { @@ -203,10 +203,10 @@ impl<'a> PhiAllocMap<'a> { } } - fn get_phi_id(&mut self, phi: &nir_phi_instr, comp: u8) -> u32 { + fn get_phi(&mut self, nphi: &nir_phi_instr, comp: u8) -> Phi { *self .map - .entry((phi.def.index, comp)) + .entry((nphi.def.index, comp)) .or_insert_with(|| self.alloc.alloc()) } } @@ -3638,7 +3638,7 @@ impl<'a> ShaderFromNir<'a> { let mut b = UniformBuilder::new(&mut b, uniform); let dst = alloc_ssa_for_nir(&mut b, np.def.as_def()); for i in 0..dst.len() { - let phi_id = phi_map.get_phi_id(np, i.try_into().unwrap()); + let phi_id = phi_map.get_phi(np, i.try_into().unwrap()); phi.dsts.push(phi_id, dst[i].into()); } self.set_ssa(np.def.as_def(), dst); @@ -3757,7 +3757,7 @@ impl<'a> ShaderFromNir<'a> { let src = src.as_ssa().unwrap(); for (i, src) in src.iter().enumerate() { let phi_id = - phi_map.get_phi_id(np, i.try_into().unwrap()); + phi_map.get_phi(np, i.try_into().unwrap()); phi.srcs.push(phi_id, (*src).into()); } break; diff --git a/src/nouveau/compiler/nak/ir.rs b/src/nouveau/compiler/nak/ir.rs index 34a14adb474..d335cd2c3d6 100644 --- a/src/nouveau/compiler/nak/ir.rs +++ b/src/nouveau/compiler/nak/ir.rs @@ -6130,26 +6130,88 @@ impl VecPair { } } -pub struct PhiAllocator { - count: u32, -} +mod phi { + #[allow(unused_imports)] + use crate::ir::{OpPhiDsts, OpPhiSrcs}; + use compiler::bitset::IntoBitIndex; + use std::fmt; -impl PhiAllocator { - pub fn new() -> PhiAllocator { - PhiAllocator { count: 0 } + /// A phi node + /// + /// Phis in NAK are implemented differently from NIR and similar IRs. + /// Instead of having a single phi instruction which lives in the successor + /// block, each `Phi` represents a single merged 32-bit (or 1-bit for + /// predicates) value and we have separate [`OpPhiSrcs`] and [`OpPhiDsts`] + /// instructions which map phis to sources and destinations. + /// + /// One of the problems fundamental to phis is that they really live on the + /// edges between blocks. Regardless of where the phi instruction lives in + /// the IR data structures, its sources are consumed at the end of the + /// predecessor block and its destinations are defined at the start of the + /// successor block and all phi sources and destinations get consumed and go + /// live simultaneously for any given CFG edge. For a phi that participates + /// in a back-edge, this means that the source of the phi may be consumed + /// after (in block order) the destination goes live. + /// + /// In NIR, this has caused no end of headaches. Most passes which need to + /// process phis ignore phis when first processing a block and then have a + /// special case at the end of each block which walks the successors and + /// processes the successor's phis, looking only at the phi sources whose + /// predecessor matches the block. This is clunky and often forgotten by + /// optimization and lowering pass authors. It's also easy to get missed by + /// testing since it only really breaks if you have a phi which participates + /// in a back-edge so it often gets found later when something breaks in the + /// wild. + /// + /// To work around this (and also make things a little more Rust-friendly), + /// NAK places the instruction which consumes phi sources at the end of the + /// predecessor block and the instruction which defines phi destinations at + /// the start of the successor block. This structurally eliminates the + /// problem that has plagued NIR for years. The cost to this solution is + /// that we have to create maps from phis to/from SSA values whenever we + /// want to optimize the phis themselves. However, this affects few enough + /// passes that the benefits to the rest of the IR are worth the trade-off, + /// at least for a back-end compiler. + #[derive(Clone, Copy, Eq, Hash, PartialEq)] + pub struct Phi { + idx: u32, } - pub fn alloc(&mut self) -> u32 { - let idx = self.count; - self.count = idx + 1; - idx + impl IntoBitIndex for Phi { + fn into_bit_index(self) -> usize { + self.idx.try_into().unwrap() + } + } + + impl fmt::Display for Phi { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "φ{}", self.idx) + } + } + + pub struct PhiAllocator { + count: u32, + } + + impl PhiAllocator { + pub fn new() -> PhiAllocator { + PhiAllocator { count: 0 } + } + + pub fn alloc(&mut self) -> Phi { + let idx = self.count; + self.count = idx + 1; + Phi { idx } + } } } +pub use phi::{Phi, PhiAllocator}; +/// An instruction which maps [Phi]s to sources in the predecessor block #[repr(C)] #[derive(DstsAsSlice)] pub struct OpPhiSrcs { - pub srcs: VecPair, + pub srcs: VecPair, } impl OpPhiSrcs { @@ -6183,21 +6245,22 @@ impl DisplayOp for OpPhiSrcs { fn fmt_op(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "phi_src ")?; - for (i, (id, src)) in self.srcs.iter().enumerate() { + for (i, (phi, src)) in self.srcs.iter().enumerate() { if i > 0 { write!(f, ", ")?; } - write!(f, "φ{} = {}", id, src)?; + write!(f, "{phi} = {src}")?; } Ok(()) } } impl_display_for_op!(OpPhiSrcs); +/// An instruction which maps [Phi]s to destinations in the succeessor block #[repr(C)] #[derive(SrcsAsSlice)] pub struct OpPhiDsts { - pub dsts: VecPair, + pub dsts: VecPair, } impl OpPhiDsts { @@ -6231,11 +6294,11 @@ impl DisplayOp for OpPhiDsts { fn fmt_op(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "phi_dst ")?; - for (i, (id, dst)) in self.dsts.iter().enumerate() { + for (i, (phi, dst)) in self.dsts.iter().enumerate() { if i > 0 { write!(f, ", ")?; } - write!(f, "{} = φ{}", dst, id)?; + write!(f, "{dst} = {phi}")?; } Ok(()) } diff --git a/src/nouveau/compiler/nak/opt_bar_prop.rs b/src/nouveau/compiler/nak/opt_bar_prop.rs index c97ab0884fe..5ef0f5c3c05 100644 --- a/src/nouveau/compiler/nak/opt_bar_prop.rs +++ b/src/nouveau/compiler/nak/opt_bar_prop.rs @@ -8,8 +8,8 @@ use compiler::bitset::BitSet; use rustc_hash::FxHashMap; struct PhiMap { - phi_ssa: FxHashMap>, - ssa_phi: FxHashMap, + phi_ssa: FxHashMap>, + ssa_phi: FxHashMap, } impl PhiMap { @@ -21,29 +21,29 @@ impl PhiMap { } fn add_phi_srcs(&mut self, op: &OpPhiSrcs) { - for (idx, src) in op.srcs.iter() { + for (phi, src) in op.srcs.iter() { if let SrcRef::SSA(ssa) = &src.src_ref { assert!(ssa.comps() == 1); - let phi_srcs = self.phi_ssa.entry(*idx).or_default(); + let phi_srcs = self.phi_ssa.entry(*phi).or_default(); phi_srcs.push(ssa[0]); } } } fn add_phi_dsts(&mut self, op: &OpPhiDsts) { - for (idx, dst) in op.dsts.iter() { + for (phi, dst) in op.dsts.iter() { if let Dst::SSA(ssa) = dst { assert!(ssa.comps() == 1); - self.ssa_phi.insert(ssa[0], *idx); + self.ssa_phi.insert(ssa[0], *phi); } } } - fn get_phi(&self, ssa: &SSAValue) -> Option<&u32> { + fn get_phi(&self, ssa: &SSAValue) -> Option<&Phi> { self.ssa_phi.get(ssa) } - fn phi_srcs(&self, idx: &u32) -> &[SSAValue] { + fn phi_srcs(&self, idx: &Phi) -> &[SSAValue] { self.phi_ssa.get(idx).unwrap() } } @@ -51,8 +51,8 @@ impl PhiMap { #[derive(Default)] struct BarPropPass { ssa_map: FxHashMap, - phi_is_bar: BitSet, - phi_is_not_bar: BitSet, + phi_is_bar: BitSet, + phi_is_not_bar: BitSet, } impl BarPropPass { @@ -89,19 +89,19 @@ impl BarPropPass { fn phi_can_be_bar_recur( &mut self, phi_map: &PhiMap, - seen: &mut BitSet, - phi: u32, + seen: &mut BitSet, + phi: Phi, ) -> bool { - if self.phi_is_not_bar.contains(phi.try_into().unwrap()) { + if self.phi_is_not_bar.contains(phi) { return false; } - if seen.contains(phi.try_into().unwrap()) { + if seen.contains(phi) { // If we've hit a cycle, that's not a fail return true; } - seen.insert(phi.try_into().unwrap()); + seen.insert(phi); for src_ssa in phi_map.phi_srcs(&phi) { if self.is_bar(src_ssa) { @@ -114,7 +114,7 @@ impl BarPropPass { } } - self.phi_is_not_bar.insert(phi.try_into().unwrap()); + self.phi_is_not_bar.insert(phi); return false; } @@ -125,18 +125,18 @@ impl BarPropPass { &mut self, ssa_alloc: &mut SSAValueAllocator, phi_map: &PhiMap, - needs_bar: &mut BitSet, - phi: u32, + needs_bar: &mut BitSet, + phi: Phi, ssa: SSAValue, ) { - if !needs_bar.contains(phi.try_into().unwrap()) { + if !needs_bar.contains(phi) { return; } let bar = ssa_alloc.alloc(RegFile::Bar); self.ssa_map.insert(ssa, bar); - self.phi_is_bar.insert(phi.try_into().unwrap()); - needs_bar.remove(phi.try_into().unwrap()); + self.phi_is_bar.insert(phi); + needs_bar.remove(phi); for src_ssa in phi_map.phi_srcs(&phi) { if let Some(src_phi) = phi_map.get_phi(src_ssa) { @@ -151,10 +151,10 @@ impl BarPropPass { &mut self, ssa_alloc: &mut SSAValueAllocator, phi_map: &PhiMap, - phi: u32, + phi: Phi, ssa: SSAValue, ) { - if self.phi_is_bar.contains(phi.try_into().unwrap()) { + if self.phi_is_bar.contains(phi) { return; } @@ -203,8 +203,7 @@ impl BarPropPass { match &mut instr.op { Op::PhiSrcs(op) => { for (idx, src) in op.srcs.iter_mut() { - if self.phi_is_bar.contains((*idx).try_into().unwrap()) - { + if self.phi_is_bar.contains(*idx) { // Barrier immediates don't exist let ssa = src.as_ssa().unwrap(); let bar = *self.map_bar(&ssa[0]).unwrap(); @@ -216,8 +215,7 @@ impl BarPropPass { Op::PhiDsts(op) => { let mut bmovs = Vec::new(); for (idx, dst) in op.dsts.iter_mut() { - if self.phi_is_bar.contains((*idx).try_into().unwrap()) - { + if self.phi_is_bar.contains(*idx) { let ssa = dst.as_ssa().unwrap().clone(); let bar = *self.ssa_map.get(&ssa[0]).unwrap(); *dst = bar.into(); diff --git a/src/nouveau/compiler/nak/opt_dce.rs b/src/nouveau/compiler/nak/opt_dce.rs index af63bec5dbd..77483d70f94 100644 --- a/src/nouveau/compiler/nak/opt_dce.rs +++ b/src/nouveau/compiler/nak/opt_dce.rs @@ -12,7 +12,7 @@ struct DeadCodePass { any_dead: bool, new_live: bool, live_ssa: HashSet, - live_phi: HashSet, + live_phi: HashSet, } impl DeadCodePass { @@ -35,8 +35,8 @@ impl DeadCodePass { } } - fn mark_phi_live(&mut self, id: u32) { - self.new_live |= self.live_phi.insert(id); + fn mark_phi_live(&mut self, phi: Phi) { + self.new_live |= self.live_phi.insert(phi); } fn is_dst_live(&self, dst: &Dst) -> bool { @@ -54,8 +54,8 @@ impl DeadCodePass { } } - fn is_phi_live(&self, id: u32) -> bool { - self.live_phi.contains(&id) + fn is_phi_live(&self, phi: Phi) -> bool { + self.live_phi.contains(&phi) } fn is_instr_live(&self, instr: &Instr) -> bool { @@ -80,8 +80,8 @@ impl DeadCodePass { match &instr.op { Op::PhiSrcs(phi) => { assert!(instr.pred.is_true()); - for (id, src) in phi.srcs.iter() { - if self.is_phi_live(*id) { + for (phi, src) in phi.srcs.iter() { + if self.is_phi_live(*phi) { self.mark_src_live(src); } else { self.any_dead = true; @@ -90,9 +90,9 @@ impl DeadCodePass { } Op::PhiDsts(phi) => { assert!(instr.pred.is_true()); - for (id, dst) in phi.dsts.iter() { + for (phi, dst) in phi.dsts.iter() { if self.is_dst_live(dst) { - self.mark_phi_live(*id); + self.mark_phi_live(*phi); } else { self.any_dead = true; } @@ -127,7 +127,7 @@ impl DeadCodePass { fn map_instr(&self, mut instr: Box) -> MappedInstrs { let is_live = match &mut instr.op { Op::PhiSrcs(phi) => { - phi.srcs.retain(|id, _| self.is_phi_live(*id)); + phi.srcs.retain(|phi, _| self.is_phi_live(*phi)); !phi.srcs.is_empty() } Op::PhiDsts(phi) => { diff --git a/src/nouveau/compiler/nak/repair_ssa.rs b/src/nouveau/compiler/nak/repair_ssa.rs index d5dffc59109..9dfe6c99ff7 100644 --- a/src/nouveau/compiler/nak/repair_ssa.rs +++ b/src/nouveau/compiler/nak/repair_ssa.rs @@ -11,7 +11,7 @@ use std::cmp::Reverse; use std::collections::BinaryHeap; struct PhiTracker { - idx: u32, + phi: Phi, orig: SSAValue, dst: SSAValue, srcs: FxHashMap, @@ -82,10 +82,10 @@ fn get_ssa_or_phi( let b_ssa = if all_same { pred_ssa.expect("Undefined value") } else { - let phi_idx = phi_alloc.alloc(); + let phi = phi_alloc.alloc(); let phi_ssa = ssa_alloc.alloc(ssa.file()); let mut pt = PhiTracker { - idx: phi_idx, + phi, orig: ssa, dst: phi_ssa, srcs: Default::default(), @@ -317,7 +317,7 @@ impl Function { if !b_phis.is_empty() { let phi_dst = get_or_insert_phi_dsts(bb); for pt in b_phis.iter() { - phi_dst.dsts.push(pt.idx, pt.dst.into()); + phi_dst.dsts.push(pt.phi, pt.dst.into()); } } @@ -337,7 +337,7 @@ impl Function { for pt in s_phis.iter() { let mut ssa = *pt.srcs.get(&b_idx).unwrap(); ssa = ssa_map.find(ssa); - phi_src.srcs.push(pt.idx, ssa.into()); + phi_src.srcs.push(pt.phi, ssa.into()); } } } diff --git a/src/nouveau/compiler/nak/spill_values.rs b/src/nouveau/compiler/nak/spill_values.rs index 96eaf8fc051..afa3fd1d151 100644 --- a/src/nouveau/compiler/nak/spill_values.rs +++ b/src/nouveau/compiler/nak/spill_values.rs @@ -15,7 +15,7 @@ use std::cmp::{max, Ordering, Reverse}; use std::collections::BinaryHeap; struct PhiDstMap { - ssa_phi: FxHashMap, + ssa_phi: FxHashMap, } impl PhiDstMap { @@ -25,10 +25,10 @@ impl PhiDstMap { } } - fn add_phi_dst(&mut self, phi_idx: u32, dst: &Dst) { + fn add_phi_dst(&mut self, phi: Phi, dst: &Dst) { let vec = dst.as_ssa().expect("Not an SSA destination"); debug_assert!(vec.comps() == 1); - self.ssa_phi.insert(vec[0], phi_idx); + self.ssa_phi.insert(vec[0], phi); } pub fn from_block(block: &BasicBlock) -> PhiDstMap { @@ -41,13 +41,13 @@ impl PhiDstMap { map } - fn get_phi_idx(&self, ssa: &SSAValue) -> Option<&u32> { + fn get_phi(&self, ssa: &SSAValue) -> Option<&Phi> { self.ssa_phi.get(ssa) } } struct PhiSrcMap { - phi_src: FxHashMap, + phi_src: FxHashMap, } impl PhiSrcMap { @@ -57,25 +57,25 @@ impl PhiSrcMap { } } - fn add_phi_src(&mut self, phi_idx: u32, src: &Src) { + fn add_phi_src(&mut self, phi: Phi, src: &Src) { debug_assert!(src.is_unmodified()); let vec = src.src_ref.as_ssa().expect("Not an SSA source"); debug_assert!(vec.comps() == 1); - self.phi_src.insert(phi_idx, vec[0]); + self.phi_src.insert(phi, vec[0]); } pub fn from_block(block: &BasicBlock) -> PhiSrcMap { let mut map = PhiSrcMap::new(); if let Some(op) = block.phi_srcs() { - for (idx, src) in op.srcs.iter() { - map.add_phi_src(*idx, src); + for (phi, src) in op.srcs.iter() { + map.add_phi_src(*phi, src); } } map } - pub fn get_src_ssa(&self, phi_idx: &u32) -> &SSAValue { - self.phi_src.get(phi_idx).expect("Phi source missing") + pub fn get_src_ssa(&self, phi: &Phi) -> &SSAValue { + self.phi_src.get(phi).expect("Phi source missing") } } @@ -495,7 +495,7 @@ fn spill_values( } let mut spill = SpillCache::new(&mut func.ssa_alloc, spill); - let mut spilled_phis: BitSet = BitSet::new(); + let mut spilled_phis: BitSet = BitSet::new(); let mut ssa_state_in: Vec = Vec::new(); let mut ssa_state_out: Vec = Vec::new(); @@ -599,7 +599,7 @@ fn spill_values( let phi_src_map = &phi_src_maps[*p_idx]; for mut ssa in ssa_state_out[*p_idx].w.iter().cloned() { - if let Some(phi) = phi_dst_map.get_phi_idx(&ssa) { + if let Some(phi) = phi_dst_map.get_phi(&ssa) { ssa = *phi_src_map.get_src_ssa(phi); } @@ -708,13 +708,13 @@ fn spill_values( Op::PhiDsts(op) => { // For phis, anything that is not in W needs to be spilled // by setting the destination to some spill value. - for (idx, dst) in op.dsts.iter_mut() { + for (phi, dst) in op.dsts.iter_mut() { let vec = dst.as_ssa().unwrap(); debug_assert!(vec.comps() == 1); let ssa = &vec[0]; if ssa.file() == file && !b.w.contains(ssa) { - spilled_phis.insert((*idx).try_into().unwrap()); + spilled_phis.insert(*phi); b.s.insert(*ssa); *dst = spill.get_spill(*ssa).into(); } @@ -974,7 +974,7 @@ fn spill_values( let mut fills = Vec::new(); if let Some(op) = pb.phi_srcs_mut() { - for (idx, src) in op.srcs.iter_mut() { + for (phi, src) in op.srcs.iter_mut() { debug_assert!(src.is_unmodified()); let vec = src.src_ref.as_ssa().unwrap(); debug_assert!(vec.comps() == 1); @@ -984,7 +984,7 @@ fn spill_values( continue; } - if spilled_phis.contains((*idx).try_into().unwrap()) { + if spilled_phis.contains(*phi) { if !p_out.s.contains(ssa) { spills.push(*ssa); } @@ -1005,7 +1005,7 @@ fn spill_values( } for ssa in s_in.w.iter() { - if phi_dst_map.get_phi_idx(ssa).is_some() { + if phi_dst_map.get_phi(ssa).is_some() { continue; } diff --git a/src/nouveau/compiler/nak/to_cssa.rs b/src/nouveau/compiler/nak/to_cssa.rs index 09a55aff1b9..82ca013fb87 100644 --- a/src/nouveau/compiler/nak/to_cssa.rs +++ b/src/nouveau/compiler/nak/to_cssa.rs @@ -54,7 +54,7 @@ where enum CoalesceItem { SSA(SSAValue), - Phi(u32), + Phi(Phi), } struct CoalesceNode { @@ -73,7 +73,7 @@ struct CoalesceGraph<'a> { nodes: Vec, sets: Vec, ssa_node: FxHashMap, - phi_node_file: FxHashMap, + phi_node_file: FxHashMap, } impl<'a> CoalesceGraph<'a> { @@ -102,7 +102,7 @@ impl<'a> CoalesceGraph<'a> { } } - fn add_phi_dst(&mut self, phi: u32, file: RegFile, block: usize) { + fn add_phi_dst(&mut self, phi: Phi, file: RegFile, block: usize) { debug_assert!(self.sets.is_empty()); // Record the register file now. We'll set the node later @@ -117,7 +117,7 @@ impl<'a> CoalesceGraph<'a> { }); } - fn add_phi_src(&mut self, phi: u32, block: usize) { + fn add_phi_src(&mut self, phi: Phi, block: usize) { debug_assert!(self.sets.is_empty()); self.nodes.push(CoalesceNode { @@ -250,7 +250,7 @@ impl<'a> CoalesceGraph<'a> { self.nodes[*self.ssa_node.get(ssa).unwrap()].set } - pub fn phi_set_file(&self, phi: &u32) -> (usize, RegFile) { + pub fn phi_set_file(&self, phi: &Phi) -> (usize, RegFile) { let (n, file) = self.phi_node_file.get(phi).unwrap(); (self.nodes[*n].set, *file) }