From 3031010fa9d67bfea7cf688409e38127f578cc8d Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Nov 2025 01:28:28 +0200 Subject: [PATCH 1/7] cf: always represent `ExitInvocation` as a node (instead of a `ControlInst`). --- src/cf/structurize.rs | 21 ----------- src/cf/unstructured.rs | 5 --- src/print/mod.rs | 12 ------- src/spv/lift.rs | 79 ++++++++++++++++++++++-------------------- src/spv/lower.rs | 24 ++++++++++--- src/transform.rs | 3 -- src/visit.rs | 3 -- 7 files changed, 61 insertions(+), 86 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 7d358120..827d0510 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -1127,27 +1127,6 @@ impl<'a> Structurizer<'a> { DeferredEdgeBundleSet::Unreachable } - ControlInstKind::ExitInvocation(kind) => { - assert_eq!(target_regions.len(), 0); - - let node = self.func_def_body.nodes.define( - self.cx, - NodeDef { - attrs, - kind: NodeKind::ExitInvocation(kind), - inputs, - child_regions: [].into_iter().collect(), - outputs: [].into_iter().collect(), - } - .into(), - ); - self.func_def_body.regions[region] - .children - .insert_last(node, &mut self.func_def_body.nodes); - - DeferredEdgeBundleSet::Unreachable - } - ControlInstKind::Return => { assert_eq!(target_regions.len(), 0); diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index af6a6254..a02305ec 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -51,11 +51,6 @@ pub enum ControlInstKind { /// Leave the current function, optionally returning a value. Return, - /// Leave the current invocation, similar to returning from every function - /// call in the stack (up to and including the entry-point), but potentially - /// indicating a fatal error as well. - ExitInvocation(cf::ExitInvocationKind), - /// Unconditional branch to a single target. Branch, diff --git a/src/print/mod.rs b/src/print/mod.rs index 6124ceee..2f2de2fe 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -4293,18 +4293,6 @@ impl Print for cf::unstructured::ControlInst { _ => unreachable!(), } } - cf::unstructured::ControlInstKind::ExitInvocation(cf::ExitInvocationKind::SpvInst( - spv::Inst { opcode, imms }, - )) => { - // FIXME(eddyb) use `targets.is_empty()` when that is stabilized. - assert!(targets.len() == 0); - printer.pretty_spv_inst( - kw_style, - *opcode, - imms, - inputs.iter().map(|v| v.print(printer)), - ) - } cf::unstructured::ControlInstKind::Branch => { assert_eq!((targets.len(), inputs.len()), (1, 0)); diff --git a/src/spv/lift.rs b/src/spv/lift.rs index cb9ac3d3..69e19015 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -14,7 +14,6 @@ use crate::{ use itertools::Itertools; use rustc_hash::FxHashMap; use smallvec::SmallVec; -use std::borrow::Cow; use std::collections::{BTreeMap, BTreeSet}; use std::num::NonZeroU32; use std::path::Path; @@ -321,7 +320,7 @@ struct Phi { struct Terminator<'a> { attrs: AttrSet, - kind: Cow<'a, cf::unstructured::ControlInstKind>, + kind: TerminatorKind<'a>, // FIXME(eddyb) use `Cow` or something, but ideally the "owned" case always // has at most one input, so allocating a whole `Vec` for that seems unwise. @@ -335,6 +334,17 @@ struct Terminator<'a> { merge: Option>, } +enum TerminatorKind<'a> { + Unreachable, + Return, + Branch, + SelectBranch(&'a cf::SelectionKind), + + // HACK(eddyb) this is the only case `cf::unstructured::ControlInst` can't + // itself represent (as it has been moved to `NodeKind::ExitInvocation`). + ExitInvocation(&'a cf::ExitInvocationKind), +} + #[derive(Copy, Clone)] enum Merge { Selection(L), @@ -679,7 +689,16 @@ impl<'a> FuncLifting<'a> { } = terminator; Terminator { attrs: *attrs, - kind: Cow::Borrowed(kind), + kind: match kind { + cf::unstructured::ControlInstKind::Unreachable => { + TerminatorKind::Unreachable + } + cf::unstructured::ControlInstKind::Return => TerminatorKind::Return, + cf::unstructured::ControlInstKind::Branch => TerminatorKind::Branch, + cf::unstructured::ControlInstKind::SelectBranch(kind) => { + TerminatorKind::SelectBranch(kind) + } + }, // FIXME(eddyb) borrow these whenever possible. inputs: inputs.clone(), targets: targets @@ -699,7 +718,7 @@ impl<'a> FuncLifting<'a> { assert!(region == func_def_body.body); Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Return), + kind: TerminatorKind::Return, inputs: func_def_body.at_body().def().outputs.clone(), targets: [].into_iter().collect(), target_phi_values: FxIndexMap::default(), @@ -714,9 +733,7 @@ impl<'a> FuncLifting<'a> { match &node_def.kind { NodeKind::Select(kind) => Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::SelectBranch( - kind.clone(), - )), + kind: TerminatorKind::SelectBranch(kind), inputs: [node_def.inputs[0]].into_iter().collect(), targets: node_def .child_regions @@ -731,7 +748,7 @@ impl<'a> FuncLifting<'a> { let body = node_def.child_regions[0]; Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Branch), + kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [CfgPoint::RegionEntry(body)].into_iter().collect(), target_phi_values: FxIndexMap::default(), @@ -751,9 +768,7 @@ impl<'a> FuncLifting<'a> { NodeKind::ExitInvocation(kind) => Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::ExitInvocation( - kind.clone(), - )), + kind: TerminatorKind::ExitInvocation(kind), inputs: node_def.inputs.clone(), targets: [].into_iter().collect(), target_phi_values: FxIndexMap::default(), @@ -782,7 +797,7 @@ impl<'a> FuncLifting<'a> { match func_def_body.at(parent_node).def().kind { NodeKind::Select { .. } => Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Branch), + kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [parent_exit].into_iter().collect(), target_phi_values: region_outputs @@ -813,7 +828,7 @@ impl<'a> FuncLifting<'a> { if is_infinite_loop { Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Branch), + kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [backedge].into_iter().collect(), target_phi_values, @@ -830,11 +845,7 @@ impl<'a> FuncLifting<'a> { } Terminator { attrs: AttrSet::default(), - kind: Cow::Owned( - cf::unstructured::ControlInstKind::SelectBranch( - SelectionKind::BoolCond, - ), - ), + kind: TerminatorKind::SelectBranch(&SelectionKind::BoolCond), inputs: [repeat_condition].into_iter().collect(), targets: [backedge, parent_exit].into_iter().collect(), target_phi_values, @@ -863,7 +874,7 @@ impl<'a> FuncLifting<'a> { // `unique_predecessor` helper (just like `unique_successor`). (_, Some(succ_cursor)) => Terminator { attrs: AttrSet::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Branch), + kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [succ_cursor.point].into_iter().collect(), target_phi_values: FxIndexMap::default(), @@ -936,10 +947,8 @@ impl<'a> FuncLifting<'a> { // SPIR-V allows their targets to just be the whole merge block // (the same one that `OpSelectionMerge` describes). let block = &blocks[block_idx]; - if let ( - cf::unstructured::ControlInstKind::SelectBranch(_), - Some(Merge::Selection(merge_point)), - ) = (&*block.terminator.kind, block.terminator.merge) + if let (TerminatorKind::SelectBranch(_), Some(Merge::Selection(merge_point))) = + (&block.terminator.kind, block.terminator.merge) { for target_idx in 0..block.terminator.targets.len() { let block = &blocks[block_idx]; @@ -966,7 +975,7 @@ impl<'a> FuncLifting<'a> { (phis.is_empty() && insts.is_empty() && *attrs == AttrSet::default() - && matches!(**kind, cf::unstructured::ControlInstKind::Branch) + && matches!(kind, TerminatorKind::Branch) && inputs.is_empty() && targets.len() == 1 && target_phi_values.is_empty() @@ -989,7 +998,7 @@ impl<'a> FuncLifting<'a> { &block.terminator; (*attrs == AttrSet::default() - && matches!(**kind, cf::unstructured::ControlInstKind::Branch) + && matches!(kind, TerminatorKind::Branch) && inputs.is_empty() && targets.len() == 1 && target_phi_values.is_empty() @@ -1015,7 +1024,7 @@ impl<'a> FuncLifting<'a> { new_terminator, Terminator { attrs: Default::default(), - kind: Cow::Owned(cf::unstructured::ControlInstKind::Unreachable), + kind: TerminatorKind::Unreachable, inputs: Default::default(), targets: Default::default(), target_phi_values: Default::default(), @@ -1429,25 +1438,21 @@ impl LazyInst<'_, '_> { ids: [merge_label_id, continue_label_id].into_iter().collect(), }, Self::Terminator { parent_func, terminator } => { - let inst = match &*terminator.kind { - cf::unstructured::ControlInstKind::Unreachable => wk.OpUnreachable.into(), - cf::unstructured::ControlInstKind::Return => { + let inst = match terminator.kind { + TerminatorKind::Unreachable => wk.OpUnreachable.into(), + TerminatorKind::Return => { if terminator.inputs.is_empty() { wk.OpReturn.into() } else { wk.OpReturnValue.into() } } - cf::unstructured::ControlInstKind::ExitInvocation( - cf::ExitInvocationKind::SpvInst(inst), - ) - | cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::SpvInst( - inst, - )) => inst.clone(), + TerminatorKind::ExitInvocation(cf::ExitInvocationKind::SpvInst(inst)) + | TerminatorKind::SelectBranch(SelectionKind::SpvInst(inst)) => inst.clone(), - cf::unstructured::ControlInstKind::Branch => wk.OpBranch.into(), + TerminatorKind::Branch => wk.OpBranch.into(), - cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::BoolCond) => { + TerminatorKind::SelectBranch(SelectionKind::BoolCond) => { wk.OpBranchConditional.into() } }; diff --git a/src/spv/lower.rs b/src/spv/lower.rs index 9384ac7f..bf426b1e 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -6,8 +6,8 @@ use crate::spv::{self, spec}; use crate::{ AddrSpace, Attr, AttrSet, Const, ConstDef, ConstKind, Context, DataInstDef, DataInstKind, DbgSrcLoc, DeclDef, Diag, EntityDefs, ExportKey, Exportee, Func, FuncDecl, FuncDefBody, - FuncParam, FxIndexMap, GlobalVarDecl, GlobalVarDefBody, Import, InternedStr, Module, Region, - RegionDef, Type, TypeDef, TypeKind, TypeOrConst, Value, VarDecl, print, + FuncParam, FxIndexMap, GlobalVarDecl, GlobalVarDefBody, Import, InternedStr, Module, NodeDef, + NodeKind, Region, RegionDef, Type, TypeDef, TypeKind, TypeOrConst, Value, VarDecl, print, }; use itertools::Either; use rustc_hash::FxHashMap; @@ -1491,9 +1491,23 @@ impl Module { assert!(targets.is_empty() && inputs.len() <= 1); cf::unstructured::ControlInstKind::Return } else if targets.is_empty() { - cf::unstructured::ControlInstKind::ExitInvocation( - cf::ExitInvocationKind::SpvInst(raw_inst.without_ids.clone()), - ) + let node = func_def_body.nodes.define( + &cx, + NodeDef { + attrs, + kind: NodeKind::ExitInvocation(cf::ExitInvocationKind::SpvInst( + raw_inst.without_ids.clone(), + )), + inputs: mem::take(&mut inputs), + child_regions: [].into_iter().collect(), + outputs: [].into_iter().collect(), + } + .into(), + ); + current_block_region_def + .children + .insert_last(node, &mut func_def_body.nodes); + cf::unstructured::ControlInstKind::Unreachable } else if opcode == wk.OpBranch { assert_eq!((targets.len(), inputs.len()), (1, 0)); cf::unstructured::ControlInstKind::Branch diff --git a/src/transform.rs b/src/transform.rs index 0c76ace4..ba469b6b 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -687,9 +687,6 @@ impl InnerInPlaceTransform for cf::unstructured::ControlInst { match kind { cf::unstructured::ControlInstKind::Unreachable | cf::unstructured::ControlInstKind::Return - | cf::unstructured::ControlInstKind::ExitInvocation(cf::ExitInvocationKind::SpvInst( - _, - )) | cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), diff --git a/src/visit.rs b/src/visit.rs index bd63094e..5c2da44a 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -521,9 +521,6 @@ impl InnerVisit for cf::unstructured::ControlInst { match kind { cf::unstructured::ControlInstKind::Unreachable | cf::unstructured::ControlInstKind::Return - | cf::unstructured::ControlInstKind::ExitInvocation(cf::ExitInvocationKind::SpvInst( - _, - )) | cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), From b6ea2a83a1ae2c5e338eb84bfbaae65784985e63 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Nov 2025 02:40:54 +0200 Subject: [PATCH 2/7] cf: remove phi-like limitation in favor of "BB args"-like edges-with-target-inputs. --- src/cf/structurize.rs | 45 ++++++++++++++++--------------- src/cf/unstructured.rs | 29 +++++++++++--------- src/print/mod.rs | 37 +++++++++++++++----------- src/spv/lift.rs | 60 +++++++++++++++++++++++++++++++----------- src/spv/lower.rs | 16 ++++++++--- src/transform.rs | 6 ++--- src/visit.rs | 6 ++--- 7 files changed, 123 insertions(+), 76 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 827d0510..83de45b5 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -4,7 +4,8 @@ use crate::cf::SelectionKind; use crate::cf::unstructured::{ - ControlInst, ControlInstKind, IncomingEdgeCount, LoopFinder, TraversalState, + ControlEdge, ControlInst, ControlInstKind, IncomingEdgeCount, LoopFinder, + TraversalState, }; use crate::transform::{InnerInPlaceTransform as _, Transformed, Transformer}; use crate::{ @@ -1056,16 +1057,17 @@ impl<'a> Structurizer<'a> { // always appending `Node`s (including the children of entire // `ClaimedRegion`s) to `region`'s definition itself. let mut deferred_edges = { - let ControlInst { attrs, kind, inputs, targets, target_inputs } = control_inst_on_exit; + let ControlInst { attrs, kind, inputs, targets } = control_inst_on_exit; let target_regions: SmallVec<[_; 8]> = targets .iter() - .map(|&target| { + .cloned() + .map(|ControlEdge { target, target_inputs }| { self.try_claim_edge_bundle(IncomingEdgeBundle { attrs: if targets.len() == 1 { attrs } else { AttrSet::default() }, target, accumulated_count: IncomingEdgeCount::ONE, - target_inputs: target_inputs.get(&target).cloned().unwrap_or_default(), + target_inputs, }) .map_err(|edge_bundle| { // HACK(eddyb) special-case "shared `unreachable`" to @@ -1807,16 +1809,17 @@ impl<'a> Structurizer<'a> { let taken_then; (taken_then, deferred_edges) = deferred_edges.split_out_matching(|deferred| match deferred.edge_bundle.target { - DeferredTarget::Region(target) => { - Ok((deferred.condition, (target, deferred.edge_bundle.target_inputs))) - } + DeferredTarget::Region(target) => Ok(( + deferred.condition, + ControlEdge { target, target_inputs: deferred.edge_bundle.target_inputs }, + )), DeferredTarget::Return => Err(deferred), }); - let Some((condition, then_target_and_inputs)) = taken_then else { + let Some((condition, then_edge)) = taken_then else { break; }; let branch_source = control_source.take().unwrap(); - let else_target_and_inputs = match deferred_edges { + let else_edge = match deferred_edges { // At most one deferral left, so it can be used as the "else" // case, or the branch left unconditional in its absence. DeferredEdgeBundleSet::Unreachable => None, @@ -1825,7 +1828,10 @@ impl<'a> Structurizer<'a> { edge_bundle, } => { deferred_edges = DeferredEdgeBundleSet::Unreachable; - Some((else_target, edge_bundle.target_inputs)) + Some(ControlEdge { + target: else_target, + target_inputs: edge_bundle.target_inputs, + }) } // Either more branches, or a deferred return, are needed, so @@ -1835,12 +1841,15 @@ impl<'a> Structurizer<'a> { let new_empty_region = self.func_def_body.regions.define(self.cx, RegionDef::default()); control_source = Some(new_empty_region); - Some((new_empty_region, [].into_iter().collect())) + Some(ControlEdge { + target: new_empty_region, + target_inputs: [].into_iter().collect(), + }) } }; let condition = Some(condition) - .filter(|_| else_target_and_inputs.is_some()) + .filter(|_| else_edge.is_some()) .map(|cond| self.materialize_lazy_cond(&cond)); let branch_control_inst = ControlInst { attrs: AttrSet::default(), @@ -1850,16 +1859,7 @@ impl<'a> Structurizer<'a> { ControlInstKind::Branch }, inputs: condition.into_iter().collect(), - targets: [&then_target_and_inputs] - .into_iter() - .chain(&else_target_and_inputs) - .map(|&(target, _)| target) - .collect(), - target_inputs: [then_target_and_inputs] - .into_iter() - .chain(else_target_and_inputs) - .filter(|(_, inputs)| !inputs.is_empty()) - .collect(), + targets: [then_edge].into_iter().chain(else_edge).collect(), }; assert!( self.func_def_body @@ -1901,7 +1901,6 @@ impl<'a> Structurizer<'a> { kind, inputs, targets: [].into_iter().collect(), - target_inputs: FxIndexMap::default(), } }; assert!( diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index a02305ec..0f6f2953 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -27,15 +27,8 @@ pub struct ControlInst { pub inputs: SmallVec<[Value; 2]>, // FIXME(eddyb) change the inline size of this to fit most instructions. - pub targets: SmallVec<[Region; 4]>, - - /// `target_inputs[region][input_idx]` is the [`Value`] that - /// `VarKind::RegionInput { region, input_idx }` will get on entry, - /// where `region` must be appear at least once in `targets` - this is a - /// separate map instead of being part of `targets` because it reflects the - /// limitations of φ ("phi") nodes, which (unlike "basic block arguments") - /// cannot tell apart multiple edges with the same source and destination. - pub target_inputs: FxIndexMap>, + // FIXME(eddyb) should this be renamed to ("outgoing") `edges`? + pub targets: SmallVec<[ControlEdge; 4]>, } #[derive(Clone)] @@ -58,6 +51,15 @@ pub enum ControlInstKind { SelectBranch(cf::SelectionKind), } +#[derive(Clone)] +pub struct ControlEdge { + pub target: Region, + + /// `target` inputs, i.e. `target_inputs[input_idx]` is the [`Value`] that + /// `VarKind::RegionInput { region: target, input_idx }` will get on entry. + pub target_inputs: SmallVec<[Value; 2]>, +} + impl ControlFlowGraph { /// Iterate over all [`Region`]s making up `func_def_body`'s CFG, in /// reverse post-order (RPO). @@ -159,7 +161,7 @@ impl ControlFlowGraph { .get(region) .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); - let targets = control_inst.targets.iter().copied(); + let targets = control_inst.targets.iter().map(|edge| edge.target); let targets = if state.reverse_targets { Either::Left(targets.rev()) } else { @@ -331,7 +333,7 @@ impl<'a> LoopFinder<'a> { let earliest_scc_root = control_inst .targets .iter() - .flat_map(|&target| { + .flat_map(|&ControlEdge { target, target_inputs: _ }| { let (earliest_scc_root_of_target, eventual_cfg_exits_of_target) = self.find_earliest_scc_root_of(target); eventual_cfg_exits |= eventual_cfg_exits_of_target; @@ -385,7 +387,10 @@ impl<'a> LoopFinder<'a> { self.scc_stack[scc_start..] .iter() .flat_map(|&scc_node| { - self.cfg.control_inst_on_exit_from[scc_node].targets.iter().copied() + self.cfg.control_inst_on_exit_from[scc_node] + .targets + .iter() + .map(|edge| edge.target) }) .filter(|&target| target_is_exit(target)) .collect(), diff --git a/src/print/mod.rs b/src/print/mod.rs index 2f2de2fe..af2522c2 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -619,8 +619,8 @@ impl<'a> Visitor<'a> for Plan<'a> { { for region in cfg.rev_post_order(func_def_body) { if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { - for &target in &control_inst.targets { - *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; + for edge in &control_inst.targets { + *self.use_counts.entry(Use::RegionLabel(edge.target)).or_default() += 1; } } } @@ -4256,27 +4256,32 @@ impl FuncAt<'_, DataInst> { impl Print for cf::unstructured::ControlInst { type Output = pretty::Fragment; fn print(&self, printer: &Printer<'_>) -> pretty::Fragment { - let Self { attrs, kind, inputs, targets, target_inputs } = self; + let Self { attrs, kind, inputs, targets } = self; let attrs = attrs.print(printer); let kw_style = printer.imperative_keyword_style(); let kw = |kw| kw_style.apply(kw).into(); - let mut targets = targets.iter().map(|&target_region| { - let mut target = pretty::Fragment::new([ - kw("branch"), - " ".into(), - Use::RegionLabel(target_region).print(printer), - ]); - if let Some(inputs) = target_inputs.get(&target_region) { - target = pretty::Fragment::new([ - target, - pretty::join_comma_sep("(", inputs.iter().map(|v| v.print(printer)), ")"), + let mut targets = + targets.iter().map(|cf::unstructured::ControlEdge { target, target_inputs }| { + let mut target = pretty::Fragment::new([ + kw("branch"), + " ".into(), + Use::RegionLabel(*target).print(printer), ]); - } - target - }); + if !target_inputs.is_empty() { + target = pretty::Fragment::new([ + target, + pretty::join_comma_sep( + "(", + target_inputs.iter().map(|v| v.print(printer)), + ")", + ), + ]); + } + target + }); let def = match kind { cf::unstructured::ControlInstKind::Unreachable => { diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 69e19015..51faf15a 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -288,6 +288,11 @@ enum CfgPoint { NodeEntry(Node), NodeExit(Node), + + // HACK(eddyb) this is only needed to recover φ ("phi") semantics for + // unstructured CFG edges, while letting SPIR-T use "BB args" semantics + // (i.e. potentially different values for the same target `Region`). + UnstructuredEdge { source: Region, edge_idx: u32 }, } struct BlockLifting<'a> { @@ -441,6 +446,10 @@ impl<'p> FuncAt<'_, CfgCursor<'p>> { CfgCursor { point: CfgPoint::NodeExit(parent_node), parent: parent.parent } } }), + CfgPoint::UnstructuredEdge { .. } => { + assert!(cursor.parent.is_none()); + None + } // Entering a `Node` depends entirely on the `NodeKind`. CfgPoint::NodeEntry(node) => match self.at(node).def().kind { @@ -593,7 +602,7 @@ impl<'a> FuncLifting<'a> { .collect::>()? } } - CfgPoint::RegionExit(_) => SmallVec::new(), + CfgPoint::RegionExit(_) | CfgPoint::UnstructuredEdge { .. } => SmallVec::new(), CfgPoint::NodeEntry(node) => { let node_def = func_def_body.at(node).def(); @@ -680,13 +689,8 @@ impl<'a> FuncLifting<'a> { .as_ref() .and_then(|cfg| cfg.control_inst_on_exit_from.get(region)); if let Some(terminator) = unstructured_terminator { - let cf::unstructured::ControlInst { - attrs, - kind, - inputs, - targets, - target_inputs, - } = terminator; + let cf::unstructured::ControlInst { attrs, kind, inputs, targets } = + terminator; Terminator { attrs: *attrs, kind: match kind { @@ -701,16 +705,15 @@ impl<'a> FuncLifting<'a> { }, // FIXME(eddyb) borrow these whenever possible. inputs: inputs.clone(), - targets: targets - .iter() - .map(|&target| CfgPoint::RegionEntry(target)) - .collect(), - target_phi_values: target_inputs - .iter() - .map(|(&target, target_inputs)| { - (CfgPoint::RegionEntry(target), &target_inputs[..]) + // FIXME(eddyb) try limiting this to repeated target `Region`s + // which *also* pass different value inputs. + targets: (0..u32::try_from(targets.len()).unwrap()) + .map(|edge_idx| CfgPoint::UnstructuredEdge { + source: region, + edge_idx, }) .collect(), + target_phi_values: FxIndexMap::default(), merge: None, } } else { @@ -726,6 +729,21 @@ impl<'a> FuncLifting<'a> { } } } + (CfgPoint::UnstructuredEdge { source, edge_idx }, None) => { + let cfg = func_def_body.unstructured_cfg.as_ref().unwrap(); + let cf::unstructured::ControlEdge { target, target_inputs } = + &cfg.control_inst_on_exit_from[source].targets[edge_idx as usize]; + Terminator { + attrs: AttrSet::default(), + kind: TerminatorKind::Branch, + inputs: [].into_iter().collect(), + targets: [CfgPoint::RegionEntry(*target)].into_iter().collect(), + target_phi_values: [(CfgPoint::RegionEntry(*target), &target_inputs[..])] + .into_iter() + .collect(), + merge: None, + } + } // Entering a `Node` with child `Region`s (or diverging). (CfgPoint::NodeEntry(node), None) => { @@ -896,6 +914,16 @@ impl<'a> FuncLifting<'a> { Some(cfg) => { for region in cfg.rev_post_order(func_def_body) { func_def_body.at(region).rev_post_order_try_for_each(&mut visit_cfg_point)?; + + // FIXME(eddyb) try limiting this to repeated target `Region`s + // which *also* pass different value inputs. + let edge_count = cfg.control_inst_on_exit_from[region].targets.len(); + for edge_idx in 0..u32::try_from(edge_count).unwrap() { + visit_cfg_point(CfgCursor { + point: CfgPoint::UnstructuredEdge { source: region, edge_idx }, + parent: None, + })?; + } } } } diff --git a/src/spv/lower.rs b/src/spv/lower.rs index bf426b1e..ee6fd416 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -1465,7 +1465,7 @@ impl Module { // Split the operands into value inputs (e.g. a branch's // condition or an `OpSwitch`'s selector) and target blocks. let mut inputs = SmallVec::new(); - let mut targets = SmallVec::new(); + let mut targets = SmallVec::<[_; 4]>::new(); for &id in ids { match lookup_global_or_local_id_for_data_or_control_inst_input(id)? { LocalIdDef::Value(v) => { @@ -1533,8 +1533,18 @@ impl Module { attrs, kind, inputs, - targets, - target_inputs, + // FIXME(eddyb) collect targets in this form to + // begin with (instead of recombining them here). + targets: targets + .into_iter() + .map(|target| cf::unstructured::ControlEdge { + target, + target_inputs: target_inputs + .get(&target) + .cloned() + .unwrap_or_default(), + }) + .collect(), }, ); } else if opcode == wk.OpPhi { diff --git a/src/transform.rs b/src/transform.rs index ba469b6b..83f2fa60 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -681,7 +681,7 @@ impl InnerInPlaceTransform for VarDecl { impl InnerInPlaceTransform for cf::unstructured::ControlInst { fn inner_in_place_transform_with(&mut self, transformer: &mut impl Transformer) { - let Self { attrs, kind, inputs, targets: _, target_inputs } = self; + let Self { attrs, kind, inputs, targets } = self; transformer.transform_attr_set_use(*attrs).apply_to(attrs); match kind { @@ -695,8 +695,8 @@ impl InnerInPlaceTransform for cf::unstructured::ControlInst { for v in inputs { transformer.transform_value_use(v).apply_to(v); } - for inputs in target_inputs.values_mut() { - for v in inputs { + for cf::unstructured::ControlEdge { target: _, target_inputs } in targets { + for v in target_inputs { transformer.transform_value_use(v).apply_to(v); } } diff --git a/src/visit.rs b/src/visit.rs index 5c2da44a..f2ec5cb8 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -515,7 +515,7 @@ impl InnerVisit for VarDecl { impl InnerVisit for cf::unstructured::ControlInst { fn inner_visit_with<'a>(&'a self, visitor: &mut impl Visitor<'a>) { - let Self { attrs, kind, inputs, targets: _, target_inputs } = self; + let Self { attrs, kind, inputs, targets } = self; visitor.visit_attr_set_use(*attrs); match kind { @@ -529,8 +529,8 @@ impl InnerVisit for cf::unstructured::ControlInst { for v in inputs { visitor.visit_value_use(v); } - for inputs in target_inputs.values() { - for v in inputs { + for cf::unstructured::ControlEdge { target: _, target_inputs } in targets { + for v in target_inputs { visitor.visit_value_use(v); } } From 3096b8fd197f7a93b91a6b44f5e78b9c423f8d77 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Nov 2025 05:06:38 +0200 Subject: [PATCH 3/7] cf: move unstructured returns from `ControlInstKind` to (per-edge) `ControlTarget`. --- src/cf/structurize.rs | 110 ++++++++++++++++++----------------------- src/cf/unstructured.rs | 47 ++++++++++++------ src/print/mod.rs | 29 ++++++----- src/spv/lift.rs | 35 +++++++++---- src/spv/lower.rs | 34 ++++++------- src/transform.rs | 1 - src/visit.rs | 1 - 7 files changed, 136 insertions(+), 121 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 83de45b5..08d518fe 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -4,8 +4,8 @@ use crate::cf::SelectionKind; use crate::cf::unstructured::{ - ControlEdge, ControlInst, ControlInstKind, IncomingEdgeCount, LoopFinder, - TraversalState, + ControlEdge, ControlInst, ControlInstKind, ControlTarget, IncomingEdgeCount, + LoopFinder, TraversalState, }; use crate::transform::{InnerInPlaceTransform as _, Transformed, Transformer}; use crate::{ @@ -254,12 +254,14 @@ struct LazyCondDyn { /// A target for one of the edge bundles in a [`DeferredEdgeBundleSet`], mostly /// separate from [`Region`] to allow expressing returns as well. +// +// FIXME(eddyb) consider reusing `ControlTarget` for this. #[derive(Copy, Clone, PartialEq, Eq, Hash)] enum DeferredTarget { Region(Region), /// Structured "return" out of the function (with `target_inputs` used for - /// the function body `output`s, i.e. inputs of [`ControlInstKind::Return`]). + /// the function body `output`s). Return, } @@ -1063,6 +1065,20 @@ impl<'a> Structurizer<'a> { .iter() .cloned() .map(|ControlEdge { target, target_inputs }| { + let target = match target { + ControlTarget::Region(target) => target, + ControlTarget::Return => { + return Err(DeferredEdgeBundleSet::Always { + target: DeferredTarget::Return, + edge_bundle: IncomingEdgeBundle { + attrs, + accumulated_count: IncomingEdgeCount::default(), + target: (), + target_inputs, + }, + }); + } + }; self.try_claim_edge_bundle(IncomingEdgeBundle { attrs: if targets.len() == 1 { attrs } else { AttrSet::default() }, target, @@ -1129,20 +1145,6 @@ impl<'a> Structurizer<'a> { DeferredEdgeBundleSet::Unreachable } - ControlInstKind::Return => { - assert_eq!(target_regions.len(), 0); - - DeferredEdgeBundleSet::Always { - target: DeferredTarget::Return, - edge_bundle: IncomingEdgeBundle { - attrs, - accumulated_count: IncomingEdgeCount::default(), - target: (), - target_inputs: inputs, - }, - } - } - ControlInstKind::Branch => { assert_eq!(inputs.len(), 0); @@ -1807,14 +1809,18 @@ impl<'a> Structurizer<'a> { let mut control_source = Some(region); loop { let taken_then; - (taken_then, deferred_edges) = - deferred_edges.split_out_matching(|deferred| match deferred.edge_bundle.target { - DeferredTarget::Region(target) => Ok(( - deferred.condition, - ControlEdge { target, target_inputs: deferred.edge_bundle.target_inputs }, - )), - DeferredTarget::Return => Err(deferred), - }); + (taken_then, deferred_edges) = deferred_edges.split_out_matching(|deferred| { + Ok(( + deferred.condition, + ControlEdge { + target: match deferred.edge_bundle.target { + DeferredTarget::Region(target) => ControlTarget::Region(target), + DeferredTarget::Return => ControlTarget::Return, + }, + target_inputs: deferred.edge_bundle.target_inputs, + }, + )) + }); let Some((condition, then_edge)) = taken_then else { break; }; @@ -1823,26 +1829,25 @@ impl<'a> Structurizer<'a> { // At most one deferral left, so it can be used as the "else" // case, or the branch left unconditional in its absence. DeferredEdgeBundleSet::Unreachable => None, - DeferredEdgeBundleSet::Always { - target: DeferredTarget::Region(else_target), - edge_bundle, - } => { + DeferredEdgeBundleSet::Always { target: else_target, edge_bundle } => { deferred_edges = DeferredEdgeBundleSet::Unreachable; Some(ControlEdge { - target: else_target, + target: match else_target { + DeferredTarget::Region(target) => ControlTarget::Region(target), + DeferredTarget::Return => ControlTarget::Return, + }, target_inputs: edge_bundle.target_inputs, }) } - // Either more branches, or a deferred return, are needed, so - // the "else" case must be a `Region` that itself can - // have a `ControlInst` attached to it later on. - _ => { + // More branches are needed, so the "else" case must be a `Region` + // that itself can have a `ControlInst` attached to it later on. + DeferredEdgeBundleSet::Choice { .. } => { let new_empty_region = self.func_def_body.regions.define(self.cx, RegionDef::default()); control_source = Some(new_empty_region); Some(ControlEdge { - target: new_empty_region, + target: ControlTarget::Region(new_empty_region), target_inputs: [].into_iter().collect(), }) } @@ -1872,36 +1877,19 @@ impl<'a> Structurizer<'a> { ); } - let deferred_return = match deferred_edges { - DeferredEdgeBundleSet::Unreachable => None, - DeferredEdgeBundleSet::Always { target: DeferredTarget::Return, edge_bundle } => { - Some(edge_bundle.target_inputs) - } - _ => unreachable!(), - }; - let final_source = match control_source { Some(region) => region, - None => { - // The loop above handled all the targets, nothing left to do. - assert!(deferred_return.is_none()); - return; - } + // The loop above handled all the targets, nothing left to do. + None => return, }; - // Final deferral is either a `Return` (if needed), or an `Unreachable` - // (only when truly divergent, i.e. no `deferred_edges`/`deferred_return`). - let final_control_inst = { - let (kind, inputs) = match deferred_return { - Some(return_values) => (ControlInstKind::Return, return_values), - None => (ControlInstKind::Unreachable, [].into_iter().collect()), - }; - ControlInst { - attrs: AttrSet::default(), - kind, - inputs, - targets: [].into_iter().collect(), - } + // Final deferral is an `Unreachable` (only when truly divergent, + // i.e. no `deferred_edges`). + let final_control_inst = ControlInst { + attrs: AttrSet::default(), + kind: ControlInstKind::Unreachable, + inputs: [].into_iter().collect(), + targets: [].into_iter().collect(), }; assert!( self.func_def_body diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index 0f6f2953..a89fc4fb 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -41,9 +41,6 @@ pub enum ControlInstKind { /// necessary preconditions for reaching this point, are never met. Unreachable, - /// Leave the current function, optionally returning a value. - Return, - /// Unconditional branch to a single target. Branch, @@ -53,13 +50,21 @@ pub enum ControlInstKind { #[derive(Clone)] pub struct ControlEdge { - pub target: Region, + pub target: ControlTarget, /// `target` inputs, i.e. `target_inputs[input_idx]` is the [`Value`] that /// `VarKind::RegionInput { region: target, input_idx }` will get on entry. pub target_inputs: SmallVec<[Value; 2]>, } +#[derive(Copy, Clone)] +pub enum ControlTarget { + Region(Region), + + /// Leave the current function (returning `target_inputs`, if any). + Return, +} + impl ControlFlowGraph { /// Iterate over all [`Region`]s making up `func_def_body`'s CFG, in /// reverse post-order (RPO). @@ -161,7 +166,10 @@ impl ControlFlowGraph { .get(region) .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); - let targets = control_inst.targets.iter().map(|edge| edge.target); + let targets = control_inst.targets.iter().filter_map(|edge| match edge.target { + ControlTarget::Region(target) => Some(target), + ControlTarget::Return => None, + }); let targets = if state.reverse_targets { Either::Left(targets.rev()) } else { @@ -309,7 +317,7 @@ impl<'a> LoopFinder<'a> { // `Complete` state until the loop header itself is complete, // and the monotonic nature of `EventualCfgExits` means that // the loop header will still get to see the complete picture. - (Some(scc_stack_idx), EventualCfgExits::default()) + (Some(scc_stack_idx), EventualCfgExits { may_return_from_func: false }) } SccState::Complete(eventual_cfg_exits) => (None, eventual_cfg_exits), }; @@ -324,16 +332,21 @@ impl<'a> LoopFinder<'a> { .get(node) .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); - let mut eventual_cfg_exits = EventualCfgExits::default(); - - if let ControlInstKind::Return = control_inst.kind { - eventual_cfg_exits.may_return_from_func = true; - } + let mut eventual_cfg_exits = EventualCfgExits { + may_return_from_func: control_inst + .targets + .iter() + .any(|edge| matches!(edge.target, ControlTarget::Return)), + }; let earliest_scc_root = control_inst .targets .iter() - .flat_map(|&ControlEdge { target, target_inputs: _ }| { + .filter_map(|edge| match edge.target { + ControlTarget::Region(target) => Some(target), + ControlTarget::Return => None, + }) + .flat_map(|target| { let (earliest_scc_root_of_target, eventual_cfg_exits_of_target) = self.find_earliest_scc_root_of(target); eventual_cfg_exits |= eventual_cfg_exits_of_target; @@ -387,10 +400,12 @@ impl<'a> LoopFinder<'a> { self.scc_stack[scc_start..] .iter() .flat_map(|&scc_node| { - self.cfg.control_inst_on_exit_from[scc_node] - .targets - .iter() - .map(|edge| edge.target) + self.cfg.control_inst_on_exit_from[scc_node].targets.iter().filter_map( + |edge| match edge.target { + ControlTarget::Region(target) => Some(target), + ControlTarget::Return => None, + }, + ) }) .filter(|&target| target_is_exit(target)) .collect(), diff --git a/src/print/mod.rs b/src/print/mod.rs index af2522c2..7827bf91 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -620,7 +620,12 @@ impl<'a> Visitor<'a> for Plan<'a> { for region in cfg.rev_post_order(func_def_body) { if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { for edge in &control_inst.targets { - *self.use_counts.entry(Use::RegionLabel(edge.target)).or_default() += 1; + match edge.target { + cf::unstructured::ControlTarget::Region(target) => { + *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; + } + cf::unstructured::ControlTarget::Return => {} + } } } } @@ -4265,11 +4270,14 @@ impl Print for cf::unstructured::ControlInst { let mut targets = targets.iter().map(|cf::unstructured::ControlEdge { target, target_inputs }| { - let mut target = pretty::Fragment::new([ - kw("branch"), - " ".into(), - Use::RegionLabel(*target).print(printer), - ]); + let mut target = match *target { + cf::unstructured::ControlTarget::Region(target) => pretty::Fragment::new([ + kw("branch"), + " ".into(), + Use::RegionLabel(target).print(printer), + ]), + cf::unstructured::ControlTarget::Return => kw("return"), + }; if !target_inputs.is_empty() { target = pretty::Fragment::new([ target, @@ -4289,15 +4297,6 @@ impl Print for cf::unstructured::ControlInst { assert!(targets.len() == 0 && inputs.is_empty()); kw("unreachable") } - cf::unstructured::ControlInstKind::Return => { - // FIXME(eddyb) use `targets.is_empty()` when that is stabilized. - assert!(targets.len() == 0); - match inputs[..] { - [] => kw("return"), - [v] => pretty::Fragment::new([kw("return"), " ".into(), v.print(printer)]), - _ => unreachable!(), - } - } cf::unstructured::ControlInstKind::Branch => { assert_eq!((targets.len(), inputs.len()), (1, 0)); diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 51faf15a..23c4cd97 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -697,7 +697,6 @@ impl<'a> FuncLifting<'a> { cf::unstructured::ControlInstKind::Unreachable => { TerminatorKind::Unreachable } - cf::unstructured::ControlInstKind::Return => TerminatorKind::Return, cf::unstructured::ControlInstKind::Branch => TerminatorKind::Branch, cf::unstructured::ControlInstKind::SelectBranch(kind) => { TerminatorKind::SelectBranch(kind) @@ -707,6 +706,7 @@ impl<'a> FuncLifting<'a> { inputs: inputs.clone(), // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. + // NOTE(eddyb) this is also now used for returns. targets: (0..u32::try_from(targets.len()).unwrap()) .map(|edge_idx| CfgPoint::UnstructuredEdge { source: region, @@ -731,17 +731,33 @@ impl<'a> FuncLifting<'a> { } (CfgPoint::UnstructuredEdge { source, edge_idx }, None) => { let cfg = func_def_body.unstructured_cfg.as_ref().unwrap(); + let cf::unstructured::ControlInst { attrs, kind: _, inputs: _, targets } = + &cfg.control_inst_on_exit_from[source]; let cf::unstructured::ControlEdge { target, target_inputs } = - &cfg.control_inst_on_exit_from[source].targets[edge_idx as usize]; - Terminator { - attrs: AttrSet::default(), - kind: TerminatorKind::Branch, - inputs: [].into_iter().collect(), - targets: [CfgPoint::RegionEntry(*target)].into_iter().collect(), - target_phi_values: [(CfgPoint::RegionEntry(*target), &target_inputs[..])] + &targets[edge_idx as usize]; + match *target { + cf::unstructured::ControlTarget::Region(target) => Terminator { + attrs: *attrs, + kind: TerminatorKind::Branch, + inputs: [].into_iter().collect(), + targets: [CfgPoint::RegionEntry(target)].into_iter().collect(), + target_phi_values: [( + CfgPoint::RegionEntry(target), + &target_inputs[..], + )] .into_iter() .collect(), - merge: None, + merge: None, + }, + cf::unstructured::ControlTarget::Return => Terminator { + attrs: *attrs, + kind: TerminatorKind::Return, + // FIXME(eddyb) borrow these whenever possible. + inputs: target_inputs.clone(), + targets: [].into_iter().collect(), + target_phi_values: FxIndexMap::default(), + merge: None, + }, } } @@ -917,6 +933,7 @@ impl<'a> FuncLifting<'a> { // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. + // NOTE(eddyb) this is also now used for returns. let edge_count = cfg.control_inst_on_exit_from[region].targets.len(); for edge_idx in 0..u32::try_from(edge_count).unwrap() { visit_cfg_point(CfgCursor { diff --git a/src/spv/lower.rs b/src/spv/lower.rs index ee6fd416..fae256d1 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -1484,12 +1484,26 @@ impl Module { } } + // FIXME(eddyb) collect targets in this form to + // begin with (instead of recombining them here). + let mut targets: SmallVec<[_; 4]> = targets + .into_iter() + .map(|target| cf::unstructured::ControlEdge { + target: cf::unstructured::ControlTarget::Region(target), + target_inputs: target_inputs.get(&target).cloned().unwrap_or_default(), + }) + .collect(); + let kind = if opcode == wk.OpUnreachable { assert!(targets.is_empty() && inputs.is_empty()); cf::unstructured::ControlInstKind::Unreachable } else if [wk.OpReturn, wk.OpReturnValue].contains(&opcode) { assert!(targets.is_empty() && inputs.len() <= 1); - cf::unstructured::ControlInstKind::Return + targets.push(cf::unstructured::ControlEdge { + target: cf::unstructured::ControlTarget::Return, + target_inputs: mem::take(&mut inputs), + }); + cf::unstructured::ControlInstKind::Branch } else if targets.is_empty() { let node = func_def_body.nodes.define( &cx, @@ -1529,23 +1543,7 @@ impl Module { .control_inst_on_exit_from .insert( current_block.region, - cf::unstructured::ControlInst { - attrs, - kind, - inputs, - // FIXME(eddyb) collect targets in this form to - // begin with (instead of recombining them here). - targets: targets - .into_iter() - .map(|target| cf::unstructured::ControlEdge { - target, - target_inputs: target_inputs - .get(&target) - .cloned() - .unwrap_or_default(), - }) - .collect(), - }, + cf::unstructured::ControlInst { attrs, kind, inputs, targets }, ); } else if opcode == wk.OpPhi { if !current_block_region_def.children.is_empty() { diff --git a/src/transform.rs b/src/transform.rs index 83f2fa60..8921c2cc 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -686,7 +686,6 @@ impl InnerInPlaceTransform for cf::unstructured::ControlInst { transformer.transform_attr_set_use(*attrs).apply_to(attrs); match kind { cf::unstructured::ControlInstKind::Unreachable - | cf::unstructured::ControlInstKind::Return | cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), diff --git a/src/visit.rs b/src/visit.rs index f2ec5cb8..bc54d09d 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -520,7 +520,6 @@ impl InnerVisit for cf::unstructured::ControlInst { visitor.visit_attr_set_use(*attrs); match kind { cf::unstructured::ControlInstKind::Unreachable - | cf::unstructured::ControlInstKind::Return | cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), From 6e958fab218805d171d9fe9ebe76ef8841c0c9db Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 21 Nov 2025 16:01:40 +0200 Subject: [PATCH 4/7] [TODO(eddyb) document] cf: reify `ControlEdge`s (target+inputs) into a new kind of `thunk` "value". --- src/cf/structurize.rs | 123 +++++++++++++++++++++++++++++++---------- src/cf/unstructured.rs | 99 +++++++++++++++++++++------------ src/lib.rs | 6 ++ src/mem/analyze.rs | 16 ++++++ src/mem/layout.rs | 3 + src/print/mod.rs | 80 +++++++++++++-------------- src/qptr/lift.rs | 11 ++++ src/qptr/lower.rs | 3 +- src/spv/lift.rs | 57 ++++++++++++++----- src/spv/lower.rs | 64 ++++++++++++++++----- src/transform.rs | 11 ++-- src/visit.rs | 11 ++-- 12 files changed, 337 insertions(+), 147 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 08d518fe..3e40da91 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -4,9 +4,9 @@ use crate::cf::SelectionKind; use crate::cf::unstructured::{ - ControlEdge, ControlInst, ControlInstKind, ControlTarget, IncomingEdgeCount, - LoopFinder, TraversalState, + ControlInst, ControlInstKind, ControlTarget, IncomingEdgeCount, LoopFinder, TraversalState, }; +use crate::func_at::FuncAtMut; use crate::transform::{InnerInPlaceTransform as _, Transformed, Transformer}; use crate::{ AttrSet, Const, ConstDef, ConstKind, Context, DbgSrcLoc, EntityOrientedDenseMap, FuncDefBody, @@ -589,8 +589,8 @@ impl<'a> Structurizer<'a> { .unstructured_cfg .as_ref() .map(|cfg| { - let loop_header_to_exit_targets = - LoopFinder::new(cfg).find_all_loops_starting_at(func_def_body.body); + let loop_header_to_exit_targets = LoopFinder::new(func_def_body.at(()), cfg) + .find_all_loops_starting_at(func_def_body.body); let mut state = TraversalState { incoming_edge_counts: EntityOrientedDenseMap::new(), @@ -1059,12 +1059,38 @@ impl<'a> Structurizer<'a> { // always appending `Node`s (including the children of entire // `ClaimedRegion`s) to `region`'s definition itself. let mut deferred_edges = { - let ControlInst { attrs, kind, inputs, targets } = control_inst_on_exit; + let ControlInst { attrs, kind, inputs, target_thunks } = control_inst_on_exit; - let target_regions: SmallVec<[_; 8]> = targets + let target_regions: SmallVec<[_; 8]> = target_thunks .iter() - .cloned() - .map(|ControlEdge { target, target_inputs }| { + .map(|&thunk| { + let (target, target_inputs) = match thunk { + Value::Var(thunk) => match self.func_def_body.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => { + let thunk_node_def = self.func_def_body.at_mut(node).def(); + let target = match thunk_node_def.kind { + NodeKind::ThunkBind(target) => target, + _ => unreachable!(), + }; + + // NOTE(eddyb) this can only occur if the thunk + // is used more than once (which is illegal). + assert!(thunk_node_def.outputs[..] == [thunk]); + thunk_node_def.outputs.clear(); + + let target_inputs = mem::take(&mut thunk_node_def.inputs); + + self.func_def_body.regions[region] + .children + .remove(node, &mut self.func_def_body.nodes); + + (target, target_inputs) + } + _ => unreachable!(), + }, + Value::Const(_) => unreachable!(), + }; + let target = match target { ControlTarget::Region(target) => target, ControlTarget::Return => { @@ -1080,7 +1106,7 @@ impl<'a> Structurizer<'a> { } }; self.try_claim_edge_bundle(IncomingEdgeBundle { - attrs: if targets.len() == 1 { attrs } else { AttrSet::default() }, + attrs: if target_thunks.len() == 1 { attrs } else { AttrSet::default() }, target, accumulated_count: IncomingEdgeCount::ONE, target_inputs, @@ -1805,6 +1831,46 @@ impl<'a> Structurizer<'a> { after it takes `structurize_region_state`" ); + let cx = self.cx; + + let thunk_ty = cx.intern(TypeKind::Thunk); + let build_thunk = |func_at_region: FuncAtMut<'_, Region>, target, target_inputs| { + let region = func_at_region.position; + let func = func_at_region.at(()); + + let target = match target { + DeferredTarget::Region(target) => ControlTarget::Region(target), + DeferredTarget::Return => ControlTarget::Return, + }; + + let thunk_node = func.nodes.define( + cx, + NodeDef { + attrs: AttrSet::default(), + kind: NodeKind::ThunkBind(target), + inputs: target_inputs, + child_regions: [].into_iter().collect(), + outputs: [].into_iter().collect(), + } + .into(), + ); + func.regions[region].children.insert_last(thunk_node, func.nodes); + + let thunk_var = func.vars.define( + cx, + VarDecl { + attrs: AttrSet::default(), + ty: thunk_ty, + + def_parent: Either::Right(thunk_node), + def_idx: 0, + }, + ); + func.nodes[thunk_node].outputs.push(thunk_var); + + Value::Var(thunk_var) + }; + // Build a chain of conditional branches to apply deferred edges. let mut control_source = Some(region); loop { @@ -1812,13 +1878,11 @@ impl<'a> Structurizer<'a> { (taken_then, deferred_edges) = deferred_edges.split_out_matching(|deferred| { Ok(( deferred.condition, - ControlEdge { - target: match deferred.edge_bundle.target { - DeferredTarget::Region(target) => ControlTarget::Region(target), - DeferredTarget::Return => ControlTarget::Return, - }, - target_inputs: deferred.edge_bundle.target_inputs, - }, + build_thunk( + self.func_def_body.at_mut(control_source.unwrap()), + deferred.edge_bundle.target, + deferred.edge_bundle.target_inputs, + ), )) }); let Some((condition, then_edge)) = taken_then else { @@ -1831,25 +1895,24 @@ impl<'a> Structurizer<'a> { DeferredEdgeBundleSet::Unreachable => None, DeferredEdgeBundleSet::Always { target: else_target, edge_bundle } => { deferred_edges = DeferredEdgeBundleSet::Unreachable; - Some(ControlEdge { - target: match else_target { - DeferredTarget::Region(target) => ControlTarget::Region(target), - DeferredTarget::Return => ControlTarget::Return, - }, - target_inputs: edge_bundle.target_inputs, - }) + Some(build_thunk( + self.func_def_body.at_mut(branch_source), + else_target, + edge_bundle.target_inputs, + )) } // More branches are needed, so the "else" case must be a `Region` // that itself can have a `ControlInst` attached to it later on. DeferredEdgeBundleSet::Choice { .. } => { let new_empty_region = - self.func_def_body.regions.define(self.cx, RegionDef::default()); + self.func_def_body.regions.define(cx, RegionDef::default()); control_source = Some(new_empty_region); - Some(ControlEdge { - target: ControlTarget::Region(new_empty_region), - target_inputs: [].into_iter().collect(), - }) + Some(build_thunk( + self.func_def_body.at_mut(branch_source), + DeferredTarget::Region(new_empty_region), + [].into_iter().collect(), + )) } }; @@ -1864,7 +1927,7 @@ impl<'a> Structurizer<'a> { ControlInstKind::Branch }, inputs: condition.into_iter().collect(), - targets: [then_edge].into_iter().chain(else_edge).collect(), + target_thunks: [then_edge].into_iter().chain(else_edge).collect(), }; assert!( self.func_def_body @@ -1889,7 +1952,7 @@ impl<'a> Structurizer<'a> { attrs: AttrSet::default(), kind: ControlInstKind::Unreachable, inputs: [].into_iter().collect(), - targets: [].into_iter().collect(), + target_thunks: [].into_iter().collect(), }; assert!( self.func_def_body diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index a89fc4fb..b4c3cb9b 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -1,7 +1,8 @@ //! Unstructured control-flow graph (CFG) abstractions and utilities. use crate::{ - AttrSet, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, FxIndexSet, Region, Value, cf, + AttrSet, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, FxIndexSet, NodeKind, Region, Value, + VarKind, cf, }; use itertools::Either; use smallvec::SmallVec; @@ -28,7 +29,7 @@ pub struct ControlInst { // FIXME(eddyb) change the inline size of this to fit most instructions. // FIXME(eddyb) should this be renamed to ("outgoing") `edges`? - pub targets: SmallVec<[ControlEdge; 4]>, + pub target_thunks: SmallVec<[Value; 4]>, } #[derive(Clone)] @@ -39,6 +40,8 @@ pub enum ControlInstKind { /// /// Optimizations can take advantage of this information, to assume that any /// necessary preconditions for reaching this point, are never met. + // + // FIXME(eddyb) turn this into an `undef` thunk. Unreachable, /// Unconditional branch to a single target. @@ -48,23 +51,38 @@ pub enum ControlInstKind { SelectBranch(cf::SelectionKind), } -#[derive(Clone)] -pub struct ControlEdge { - pub target: ControlTarget, - - /// `target` inputs, i.e. `target_inputs[input_idx]` is the [`Value`] that - /// `VarKind::RegionInput { region: target, input_idx }` will get on entry. - pub target_inputs: SmallVec<[Value; 2]>, -} - -#[derive(Copy, Clone)] +#[derive(Copy, Clone, PartialEq, Eq, Hash)] pub enum ControlTarget { Region(Region), /// Leave the current function (returning `target_inputs`, if any). + // + // FIXME(eddyb) now that this is used through `NodeKind::ThunkBind`, + // it should probably be more like `break` or some kind of "leave scope". Return, } +impl ControlTarget { + // HACK(eddyb) this isn't necessarily the best place, but allows reuse. + // FIXME(eddyb) properly distinguish the different failure cases possible, + // maybe even avoid panicking entirely? + fn of_thunk(func_at_thunk: FuncAt<'_, Value>) -> Self { + let thunk = func_at_thunk.position; + let func = func_at_thunk.at(()); + + match thunk { + Value::Var(thunk) => match func.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => match func.at(node).def().kind { + NodeKind::ThunkBind(target) => target, + _ => unreachable!(), + }, + _ => unreachable!(), + }, + Value::Const(_) => unreachable!(), + } + } +} + impl ControlFlowGraph { /// Iterate over all [`Region`]s making up `func_def_body`'s CFG, in /// reverse post-order (RPO). @@ -121,6 +139,7 @@ mod sealed { } } } +use crate::func_at::FuncAt; pub use sealed::IncomingEdgeCount; pub struct TraversalState { @@ -144,14 +163,17 @@ impl ControlFlowGraph { assert!(std::ptr::eq(func_def_body.unstructured_cfg.as_ref().unwrap(), self)); assert!(func_at_body.def().outputs.is_empty()); - self.traverse(func_def_body.body, state); + self.traverse(func_at_body, state); } fn traverse( &self, - region: Region, + func_at_region: FuncAt<'_, Region>, state: &mut TraversalState, ) { + let func = func_at_region.at(()); + let region = func_at_region.position; + // FIXME(eddyb) `EntityOrientedDenseMap` should have an `entry` API. if let Some(existing_count) = state.incoming_edge_counts.get_mut(region) { *existing_count += IncomingEdgeCount::ONE; @@ -166,9 +188,11 @@ impl ControlFlowGraph { .get(region) .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); - let targets = control_inst.targets.iter().filter_map(|edge| match edge.target { - ControlTarget::Region(target) => Some(target), - ControlTarget::Return => None, + let targets = control_inst.target_thunks.iter().filter_map(|&thunk| { + match ControlTarget::of_thunk(func.at(thunk)) { + ControlTarget::Region(target) => Some(target), + ControlTarget::Return => None, + } }); let targets = if state.reverse_targets { Either::Left(targets.rev()) @@ -176,7 +200,7 @@ impl ControlFlowGraph { Either::Right(targets) }; for target in targets { - self.traverse(target, state); + self.traverse(func.at(target), state); } (state.post_order_visit)(region); @@ -208,6 +232,7 @@ impl ControlFlowGraph { /// be misleading with explicit `break`s (moving user code from just before /// the `break` to after the loop), but is less impactful than "maximal loops" pub struct LoopFinder<'a> { + func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph, // FIXME(eddyb) this feels a bit inefficient (are many-exit loops rare?). @@ -272,8 +297,9 @@ impl std::ops::BitOrAssign for EventualCfgExits { } impl<'a> LoopFinder<'a> { - pub fn new(cfg: &'a ControlFlowGraph) -> Self { + pub fn new(func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph) -> Self { Self { + func, cfg, loop_header_to_exit_targets: FxIndexMap::default(), scc_stack: vec![], @@ -333,20 +359,20 @@ impl<'a> LoopFinder<'a> { .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); let mut eventual_cfg_exits = EventualCfgExits { - may_return_from_func: control_inst - .targets - .iter() - .any(|edge| matches!(edge.target, ControlTarget::Return)), + may_return_from_func: control_inst.target_thunks.iter().any(|&thunk| { + matches!(ControlTarget::of_thunk(self.func.at(thunk)), ControlTarget::Return) + }), }; let earliest_scc_root = control_inst - .targets + .target_thunks .iter() - .filter_map(|edge| match edge.target { - ControlTarget::Region(target) => Some(target), - ControlTarget::Return => None, - }) - .flat_map(|target| { + .flat_map(|&thunk| { + let target = match ControlTarget::of_thunk(self.func.at(thunk)) { + ControlTarget::Region(target) => target, + ControlTarget::Return => return None.into_iter().chain(None), + }; + let (earliest_scc_root_of_target, eventual_cfg_exits_of_target) = self.find_earliest_scc_root_of(target); eventual_cfg_exits |= eventual_cfg_exits_of_target; @@ -400,12 +426,15 @@ impl<'a> LoopFinder<'a> { self.scc_stack[scc_start..] .iter() .flat_map(|&scc_node| { - self.cfg.control_inst_on_exit_from[scc_node].targets.iter().filter_map( - |edge| match edge.target { - ControlTarget::Region(target) => Some(target), - ControlTarget::Return => None, - }, - ) + self.cfg.control_inst_on_exit_from[scc_node] + .target_thunks + .iter() + .filter_map(|&thunk| { + match ControlTarget::of_thunk(self.func.at(thunk)) { + ControlTarget::Region(target) => Some(target), + ControlTarget::Return => None, + } + }) }) .filter(|&target| target_is_exit(target)) .collect(), diff --git a/src/lib.rs b/src/lib.rs index 52c6ce1c..f1cbdb2f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -552,6 +552,9 @@ pub enum TypeKind { // and kept separately in `VarDecl`, might be a better approach? QPtr, + // TODO(eddyb) reconsider name? add signature? etc. + Thunk, + SpvInst { spv_inst: spv::Inst, // FIXME(eddyb) find a better name. @@ -907,6 +910,9 @@ pub enum NodeKind { #[from] QPtr(qptr::QPtrOp), + // TODO(eddyb) document (maybe move into e.g. `cf::ThunkOp`?). + ThunkBind(cf::unstructured::ControlTarget), + // FIXME(eddyb) should this have `#[from]`? SpvInst(spv::Inst), SpvExtInst { diff --git a/src/mem/analyze.rs b/src/mem/analyze.rs index 6f54919b..762ff2a8 100644 --- a/src/mem/analyze.rs +++ b/src/mem/analyze.rs @@ -934,6 +934,7 @@ impl<'a> GatherAccesses<'a> { DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => {} } @@ -1216,6 +1217,21 @@ impl<'a> GatherAccesses<'a> { ); } + DataInstKind::ThunkBind(_) => { + if data_inst_def + .inputs + .iter() + .any(|&v| is_qptr(func_def_body.at(v).type_of(&cx))) + { + accesses_or_err_attrs_to_attach.push(( + AttrTarget::Node(node), + Err(AnalysisError(Diag::bug([ + "unsupported `thunk.bind` with pointer inputs".into(), + ]))), + )); + } + } + DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => { let mut has_from_spv_ptr_output_attr = false; for attr in &cx[data_inst_def.attrs].attrs { diff --git a/src/mem/layout.rs b/src/mem/layout.rs index 7b0656af..9687afab 100644 --- a/src/mem/layout.rs +++ b/src/mem/layout.rs @@ -213,6 +213,9 @@ impl<'a> LayoutCache<'a> { ["`layout_of(qptr)` (already lowered?)".into()], ))); } + TypeKind::Thunk => { + return Err(LayoutError(Diag::bug(["`layout_of(thunk)`".into()]))); + } TypeKind::SpvInst { spv_inst, type_and_const_inputs } => { (spv_inst, type_and_const_inputs) } diff --git a/src/print/mod.rs b/src/print/mod.rs index 7827bf91..ecd96e49 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -613,25 +613,16 @@ impl<'a> Visitor<'a> for Plan<'a> { } } - fn visit_func_decl(&mut self, func_decl: &'a FuncDecl) { - if let DeclDef::Present(func_def_body) = &func_decl.def - && let Some(cfg) = &func_def_body.unstructured_cfg - { - for region in cfg.rev_post_order(func_def_body) { - if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { - for edge in &control_inst.targets { - match edge.target { - cf::unstructured::ControlTarget::Region(target) => { - *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; - } - cf::unstructured::ControlTarget::Return => {} - } - } + fn visit_node_def(&mut self, func_at_node: FuncAt<'a, Node>) { + if let DataInstKind::ThunkBind(target) = func_at_node.def().kind { + match target { + cf::unstructured::ControlTarget::Region(target) => { + *self.use_counts.entry(Use::RegionLabel(target)).or_default() += 1; } + cf::unstructured::ControlTarget::Return => {} } } - - func_decl.inner_visit_with(self); + func_at_node.inner_visit_with(self); } fn visit_value_use(&mut self, v: &'a Value) { @@ -1115,9 +1106,9 @@ impl<'a> Printer<'a> { || type_and_const_inputs.is_empty() } - TypeKind::QPtr | TypeKind::SpvStringLiteralForExtInst => { - true - } + TypeKind::QPtr + | TypeKind::Thunk + | TypeKind::SpvStringLiteralForExtInst => true, }; ty_def.attrs == AttrSet::default() @@ -3132,6 +3123,8 @@ impl Print for TypeDef { // FIXME(eddyb) should this be shortened to `qtr`? TypeKind::QPtr => printer.declarative_keyword_style().apply("qptr").into(), + TypeKind::Thunk => printer.imperative_keyword_style().apply("thunk").into(), + TypeKind::SpvInst { spv_inst, type_and_const_inputs } => printer .pretty_spv_inst( printer.spv_op_style(), @@ -3850,6 +3843,7 @@ impl Print for FuncAt<'_, Node> { DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => { // FIXME(eddyb) `outputs_header` is wastefully built even in @@ -4035,6 +4029,29 @@ impl FuncAt<'_, DataInst> { ]) } + &DataInstKind::ThunkBind(target) => pretty::Fragment::new([ + printer + .demote_style_for_namespace_prefix(printer.declarative_keyword_style()) + .apply("thunk.") + .into(), + printer.imperative_keyword_style().apply("bind").into(), + pretty::join_comma_sep( + "(", + [ + match target { + cf::unstructured::ControlTarget::Region(target) => { + Use::RegionLabel(target).print(printer) + } + cf::unstructured::ControlTarget::Return => { + printer.imperative_keyword_style().apply("return").into() + } + }, + pretty::join_comma_sep("(", inputs.iter().map(|v| v.print(printer)), ")"), + ], + ")", + ), + ]), + DataInstKind::SpvInst(inst) => printer.pretty_spv_inst( printer.spv_op_style(), inst.opcode, @@ -4261,35 +4278,14 @@ impl FuncAt<'_, DataInst> { impl Print for cf::unstructured::ControlInst { type Output = pretty::Fragment; fn print(&self, printer: &Printer<'_>) -> pretty::Fragment { - let Self { attrs, kind, inputs, targets } = self; + let Self { attrs, kind, inputs, target_thunks } = self; let attrs = attrs.print(printer); let kw_style = printer.imperative_keyword_style(); let kw = |kw| kw_style.apply(kw).into(); - let mut targets = - targets.iter().map(|cf::unstructured::ControlEdge { target, target_inputs }| { - let mut target = match *target { - cf::unstructured::ControlTarget::Region(target) => pretty::Fragment::new([ - kw("branch"), - " ".into(), - Use::RegionLabel(target).print(printer), - ]), - cf::unstructured::ControlTarget::Return => kw("return"), - }; - if !target_inputs.is_empty() { - target = pretty::Fragment::new([ - target, - pretty::join_comma_sep( - "(", - target_inputs.iter().map(|v| v.print(printer)), - ")", - ), - ]); - } - target - }); + let mut targets = target_thunks.iter().map(|v| v.print(printer)); let def = match kind { cf::unstructured::ControlInstKind::Unreachable => { diff --git a/src/qptr/lift.rs b/src/qptr/lift.rs index 890dbe57..48eba658 100644 --- a/src/qptr/lift.rs +++ b/src/qptr/lift.rs @@ -832,6 +832,17 @@ impl LiftToSpvPtrInstsInFunc<'_> { new_data_inst_def } + &DataInstKind::ThunkBind(_) => { + for &v in &data_inst_def.inputs { + if self.lifter.as_spv_ptr_type(type_of_val(v)).is_some() { + return Err(LiftError(Diag::bug([ + "unsupported `thunk.bind` with pointer inputs".into(), + ]))); + } + } + return Ok(Transformed::Unchanged); + } + DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => { let mut to_spv_ptr_input_adjustments = vec![]; let mut from_spv_ptr_output = None; diff --git a/src/qptr/lower.rs b/src/qptr/lower.rs index e2cddefb..01d893fb 100644 --- a/src/qptr/lower.rs +++ b/src/qptr/lower.rs @@ -637,7 +637,8 @@ impl LowerFromSpvPtrInstsInFunc<'_> { | NodeKind::ExitInvocation(_) | DataInstKind::FuncCall(_) | DataInstKind::Mem(_) - | DataInstKind::QPtr(_) => return, + | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) => return, DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => {} } diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 23c4cd97..7cceed6c 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -127,6 +127,11 @@ impl Visitor<'_> for NeedsIdsCollector<'_> { unreachable!("`TypeKind::QPtr` should be legalized away before lifting"); } + TypeKind::Thunk => { + // HACK(eddyb) unstructured control-flow uses thunks. + return; + } + TypeKind::SpvInst { .. } => {} TypeKind::SpvStringLiteralForExtInst => { unreachable!( @@ -145,6 +150,10 @@ impl Visitor<'_> for NeedsIdsCollector<'_> { } let ct_def = &self.cx[ct]; match ct_def.kind { + ConstKind::Undef if matches!(self.cx[ct_def.ty].kind, TypeKind::Thunk) => { + // HACK(eddyb) unstructured control-flow may use `undef` thunks. + } + ConstKind::Undef | ConstKind::PtrToGlobalVar(_) | ConstKind::PtrToFunc(_) @@ -218,6 +227,8 @@ impl Visitor<'_> for NeedsIdsCollector<'_> { match func_at_node.def().kind { NodeKind::Select(_) | NodeKind::Loop { .. } | NodeKind::ExitInvocation(_) => {} + DataInstKind::FuncCall(_) => {} + // FIXME(eddyb) this should be a proper `Result`-based error instead, // and/or `spv::lift` should mutate the module for legalization. DataInstKind::Mem(_) => { @@ -230,7 +241,7 @@ impl Visitor<'_> for NeedsIdsCollector<'_> { unreachable!("`DataInstKind::QPtr` should be legalized away before lifting"); } - DataInstKind::FuncCall(_) => {} + DataInstKind::ThunkBind(_) => {} DataInstKind::SpvInst(_) => {} DataInstKind::SpvExtInst { ext_set, .. } => { @@ -460,6 +471,7 @@ impl<'p> FuncAt<'_, CfgCursor<'p>> { DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => { Some(CfgCursor { point: CfgPoint::NodeExit(node), parent: cursor.parent }) @@ -667,9 +679,10 @@ impl<'a> FuncLifting<'a> { let insts = match point { CfgPoint::NodeEntry(node) => match func_def_body.at(node).def().kind { - NodeKind::Select(_) | NodeKind::Loop { .. } | NodeKind::ExitInvocation(_) => { - SmallVec::new() - } + NodeKind::Select(_) + | NodeKind::Loop { .. } + | NodeKind::ExitInvocation(_) + | DataInstKind::ThunkBind(_) => SmallVec::new(), DataInstKind::FuncCall(_) | DataInstKind::Mem(_) @@ -689,7 +702,7 @@ impl<'a> FuncLifting<'a> { .as_ref() .and_then(|cfg| cfg.control_inst_on_exit_from.get(region)); if let Some(terminator) = unstructured_terminator { - let cf::unstructured::ControlInst { attrs, kind, inputs, targets } = + let cf::unstructured::ControlInst { attrs, kind, inputs, target_thunks } = terminator; Terminator { attrs: *attrs, @@ -707,7 +720,7 @@ impl<'a> FuncLifting<'a> { // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. // NOTE(eddyb) this is also now used for returns. - targets: (0..u32::try_from(targets.len()).unwrap()) + targets: (0..u32::try_from(target_thunks.len()).unwrap()) .map(|edge_idx| CfgPoint::UnstructuredEdge { source: region, edge_idx, @@ -731,11 +744,25 @@ impl<'a> FuncLifting<'a> { } (CfgPoint::UnstructuredEdge { source, edge_idx }, None) => { let cfg = func_def_body.unstructured_cfg.as_ref().unwrap(); - let cf::unstructured::ControlInst { attrs, kind: _, inputs: _, targets } = + let cf::unstructured::ControlInst { attrs, kind: _, inputs: _, target_thunks } = &cfg.control_inst_on_exit_from[source]; - let cf::unstructured::ControlEdge { target, target_inputs } = - &targets[edge_idx as usize]; - match *target { + + // FIXME(eddyb) deduplicate with `ControlTarget::of_thunk`. + let func = func_def_body.at(()); + let (target, target_inputs) = match target_thunks[edge_idx as usize] { + Value::Var(thunk) => match func.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => { + let thunk_node_def = func.at(node).def(); + match thunk_node_def.kind { + NodeKind::ThunkBind(target) => (target, &thunk_node_def.inputs), + _ => unreachable!(), + } + } + _ => unreachable!(), + }, + Value::Const(_) => unreachable!(), + }; + match target { cf::unstructured::ControlTarget::Region(target) => Terminator { attrs: *attrs, kind: TerminatorKind::Branch, @@ -812,6 +839,7 @@ impl<'a> FuncLifting<'a> { DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => unreachable!(), } @@ -892,6 +920,7 @@ impl<'a> FuncLifting<'a> { | DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => unreachable!(), } @@ -934,7 +963,7 @@ impl<'a> FuncLifting<'a> { // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. // NOTE(eddyb) this is also now used for returns. - let edge_count = cfg.control_inst_on_exit_from[region].targets.len(); + let edge_count = cfg.control_inst_on_exit_from[region].target_thunks.len(); for edge_idx in 0..u32::try_from(edge_count).unwrap() { visit_cfg_point(CfgCursor { point: CfgPoint::UnstructuredEdge { source: region, edge_idx }, @@ -1300,7 +1329,9 @@ impl LazyInst<'_, '_> { }, // Not inserted into `globals` while visiting. - TypeKind::QPtr | TypeKind::SpvStringLiteralForExtInst => unreachable!(), + TypeKind::QPtr | TypeKind::Thunk | TypeKind::SpvStringLiteralForExtInst => { + unreachable!() + } }, Global::Const(ct) => { let ct_def = &cx[ct]; @@ -1431,7 +1462,7 @@ impl LazyInst<'_, '_> { unreachable!() } - DataInstKind::Mem(_) | DataInstKind::QPtr(_) => { + DataInstKind::Mem(_) | DataInstKind::QPtr(_) | DataInstKind::ThunkBind(_) => { // Disallowed while visiting. unreachable!() } diff --git a/src/spv/lower.rs b/src/spv/lower.rs index fae256d1..8548378b 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -1484,27 +1484,63 @@ impl Module { } } + let mut build_thunk = |target, target_inputs| { + let thunk_node = func_def_body.nodes.define( + &cx, + NodeDef { + attrs: AttrSet::default(), + kind: NodeKind::ThunkBind(target), + inputs: target_inputs, + child_regions: [].into_iter().collect(), + outputs: [].into_iter().collect(), + } + .into(), + ); + current_block_region_def + .children + .insert_last(thunk_node, &mut func_def_body.nodes); + + // FIXME(eddyb) cache this. + let thunk_ty = cx.intern(TypeKind::Thunk); + + let thunk_var = func_def_body.vars.define( + &cx, + VarDecl { + attrs: AttrSet::default(), + ty: thunk_ty, + + def_parent: Either::Right(thunk_node), + def_idx: 0, + }, + ); + func_def_body.nodes[thunk_node].outputs.push(thunk_var); + + Value::Var(thunk_var) + }; + // FIXME(eddyb) collect targets in this form to // begin with (instead of recombining them here). - let mut targets: SmallVec<[_; 4]> = targets + let mut target_thunks: SmallVec<[_; 4]> = targets .into_iter() - .map(|target| cf::unstructured::ControlEdge { - target: cf::unstructured::ControlTarget::Region(target), - target_inputs: target_inputs.get(&target).cloned().unwrap_or_default(), + .map(|target| { + build_thunk( + cf::unstructured::ControlTarget::Region(target), + target_inputs.get(&target).cloned().unwrap_or_default(), + ) }) .collect(); let kind = if opcode == wk.OpUnreachable { - assert!(targets.is_empty() && inputs.is_empty()); + assert!(target_thunks.is_empty() && inputs.is_empty()); cf::unstructured::ControlInstKind::Unreachable } else if [wk.OpReturn, wk.OpReturnValue].contains(&opcode) { - assert!(targets.is_empty() && inputs.len() <= 1); - targets.push(cf::unstructured::ControlEdge { - target: cf::unstructured::ControlTarget::Return, - target_inputs: mem::take(&mut inputs), - }); + assert!(target_thunks.is_empty() && inputs.len() <= 1); + target_thunks.push(build_thunk( + cf::unstructured::ControlTarget::Return, + mem::take(&mut inputs), + )); cf::unstructured::ControlInstKind::Branch - } else if targets.is_empty() { + } else if target_thunks.is_empty() { let node = func_def_body.nodes.define( &cx, NodeDef { @@ -1523,10 +1559,10 @@ impl Module { .insert_last(node, &mut func_def_body.nodes); cf::unstructured::ControlInstKind::Unreachable } else if opcode == wk.OpBranch { - assert_eq!((targets.len(), inputs.len()), (1, 0)); + assert_eq!((target_thunks.len(), inputs.len()), (1, 0)); cf::unstructured::ControlInstKind::Branch } else if opcode == wk.OpBranchConditional { - assert_eq!((targets.len(), inputs.len()), (2, 1)); + assert_eq!((target_thunks.len(), inputs.len()), (2, 1)); cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::BoolCond) } else if opcode == wk.OpSwitch { cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::SpvInst( @@ -1543,7 +1579,7 @@ impl Module { .control_inst_on_exit_from .insert( current_block.region, - cf::unstructured::ControlInst { attrs, kind, inputs, targets }, + cf::unstructured::ControlInst { attrs, kind, inputs, target_thunks }, ); } else if opcode == wk.OpPhi { if !current_block_region_def.children.is_empty() { diff --git a/src/transform.rs b/src/transform.rs index 8921c2cc..292e9cff 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -438,7 +438,7 @@ impl InnerTransform for TypeDef { transform!({ attrs -> transformer.transform_attr_set_use(*attrs), kind -> match kind { - TypeKind::QPtr | TypeKind::SpvStringLiteralForExtInst => Transformed::Unchanged, + TypeKind::QPtr | TypeKind::Thunk | TypeKind::SpvStringLiteralForExtInst => Transformed::Unchanged, TypeKind::SpvInst { spv_inst, type_and_const_inputs } => Transformed::map_iter( type_and_const_inputs.iter(), @@ -641,6 +641,7 @@ impl InnerInPlaceTransform for FuncAtMut<'_, Node> { | QPtrOp::Offset(_) | QPtrOp::DynOffset { .. }, ) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => {} } @@ -681,7 +682,7 @@ impl InnerInPlaceTransform for VarDecl { impl InnerInPlaceTransform for cf::unstructured::ControlInst { fn inner_in_place_transform_with(&mut self, transformer: &mut impl Transformer) { - let Self { attrs, kind, inputs, targets } = self; + let Self { attrs, kind, inputs, target_thunks } = self; transformer.transform_attr_set_use(*attrs).apply_to(attrs); match kind { @@ -694,10 +695,8 @@ impl InnerInPlaceTransform for cf::unstructured::ControlInst { for v in inputs { transformer.transform_value_use(v).apply_to(v); } - for cf::unstructured::ControlEdge { target: _, target_inputs } in targets { - for v in target_inputs { - transformer.transform_value_use(v).apply_to(v); - } + for v in target_thunks { + transformer.transform_value_use(v).apply_to(v); } } } diff --git a/src/visit.rs b/src/visit.rs index bc54d09d..a03433a3 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -321,7 +321,7 @@ impl InnerVisit for TypeDef { visitor.visit_attr_set_use(*attrs); match kind { - TypeKind::QPtr | TypeKind::SpvStringLiteralForExtInst => {} + TypeKind::QPtr | TypeKind::Thunk | TypeKind::SpvStringLiteralForExtInst => {} TypeKind::SpvInst { spv_inst: _, type_and_const_inputs } => { for &ty_or_ct in type_and_const_inputs { @@ -483,6 +483,7 @@ impl<'a> FuncAt<'a, Node> { | QPtrOp::Offset(_) | QPtrOp::DynOffset { .. }, ) + | DataInstKind::ThunkBind(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => {} } @@ -515,7 +516,7 @@ impl InnerVisit for VarDecl { impl InnerVisit for cf::unstructured::ControlInst { fn inner_visit_with<'a>(&'a self, visitor: &mut impl Visitor<'a>) { - let Self { attrs, kind, inputs, targets } = self; + let Self { attrs, kind, inputs, target_thunks } = self; visitor.visit_attr_set_use(*attrs); match kind { @@ -528,10 +529,8 @@ impl InnerVisit for cf::unstructured::ControlInst { for v in inputs { visitor.visit_value_use(v); } - for cf::unstructured::ControlEdge { target: _, target_inputs } in targets { - for v in target_inputs { - visitor.visit_value_use(v); - } + for v in target_thunks { + visitor.visit_value_use(v); } } } From ec57e04f6232fb93ed5c0abbb89bcab40ed91689 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 28 Nov 2025 11:41:43 +0200 Subject: [PATCH 5/7] cf: replace `ControlInstKind::Unreachable` with branches to `undef` thunks. --- src/cf/structurize.rs | 76 ++++++++++++++++-------------------------- src/cf/unstructured.rs | 75 +++++++++++++++++++++++------------------ src/print/mod.rs | 7 ---- src/spv/lift.rs | 51 ++++++++++++++++------------ src/spv/lower.rs | 22 +++++++++--- src/transform.rs | 3 +- src/visit.rs | 3 +- 7 files changed, 120 insertions(+), 117 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 3e40da91..c5429679 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -589,8 +589,9 @@ impl<'a> Structurizer<'a> { .unstructured_cfg .as_ref() .map(|cfg| { - let loop_header_to_exit_targets = LoopFinder::new(func_def_body.at(()), cfg) - .find_all_loops_starting_at(func_def_body.body); + let loop_header_to_exit_targets = + LoopFinder::new(cx, func_def_body.at(()), cfg) + .find_all_loops_starting_at(func_def_body.body); let mut state = TraversalState { incoming_edge_counts: EntityOrientedDenseMap::new(), @@ -1088,7 +1089,10 @@ impl<'a> Structurizer<'a> { } _ => unreachable!(), }, - Value::Const(_) => unreachable!(), + Value::Const(ct) => match self.cx[ct].kind { + ConstKind::Undef => return Err(DeferredEdgeBundleSet::Unreachable), + _ => unreachable!(), + }, }; let target = match target { @@ -1145,33 +1149,10 @@ impl<'a> Structurizer<'a> { .collect(); match kind { - ControlInstKind::Unreachable => { + ControlInstKind::Branch => { // FIXME(eddyb) this loses `attrs`. let _ = attrs; - assert_eq!((inputs.len(), target_regions.len()), (0, 0)); - - // FIXME(eddyb) this may result in lost optimizations over - // actually encoding it in `Node`/`Region` - // (e.g. a new `NodeKind`, or replacing region `outputs`), - // but it's simpler to handle it like this. - // - // NOTE(eddyb) actually, this encoding is lossless *during* - // structurization, and a divergent region can only end up as: - // - the function body, where it implies the function can - // never actually return: not fully structurized currently - // (but only for a silly reason, and is entirely fixable) - // - a `Select` case, where it implies that case never merges - // back into the `Select` node, and potentially that the - // case can never be taken: this is where a structured - // encoding can be introduced, by pruning unreachable - // cases, and potentially even introducing `assume`s - // - a `Loop` body is not actually possible when divergent - // (as there can be no backedge to form a cyclic CFG) - DeferredEdgeBundleSet::Unreachable - } - - ControlInstKind::Branch => { assert_eq!(inputs.len(), 0); self.append_maybe_claimed_region( @@ -1834,7 +1815,7 @@ impl<'a> Structurizer<'a> { let cx = self.cx; let thunk_ty = cx.intern(TypeKind::Thunk); - let build_thunk = |func_at_region: FuncAtMut<'_, Region>, target, target_inputs| { + let build_thunk = |func_at_region: FuncAtMut<'_, Region>, (target, target_inputs)| { let region = func_at_region.position; let func = func_at_region.at(()); @@ -1878,16 +1859,13 @@ impl<'a> Structurizer<'a> { (taken_then, deferred_edges) = deferred_edges.split_out_matching(|deferred| { Ok(( deferred.condition, - build_thunk( - self.func_def_body.at_mut(control_source.unwrap()), - deferred.edge_bundle.target, - deferred.edge_bundle.target_inputs, - ), + (deferred.edge_bundle.target, deferred.edge_bundle.target_inputs), )) }); let Some((condition, then_edge)) = taken_then else { break; }; + let branch_source = control_source.take().unwrap(); let else_edge = match deferred_edges { // At most one deferral left, so it can be used as the "else" @@ -1895,11 +1873,7 @@ impl<'a> Structurizer<'a> { DeferredEdgeBundleSet::Unreachable => None, DeferredEdgeBundleSet::Always { target: else_target, edge_bundle } => { deferred_edges = DeferredEdgeBundleSet::Unreachable; - Some(build_thunk( - self.func_def_body.at_mut(branch_source), - else_target, - edge_bundle.target_inputs, - )) + Some((else_target, edge_bundle.target_inputs)) } // More branches are needed, so the "else" case must be a `Region` @@ -1908,11 +1882,7 @@ impl<'a> Structurizer<'a> { let new_empty_region = self.func_def_body.regions.define(cx, RegionDef::default()); control_source = Some(new_empty_region); - Some(build_thunk( - self.func_def_body.at_mut(branch_source), - DeferredTarget::Region(new_empty_region), - [].into_iter().collect(), - )) + Some((DeferredTarget::Region(new_empty_region), [].into_iter().collect())) } }; @@ -1927,7 +1897,11 @@ impl<'a> Structurizer<'a> { ControlInstKind::Branch }, inputs: condition.into_iter().collect(), - target_thunks: [then_edge].into_iter().chain(else_edge).collect(), + target_thunks: [then_edge] + .into_iter() + .chain(else_edge) + .map(|edge| build_thunk(self.func_def_body.at_mut(branch_source), edge)) + .collect(), }; assert!( self.func_def_body @@ -1946,13 +1920,21 @@ impl<'a> Structurizer<'a> { None => return, }; - // Final deferral is an `Unreachable` (only when truly divergent, + // Final deferral is unreachable (only when truly divergent, // i.e. no `deferred_edges`). + // FIXME(eddyb) this should probably be special-cased at the start of + // this function, instead of here at the end. let final_control_inst = ControlInst { attrs: AttrSet::default(), - kind: ControlInstKind::Unreachable, + kind: ControlInstKind::Branch, inputs: [].into_iter().collect(), - target_thunks: [].into_iter().collect(), + target_thunks: [Value::Const(cx.intern(ConstDef { + attrs: AttrSet::default(), + ty: thunk_ty, + kind: ConstKind::Undef, + }))] + .into_iter() + .collect(), }; assert!( self.func_def_body diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index b4c3cb9b..9afcee91 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -1,8 +1,8 @@ //! Unstructured control-flow graph (CFG) abstractions and utilities. use crate::{ - AttrSet, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, FxIndexSet, NodeKind, Region, Value, - VarKind, cf, + AttrSet, ConstKind, Context, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, FxIndexSet, + NodeKind, Region, Value, Var, VarKind, cf, }; use itertools::Either; use smallvec::SmallVec; @@ -34,16 +34,6 @@ pub struct ControlInst { #[derive(Clone)] pub enum ControlInstKind { - /// Reaching this point in the control-flow is undefined behavior, e.g.: - /// * a `SelectBranch` case that's known to be impossible - /// * after a function call, where the function never returns - /// - /// Optimizations can take advantage of this information, to assume that any - /// necessary preconditions for reaching this point, are never met. - // - // FIXME(eddyb) turn this into an `undef` thunk. - Unreachable, - /// Unconditional branch to a single target. Branch, @@ -62,23 +52,34 @@ pub enum ControlTarget { Return, } +// HACK(eddyb) marker type for an `undef` thunk. +struct Unreachable; + impl ControlTarget { // HACK(eddyb) this isn't necessarily the best place, but allows reuse. // FIXME(eddyb) properly distinguish the different failure cases possible, // maybe even avoid panicking entirely? - fn of_thunk(func_at_thunk: FuncAt<'_, Value>) -> Self { - let thunk = func_at_thunk.position; - let func = func_at_thunk.at(()); + fn of_thunk(cx: &Context, func_at_thunk: FuncAt<'_, Value>) -> Result { + match func_at_thunk.position { + Value::Var(thunk) => Ok(ControlTarget::of_thunk_var(func_at_thunk.at(thunk))), + Value::Const(ct) => match cx[ct].kind { + ConstKind::Undef => Err(Unreachable), + _ => unreachable!(), + }, + } + } - match thunk { - Value::Var(thunk) => match func.at(thunk).decl().kind() { - VarKind::NodeOutput { node, output_idx: 0 } => match func.at(node).def().kind { + // HACK(eddyb) this is the `Value::Var` case from `of_thunk`, separated so + // that traversal doesn't need to have access to the `Context`. + fn of_thunk_var(func_at_thunk: FuncAt<'_, Var>) -> Self { + match func_at_thunk.decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => { + match func_at_thunk.at(node).def().kind { NodeKind::ThunkBind(target) => target, _ => unreachable!(), - }, - _ => unreachable!(), - }, - Value::Const(_) => unreachable!(), + } + } + _ => unreachable!(), } } } @@ -188,11 +189,12 @@ impl ControlFlowGraph { .get(region) .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); - let targets = control_inst.target_thunks.iter().filter_map(|&thunk| { - match ControlTarget::of_thunk(func.at(thunk)) { + let targets = control_inst.target_thunks.iter().filter_map(|&thunk| match thunk { + Value::Var(thunk) => match ControlTarget::of_thunk_var(func.at(thunk)) { ControlTarget::Region(target) => Some(target), ControlTarget::Return => None, - } + }, + Value::Const(_) => None, }); let targets = if state.reverse_targets { Either::Left(targets.rev()) @@ -232,6 +234,7 @@ impl ControlFlowGraph { /// be misleading with explicit `break`s (moving user code from just before /// the `break` to after the loop), but is less impactful than "maximal loops" pub struct LoopFinder<'a> { + cx: &'a Context, func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph, @@ -297,8 +300,9 @@ impl std::ops::BitOrAssign for EventualCfgExits { } impl<'a> LoopFinder<'a> { - pub fn new(func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph) -> Self { + pub fn new(cx: &'a Context, func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph) -> Self { Self { + cx, func, cfg, loop_header_to_exit_targets: FxIndexMap::default(), @@ -360,7 +364,10 @@ impl<'a> LoopFinder<'a> { let mut eventual_cfg_exits = EventualCfgExits { may_return_from_func: control_inst.target_thunks.iter().any(|&thunk| { - matches!(ControlTarget::of_thunk(self.func.at(thunk)), ControlTarget::Return) + matches!( + ControlTarget::of_thunk(self.cx, self.func.at(thunk)), + Ok(ControlTarget::Return) + ) }), }; @@ -368,9 +375,11 @@ impl<'a> LoopFinder<'a> { .target_thunks .iter() .flat_map(|&thunk| { - let target = match ControlTarget::of_thunk(self.func.at(thunk)) { - ControlTarget::Region(target) => target, - ControlTarget::Return => return None.into_iter().chain(None), + let target = match ControlTarget::of_thunk(self.cx, self.func.at(thunk)) { + Ok(ControlTarget::Region(target)) => target, + Ok(ControlTarget::Return) | Err(Unreachable) => { + return None.into_iter().chain(None); + } }; let (earliest_scc_root_of_target, eventual_cfg_exits_of_target) = @@ -430,9 +439,9 @@ impl<'a> LoopFinder<'a> { .target_thunks .iter() .filter_map(|&thunk| { - match ControlTarget::of_thunk(self.func.at(thunk)) { - ControlTarget::Region(target) => Some(target), - ControlTarget::Return => None, + match ControlTarget::of_thunk(self.cx, self.func.at(thunk)) { + Ok(ControlTarget::Region(target)) => Some(target), + Ok(ControlTarget::Return) | Err(Unreachable) => None, } }) }) diff --git a/src/print/mod.rs b/src/print/mod.rs index ecd96e49..993e1174 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -4283,17 +4283,10 @@ impl Print for cf::unstructured::ControlInst { let attrs = attrs.print(printer); let kw_style = printer.imperative_keyword_style(); - let kw = |kw| kw_style.apply(kw).into(); let mut targets = target_thunks.iter().map(|v| v.print(printer)); let def = match kind { - cf::unstructured::ControlInstKind::Unreachable => { - // FIXME(eddyb) use `targets.is_empty()` when that is stabilized. - assert!(targets.len() == 0 && inputs.is_empty()); - kw("unreachable") - } - cf::unstructured::ControlInstKind::Branch => { assert_eq!((targets.len(), inputs.len()), (1, 0)); targets.next().unwrap() diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 7cceed6c..60051244 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -707,9 +707,6 @@ impl<'a> FuncLifting<'a> { Terminator { attrs: *attrs, kind: match kind { - cf::unstructured::ControlInstKind::Unreachable => { - TerminatorKind::Unreachable - } cf::unstructured::ControlInstKind::Branch => TerminatorKind::Branch, cf::unstructured::ControlInstKind::SelectBranch(kind) => { TerminatorKind::SelectBranch(kind) @@ -747,6 +744,9 @@ impl<'a> FuncLifting<'a> { let cf::unstructured::ControlInst { attrs, kind: _, inputs: _, target_thunks } = &cfg.control_inst_on_exit_from[source]; + // HACK(eddyb) marker type for an `undef` thunk. + struct Unreachable; + // FIXME(eddyb) deduplicate with `ControlTarget::of_thunk`. let func = func_def_body.at(()); let (target, target_inputs) = match target_thunks[edge_idx as usize] { @@ -754,37 +754,46 @@ impl<'a> FuncLifting<'a> { VarKind::NodeOutput { node, output_idx: 0 } => { let thunk_node_def = func.at(node).def(); match thunk_node_def.kind { - NodeKind::ThunkBind(target) => (target, &thunk_node_def.inputs), + NodeKind::ThunkBind(target) => { + (Ok(target), &thunk_node_def.inputs[..]) + } _ => unreachable!(), } } _ => unreachable!(), }, - Value::Const(_) => unreachable!(), + Value::Const(ct) => match cx[ct].kind { + ConstKind::Undef => (Err(Unreachable), &[][..]), + _ => unreachable!(), + }, + }; + let target = match target { + Ok(cf::unstructured::ControlTarget::Region(target)) => Ok(target), + Ok(cf::unstructured::ControlTarget::Return) => Err(TerminatorKind::Return), + Err(Unreachable) => Err(TerminatorKind::Unreachable), }; match target { - cf::unstructured::ControlTarget::Region(target) => Terminator { + Ok(target) => Terminator { attrs: *attrs, kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [CfgPoint::RegionEntry(target)].into_iter().collect(), - target_phi_values: [( - CfgPoint::RegionEntry(target), - &target_inputs[..], - )] - .into_iter() - .collect(), - merge: None, - }, - cf::unstructured::ControlTarget::Return => Terminator { - attrs: *attrs, - kind: TerminatorKind::Return, - // FIXME(eddyb) borrow these whenever possible. - inputs: target_inputs.clone(), - targets: [].into_iter().collect(), - target_phi_values: FxIndexMap::default(), + target_phi_values: [(CfgPoint::RegionEntry(target), target_inputs)] + .into_iter() + .collect(), merge: None, }, + Err(terminator_kind) => { + Terminator { + attrs: *attrs, + kind: terminator_kind, + // FIXME(eddyb) borrow these whenever possible. + inputs: target_inputs.iter().copied().collect(), + targets: [].into_iter().collect(), + target_phi_values: FxIndexMap::default(), + merge: None, + } + } } } diff --git a/src/spv/lower.rs b/src/spv/lower.rs index 8548378b..160b8fa2 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -1484,6 +1484,9 @@ impl Module { } } + // FIXME(eddyb) cache this. + let thunk_ty = cx.intern(TypeKind::Thunk); + let mut build_thunk = |target, target_inputs| { let thunk_node = func_def_body.nodes.define( &cx, @@ -1500,9 +1503,6 @@ impl Module { .children .insert_last(thunk_node, &mut func_def_body.nodes); - // FIXME(eddyb) cache this. - let thunk_ty = cx.intern(TypeKind::Thunk); - let thunk_var = func_def_body.vars.define( &cx, VarDecl { @@ -1532,7 +1532,13 @@ impl Module { let kind = if opcode == wk.OpUnreachable { assert!(target_thunks.is_empty() && inputs.is_empty()); - cf::unstructured::ControlInstKind::Unreachable + // FIXME(eddyb) cache this. + target_thunks.push(Value::Const(cx.intern(ConstDef { + attrs: AttrSet::default(), + ty: thunk_ty, + kind: ConstKind::Undef, + }))); + cf::unstructured::ControlInstKind::Branch } else if [wk.OpReturn, wk.OpReturnValue].contains(&opcode) { assert!(target_thunks.is_empty() && inputs.len() <= 1); target_thunks.push(build_thunk( @@ -1557,7 +1563,13 @@ impl Module { current_block_region_def .children .insert_last(node, &mut func_def_body.nodes); - cf::unstructured::ControlInstKind::Unreachable + // FIXME(eddyb) cache this. + target_thunks.push(Value::Const(cx.intern(ConstDef { + attrs: AttrSet::default(), + ty: thunk_ty, + kind: ConstKind::Undef, + }))); + cf::unstructured::ControlInstKind::Branch } else if opcode == wk.OpBranch { assert_eq!((target_thunks.len(), inputs.len()), (1, 0)); cf::unstructured::ControlInstKind::Branch diff --git a/src/transform.rs b/src/transform.rs index 292e9cff..c73b7980 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -686,8 +686,7 @@ impl InnerInPlaceTransform for cf::unstructured::ControlInst { transformer.transform_attr_set_use(*attrs).apply_to(attrs); match kind { - cf::unstructured::ControlInstKind::Unreachable - | cf::unstructured::ControlInstKind::Branch + cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), ) => {} diff --git a/src/visit.rs b/src/visit.rs index a03433a3..5907e468 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -520,8 +520,7 @@ impl InnerVisit for cf::unstructured::ControlInst { visitor.visit_attr_set_use(*attrs); match kind { - cf::unstructured::ControlInstKind::Unreachable - | cf::unstructured::ControlInstKind::Branch + cf::unstructured::ControlInstKind::Branch | cf::unstructured::ControlInstKind::SelectBranch( SelectionKind::BoolCond | SelectionKind::SpvInst(_), ) => {} From adac644ad346f1aa38424c0337340eb25174805f Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Thu, 4 Dec 2025 19:51:26 +0200 Subject: [PATCH 6/7] cf: replace `ControlInstKind::SelectBranch` with `thunk`-valued `Select`. --- src/cf/structurize.rs | 324 ++++++++++++++++++++++++----------------- src/cf/unstructured.rs | 304 ++++++++++++++++++++++++++------------ src/lib.rs | 8 +- src/print/mod.rs | 38 +---- src/spv/lift.rs | 153 ++++++++++--------- src/spv/lower.rs | 188 ++++++++++++++---------- src/transform.rs | 24 +-- src/visit.rs | 24 +-- 8 files changed, 612 insertions(+), 451 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index c5429679..27073dbe 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -3,9 +3,7 @@ // FIXME(eddyb) consider moving docs to the module level? use crate::cf::SelectionKind; -use crate::cf::unstructured::{ - ControlInst, ControlInstKind, ControlTarget, IncomingEdgeCount, LoopFinder, TraversalState, -}; +use crate::cf::unstructured::{ControlTarget, IncomingEdgeCount, LoopFinder, TraversalState}; use crate::func_at::FuncAtMut; use crate::transform::{InnerInPlaceTransform as _, Transformed, Transformer}; use crate::{ @@ -146,10 +144,10 @@ enum StructurizeRegionState { /// **Note**: `target` has a generic type `T` to reduce redundancy when it's /// already implied (e.g. by the key in [`DeferredEdgeBundleSet`]'s map). struct IncomingEdgeBundle { - /// Attributes from the original [`ControlInst`]s (likely debuginfo), kept - /// when merging only when exactly identical, which can naturally be the case - /// for debuginfo (e.g. for branches from inside `if`-`else`/`switch` to a - /// common merge point, just after the whole control-flow construct). + /// Attributes from the original `thunk`s (likely debuginfo), kept when + /// merging only when exactly identical, which can naturally be the case + /// for debuginfo (e.g. for branches from inside `if`-`else`/`switch` to + /// a common merge point, just after the whole control-flow construct). // // FIXME(eddyb) semantically filter these, maybe focus on debuginfo? attrs: AttrSet, @@ -885,7 +883,7 @@ impl<'a> Structurizer<'a> { self.cx, NodeDef { // FIXME(eddyb) could it be possible to synthesize attrs - // from `ControlInst`s' attrs and/or `OpLoopMerge`'s? + // from thunks' attrs and/or `OpLoopMerge`'s? attrs: AttrSet::default(), kind: NodeKind::Loop { // TODO(eddyb) make this a regular body output (first one?). @@ -1044,98 +1042,133 @@ impl<'a> Structurizer<'a> { } } - let control_inst_on_exit = self - .func_def_body - .unstructured_cfg - .as_mut() - .unwrap() - .control_inst_on_exit_from - .remove(region) - .expect( - "cfg::Structurizer::structurize_region: missing \ - `ControlInst` (CFG wasn't unstructured in the first place?)", + // FIXME(eddyb) `ControlFlowGraph::edges_from_thunk_tailed_region` + // overlaps a lot with this, but is only traversing, not "stealing", + // so there might not be an easy way to deduplicate between the two. + // HACK(eddyb) only not a method because of its very limited usability + // (e.g. it modifies the function in such a way to guarantee that being + // called twice with the same arguments would result in panics). + // + // FIXME(eddyb) consider keeping `thunk`s in, initially, plumbing them + // in the same way that the "deferred edge" conditions and target inputs + // are today, but instead of generating `if`-`else` on conditions, using + // some kind of "`thunk` `switch`" node (handling only claimed regions), + // which would leave behind the subset of unhandled cases, merged with + // new potential destinations from within the handled cases, and doing + // this "unfolding" of the CFG would be the main point of structurization, + // with actual encodings for the remaining `thunk`s being implemented + // as a separate step (with optimizations such as integers replacing + // the "one-hot" `bool` condition encoding, reusing output slots with + // the same type - or even the same bit width - between targets, etc.). + fn steal_tail_thunk_from_region( + this: &mut Structurizer<'_>, + edge_source_region: Region, + thunk_binding_region: Region, + ) -> Result { + let thunk = if edge_source_region == thunk_binding_region { + // FIXME(eddyb) make the thunk an actual region output instead. + this.func_def_body + .unstructured_cfg + .as_mut() + .unwrap() + .target_thunk_on_exit_from + .remove(edge_source_region) + .expect( + // FIXME(eddyb) this only really makes sense + "cfg::Structurizer::structurize_region: missing \ + `target_thunk_on_exit_from` \ + (CFG wasn't unstructured in the first place?)", + ) + } else { + mem::take(&mut this.func_def_body.at_mut(thunk_binding_region).def().outputs) + .into_iter() + .exactly_one() + .ok() + .unwrap() + }; + + let thunk_node = match thunk { + Value::Var(thunk) => match this.func_def_body.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => node, + _ => unreachable!(), + }, + Value::Const(ct) => match this.cx[ct].kind { + ConstKind::Undef => return Err(DeferredEdgeBundleSet::Unreachable), + _ => unreachable!(), + }, + }; + + let thunk_node_def = mem::replace( + this.func_def_body.at_mut(thunk_node).def(), + // HACK(eddyb) there isn't a good "tombstone" to use here, + // but really the only thing we care about is no + // heap allocations remain around. + // FIXME(eddyb) come up with a better "nop" or add first-class + // tombstones (maybe with generational entity refs?). + NodeDef { + attrs: Default::default(), + kind: NodeKind::Select(SelectionKind::BoolCond), + inputs: Default::default(), + child_regions: Default::default(), + outputs: Default::default(), + }, ); - // Start with the concatenation of `region` and `control_inst_on_exit`, - // always appending `Node`s (including the children of entire - // `ClaimedRegion`s) to `region`'s definition itself. - let mut deferred_edges = { - let ControlInst { attrs, kind, inputs, target_thunks } = control_inst_on_exit; + assert!( + Value::Var(thunk_node_def.outputs.into_iter().exactly_one().ok().unwrap()) == thunk + ); - let target_regions: SmallVec<[_; 8]> = target_thunks - .iter() - .map(|&thunk| { - let (target, target_inputs) = match thunk { - Value::Var(thunk) => match self.func_def_body.at(thunk).decl().kind() { - VarKind::NodeOutput { node, output_idx: 0 } => { - let thunk_node_def = self.func_def_body.at_mut(node).def(); - let target = match thunk_node_def.kind { - NodeKind::ThunkBind(target) => target, - _ => unreachable!(), - }; - - // NOTE(eddyb) this can only occur if the thunk - // is used more than once (which is illegal). - assert!(thunk_node_def.outputs[..] == [thunk]); - thunk_node_def.outputs.clear(); - - let target_inputs = mem::take(&mut thunk_node_def.inputs); - - self.func_def_body.regions[region] - .children - .remove(node, &mut self.func_def_body.nodes); - - (target, target_inputs) - } - _ => unreachable!(), - }, - Value::Const(ct) => match self.cx[ct].kind { - ConstKind::Undef => return Err(DeferredEdgeBundleSet::Unreachable), - _ => unreachable!(), - }, - }; + let thunk_binding_def = &mut this.func_def_body.regions[thunk_binding_region]; + { + let thunk_binding_region_children = thunk_binding_def.children.iter(); + assert!(thunk_binding_region_children.last == Some(thunk_node)); + if edge_source_region != thunk_binding_region { + assert!( + thunk_binding_region_children.first == thunk_binding_region_children.last + ); + } + } + thunk_binding_def.children.remove(thunk_node, &mut this.func_def_body.nodes); + match thunk_node_def.kind { + NodeKind::ThunkBind(target) => { let target = match target { ControlTarget::Region(target) => target, ControlTarget::Return => { return Err(DeferredEdgeBundleSet::Always { target: DeferredTarget::Return, edge_bundle: IncomingEdgeBundle { - attrs, + attrs: thunk_node_def.attrs, accumulated_count: IncomingEdgeCount::default(), target: (), - target_inputs, + target_inputs: thunk_node_def.inputs, }, }); } }; - self.try_claim_edge_bundle(IncomingEdgeBundle { - attrs: if target_thunks.len() == 1 { attrs } else { AttrSet::default() }, + this.try_claim_edge_bundle(IncomingEdgeBundle { + attrs: thunk_node_def.attrs, target, accumulated_count: IncomingEdgeCount::ONE, - target_inputs, + target_inputs: thunk_node_def.inputs, }) .map_err(|edge_bundle| { // HACK(eddyb) special-case "shared `unreachable`" to // always inline it and avoid awkward "merges". // FIXME(eddyb) should this be in a separate CFG pass? // (i.e. is there a risk of other logic needing this?) - let target_is_trivial_unreachable = - match self.structurize_region_state.get(&edge_bundle.target) { - Some(StructurizeRegionState::Ready { - region_deferred_edges: DeferredEdgeBundleSet::Unreachable, - .. - }) => { - // FIXME(eddyb) DRY this "is empty region" check. - self.func_def_body - .at(edge_bundle.target) - .at_children() - .into_iter() - .next() - .is_none() - } - _ => false, - }; + let target_is_trivial_unreachable = match this + .structurize_region_state + .get(&edge_bundle.target) + { + Some(StructurizeRegionState::Ready { + region_deferred_edges: DeferredEdgeBundleSet::Unreachable, + .. + }) => { + this.func_def_body.at(edge_bundle.target).def().children.is_empty() + } + _ => false, + }; if target_is_trivial_unreachable { DeferredEdgeBundleSet::Unreachable } else { @@ -1145,30 +1178,39 @@ impl<'a> Structurizer<'a> { } } }) - }) - .collect(); - - match kind { - ControlInstKind::Branch => { - // FIXME(eddyb) this loses `attrs`. - let _ = attrs; - - assert_eq!(inputs.len(), 0); - - self.append_maybe_claimed_region( - region, - target_regions.into_iter().exactly_one().ok().unwrap(), - ) } - - ControlInstKind::SelectBranch(kind) => { - assert_eq!(inputs.len(), 1); - - let scrutinee = inputs[0]; - - self.structurize_select_into(region, attrs, kind, Ok(scrutinee), target_regions) + // FIXME(eddyb) try to reuse the existing node and regions, + // which housed the individual thunks for CFG edges, in the + // final structured control-flow. + NodeKind::Select(kind) => { + assert!(edge_source_region == thunk_binding_region); + + let scrutinee = thunk_node_def.inputs.into_iter().exactly_one().ok().unwrap(); + + let cases = thunk_node_def + .child_regions + .into_iter() + .map(|case| steal_tail_thunk_from_region(this, edge_source_region, case)) + .collect(); + + Err(this.structurize_select_into( + edge_source_region, + thunk_node_def.attrs, + kind, + Ok(scrutinee), + cases, + )) } + _ => unreachable!(), } + } + + // Start with the concatenation of `region` and its unstructured thunk, + // always appending `Node`s (including the children of entire + // `ClaimedRegion`s) to `region`'s definition itself. + let mut deferred_edges = { + let tail_thunk = steal_tail_thunk_from_region(self, region, region); + self.append_maybe_claimed_region(region, tail_thunk) }; // Try to resolve deferred edges that may have accumulated, and keep @@ -1795,8 +1837,8 @@ impl<'a> Structurizer<'a> { } /// When structurization is only partial, and there remain unclaimed regions, - /// they have to be reintegrated into the CFG, putting back [`ControlInst`]s - /// where `structurize_region` has taken them from. + /// they have to be reintegrated into the CFG, putting back `thunk`s where + /// `structurize_region` has taken them from. /// /// This function handles one region at a time to make it more manageable, /// despite it having a single call site (in a loop in `structurize_func`). @@ -1877,7 +1919,7 @@ impl<'a> Structurizer<'a> { } // More branches are needed, so the "else" case must be a `Region` - // that itself can have a `ControlInst` attached to it later on. + // that itself can have a `thunk` attached to it later on. DeferredEdgeBundleSet::Choice { .. } => { let new_empty_region = self.func_def_body.regions.define(cx, RegionDef::default()); @@ -1886,30 +1928,59 @@ impl<'a> Structurizer<'a> { } }; - let condition = Some(condition) - .filter(|_| else_edge.is_some()) - .map(|cond| self.materialize_lazy_cond(&cond)); - let branch_control_inst = ControlInst { - attrs: AttrSet::default(), - kind: if condition.is_some() { - ControlInstKind::SelectBranch(SelectionKind::BoolCond) - } else { - ControlInstKind::Branch - }, - inputs: condition.into_iter().collect(), - target_thunks: [then_edge] + let thunk = if let Some(else_edge) = else_edge { + let condition = self.materialize_lazy_cond(&condition); + + let cases = [then_edge, else_edge] .into_iter() - .chain(else_edge) - .map(|edge| build_thunk(self.func_def_body.at_mut(branch_source), edge)) - .collect(), + .map(|target_with_inputs| { + let case = self.func_def_body.regions.define(cx, RegionDef::default()); + let thunk = + build_thunk(self.func_def_body.at_mut(case), target_with_inputs); + self.func_def_body.regions[case].outputs.push(thunk); + case + }) + .collect(); + + let select_node = self.func_def_body.nodes.define( + cx, + NodeDef { + attrs: AttrSet::default(), + kind: NodeKind::Select(SelectionKind::BoolCond), + inputs: [condition].into_iter().collect(), + child_regions: cases, + outputs: [].into_iter().collect(), + } + .into(), + ); + self.func_def_body.regions[branch_source] + .children + .insert_last(select_node, &mut self.func_def_body.nodes); + + let select_thunk_var = self.func_def_body.vars.define( + cx, + VarDecl { + attrs: AttrSet::default(), + ty: thunk_ty, + + def_parent: Either::Right(select_node), + def_idx: 0, + }, + ); + self.func_def_body.nodes[select_node].outputs.push(select_thunk_var); + + Value::Var(select_thunk_var) + } else { + build_thunk(self.func_def_body.at_mut(branch_source), then_edge) }; + assert!( self.func_def_body .unstructured_cfg .as_mut() .unwrap() - .control_inst_on_exit_from - .insert(branch_source, branch_control_inst) + .target_thunk_on_exit_from + .insert(branch_source, thunk) .is_none() ); } @@ -1924,25 +1995,18 @@ impl<'a> Structurizer<'a> { // i.e. no `deferred_edges`). // FIXME(eddyb) this should probably be special-cased at the start of // this function, instead of here at the end. - let final_control_inst = ControlInst { + let final_thunk = Value::Const(cx.intern(ConstDef { attrs: AttrSet::default(), - kind: ControlInstKind::Branch, - inputs: [].into_iter().collect(), - target_thunks: [Value::Const(cx.intern(ConstDef { - attrs: AttrSet::default(), - ty: thunk_ty, - kind: ConstKind::Undef, - }))] - .into_iter() - .collect(), - }; + ty: thunk_ty, + kind: ConstKind::Undef, + })); assert!( self.func_def_body .unstructured_cfg .as_mut() .unwrap() - .control_inst_on_exit_from - .insert(final_source, final_control_inst) + .target_thunk_on_exit_from + .insert(final_source, final_thunk) .is_none() ); } diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index 9afcee91..bd3efdfb 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -1,46 +1,61 @@ //! Unstructured control-flow graph (CFG) abstractions and utilities. +use crate::func_at::FuncAt; use crate::{ - AttrSet, ConstKind, Context, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, FxIndexSet, - NodeKind, Region, Value, Var, VarKind, cf, + AttrSet, Const, ConstKind, Context, EntityOrientedDenseMap, FuncDefBody, FxIndexMap, + FxIndexSet, NodeKind, Region, Value, VarKind, }; -use itertools::Either; +use itertools::{Either, Itertools as _}; use smallvec::SmallVec; -/// The control-flow graph (CFG) of a function, as control-flow instructions -/// ([`ControlInst`]s) attached to [`Region`]s, as an "action on exit", i.e. -/// "terminator" (while intra-region control-flow is strictly structured). +/// The control-flow graph (CFG) of a function, as control-flow edges (`thunk`s) +/// attached to [`Region`]s, as an "action on exit" (similar to "terminator"s), +/// while intra-region control-flow is strictly structured. +// +// FIXME(eddyb) update docs above (or remove `ControlFlowGraph` entirely). +// +// FIXME(eddyb) treat "loop `break`s" (branches to the "loop merge" block) +// as Rust-like "`break` out of labeled block" for a block around the loop, +// e.g. `loop { ... break; ... }` -> `'label: { loop { ... break 'label; ... } }`, +// and encode the "labeled block" aspect using a node for "executing a CFG", +// with a `thunk` output type whenever the loop body has control-flow edges +// to the outside of the loop (and they're not to the "loop merge" block), +// such as `break`-ing out of an enclosing `switch`, or even `return`ing. +// +// FIXME(eddyb) the above approach could also be extended to encode other +// kinds of "merges" (`OpSelectionMerge`, or even the "continue" of a loop) +// as well (same "break out of labeled block" idea, really), and inside-out +// structurization should then preserve all possible SPIR-V merge hierarchies +// (even if structured SPIR-T lacks e.g. `switch` fallthru, it should always +// correctly reconverge, thanks to structurization never duplicating regions). +// +// FIXME(eddyb) in the end, it seems like there is a need for *both*: +// 1. `thunk` `switch`ing (a new `SelectionKind` variant?) +// - structurization (and similar transforms) can be seen as replacing +// `thunk` with boolean/integer target encodings, along the "target inputs", +// as plain values region can output, and `if`-`else`/`switch` for dispatch +// 2. `thunk` `loop`ing (new node or some kind of modified `NodeKind::Loop`?) +// - can be seen as taking the fixpoint of a `thunk -> thunk` region, +// which, in the fully unstructured CFG case, would consist of a single node +// (specifically a "`thunk` `switch`" dispatching all regions in the CFG, +// which makes it feel wasteful, but there has to be a way to distinguish +// between single-step and "until `break`-ing out") +// - maybe this can be done using the `thunk` `switch` node, with extra modes +// in e.g. its `SelectionKind` variant(s), such as "has a default case" and +// "`loop`-`switch` until `break" (although there is an unique hazard with +// reusing `NodeKind::Select` while having regions with different signatures, +// in that passes might assume matching signatures, and also any kind of +// looping control-flow has to account for the potential to execute any +// of the child regions, and therefore any side-effect, more than once etc.) #[derive(Clone, Default)] pub struct ControlFlowGraph { - pub control_inst_on_exit_from: EntityOrientedDenseMap, + pub target_thunk_on_exit_from: EntityOrientedDenseMap, // HACK(eddyb) this currently only comes from `OpLoopMerge`, and cannot be // inferred (because implies too strong of an ownership/uniqueness notion). pub loop_merge_to_loop_header: FxIndexMap, } -#[derive(Clone)] -pub struct ControlInst { - pub attrs: AttrSet, - - pub kind: ControlInstKind, - - pub inputs: SmallVec<[Value; 2]>, - - // FIXME(eddyb) change the inline size of this to fit most instructions. - // FIXME(eddyb) should this be renamed to ("outgoing") `edges`? - pub target_thunks: SmallVec<[Value; 4]>, -} - -#[derive(Clone)] -pub enum ControlInstKind { - /// Unconditional branch to a single target. - Branch, - - /// Branch to one of several targets, chosen by a single value input. - SelectBranch(cf::SelectionKind), -} - #[derive(Copy, Clone, PartialEq, Eq, Hash)] pub enum ControlTarget { Region(Region), @@ -49,39 +64,27 @@ pub enum ControlTarget { // // FIXME(eddyb) now that this is used through `NodeKind::ThunkBind`, // it should probably be more like `break` or some kind of "leave scope". + // FIXME(eddyb) one way to encode this unambiguously, even when dealing + // with e.g. multiple nested CFGs (such as the "execute a CFG" node idea), + // is to pick a region as the function-scoped "exit" (or "postdominator"), + // and in the worst-case it would only serve to output its own inputs + // (alternatively, non-trivial exits from "execute a CFG" can use `thunk` + // wrapping, so in the case of e.g. `return`ing from inside N nested loops, + // one wrap N `break`-like thunks, with each of the N levels being unwrapped + // by leaving the loops etc. - but that still technically has the ambiguity, + // and would result in the nested encoding showing up in the final structured + // result, so e.g. even if integers are used, N of them would be neeeded). Return, } -// HACK(eddyb) marker type for an `undef` thunk. -struct Unreachable; - -impl ControlTarget { - // HACK(eddyb) this isn't necessarily the best place, but allows reuse. - // FIXME(eddyb) properly distinguish the different failure cases possible, - // maybe even avoid panicking entirely? - fn of_thunk(cx: &Context, func_at_thunk: FuncAt<'_, Value>) -> Result { - match func_at_thunk.position { - Value::Var(thunk) => Ok(ControlTarget::of_thunk_var(func_at_thunk.at(thunk))), - Value::Const(ct) => match cx[ct].kind { - ConstKind::Undef => Err(Unreachable), - _ => unreachable!(), - }, - } - } +// HACK(eddyb) this is only public so it can be shared with `spv::lift`. +#[derive(Copy, Clone, PartialEq, Eq, Hash)] +pub struct ControlEdge { + pub source_region: Region, - // HACK(eddyb) this is the `Value::Var` case from `of_thunk`, separated so - // that traversal doesn't need to have access to the `Context`. - fn of_thunk_var(func_at_thunk: FuncAt<'_, Var>) -> Self { - match func_at_thunk.decl().kind() { - VarKind::NodeOutput { node, output_idx: 0 } => { - match func_at_thunk.at(node).def().kind { - NodeKind::ThunkBind(target) => target, - _ => unreachable!(), - } - } - _ => unreachable!(), - } - } + // HACK(eddyb) this is the minimal amount of information needed to look up + // the thunk, given access to the function (including `ControlFlowGraph`). + pub thunk_binding_region: Region, } impl ControlFlowGraph { @@ -111,6 +114,113 @@ impl ControlFlowGraph { ); post_order.into_iter().rev() } + + pub fn edges_from<'a>( + &'a self, + func_at_edge_source: FuncAt<'a, Region>, + ) -> impl DoubleEndedIterator + ExactSizeIterator + Clone + 'a { + self.edges_from_thunk_tailed_region(func_at_edge_source, func_at_edge_source.position) + } + + // FIXME(eddyb) properly distinguish the different failure cases possible, + // maybe even avoid panicking entirely? + fn edges_from_thunk_tailed_region<'a>( + &'a self, + func_at_edge_source: FuncAt<'a, Region>, + thunk_binding_region: Region, + ) -> impl DoubleEndedIterator + ExactSizeIterator + Clone + 'a { + let func = func_at_edge_source.at(()); + let edge_source_region = func_at_edge_source.position; + + let thunk = if edge_source_region == thunk_binding_region { + // FIXME(eddyb) make the thunk an actual region output instead. + *self.target_thunk_on_exit_from.get(edge_source_region).expect( + "cf::unstructured: missing `target_thunk_on_exit_from`, \ + despite having left structured control-flow", + ) + } else { + func.at(thunk_binding_region).def().outputs.iter().copied().exactly_one().ok().unwrap() + }; + + let thunk_node = match thunk { + Value::Var(thunk) => match func.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => node, + _ => unreachable!(), + }, + Value::Const(_) => { + return Either::Left( + [ControlEdge { source_region: edge_source_region, thunk_binding_region }] + .into_iter(), + ); + } + }; + + let thunk_binding_region_children = func.at(thunk_binding_region).def().children.iter(); + assert!(thunk_binding_region_children.last == Some(thunk_node)); + if edge_source_region != thunk_binding_region { + assert!(thunk_binding_region_children.first == thunk_binding_region_children.last); + } + + let thunk_node_def = func.at(thunk_node).def(); + match thunk_node_def.kind { + NodeKind::ThunkBind(_) => Either::Left( + [ControlEdge { source_region: edge_source_region, thunk_binding_region }] + .into_iter(), + ), + NodeKind::Select(_) => { + assert!(edge_source_region == thunk_binding_region); + + Either::Right(thunk_node_def.child_regions.iter().map(move |&case| { + self.edges_from_thunk_tailed_region(func_at_edge_source, case) + .exactly_one() + .ok() + .unwrap() + })) + } + _ => unreachable!(), + } + } + + pub fn edge_attrs_target_and_inputs<'a>( + &self, + func_at_edge: FuncAt<'a, ControlEdge>, + ) -> (AttrSet, Result, &'a [Value]) { + let func = func_at_edge.at(()); + let edge = func_at_edge.position; + + let thunk = if edge.source_region == edge.thunk_binding_region { + // FIXME(eddyb) make the thunk an actual region output instead. + *self.target_thunk_on_exit_from.get(edge.thunk_binding_region).expect( + "cf::unstructured: missing `target_thunk_on_exit_from`, \ + despite having left structured control-flow", + ) + } else { + func.at(edge.thunk_binding_region) + .def() + .outputs + .iter() + .copied() + .exactly_one() + .ok() + .unwrap() + }; + + match thunk { + Value::Var(thunk) => match func.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => { + let thunk_node_def = func.at(node).def(); + match thunk_node_def.kind { + NodeKind::ThunkBind(target) => { + (thunk_node_def.attrs, Ok(target), &thunk_node_def.inputs) + } + _ => unreachable!(), + } + } + _ => unreachable!(), + }, + Value::Const(ct) => (AttrSet::default(), Err(ct), &[]), + } + } } // HACK(eddyb) this only serves to disallow accessing `IncomingEdgeCount`'s private field. @@ -140,7 +250,6 @@ mod sealed { } } } -use crate::func_at::FuncAt; pub use sealed::IncomingEdgeCount; pub struct TraversalState { @@ -184,17 +293,11 @@ impl ControlFlowGraph { (state.pre_order_visit)(region); - let control_inst = self - .control_inst_on_exit_from - .get(region) - .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); + let targets = self.targets_of(func_at_region); - let targets = control_inst.target_thunks.iter().filter_map(|&thunk| match thunk { - Value::Var(thunk) => match ControlTarget::of_thunk_var(func.at(thunk)) { - ControlTarget::Region(target) => Some(target), - ControlTarget::Return => None, - }, - Value::Const(_) => None, + let targets = targets.filter_map(|target| match target { + Ok(ControlTarget::Region(target)) => Some(target), + Ok(ControlTarget::Return) | Err(_) => None, }); let targets = if state.reverse_targets { Either::Left(targets.rev()) @@ -207,6 +310,15 @@ impl ControlFlowGraph { (state.post_order_visit)(region); } + + fn targets_of<'a>( + &'a self, + func_at_edge_source: FuncAt<'a, Region>, + ) -> impl DoubleEndedIterator> + Clone + 'a { + let func = func_at_edge_source.at(()); + self.edges_from(func_at_edge_source) + .map(move |edge| self.edge_attrs_target_and_inputs(func.at(edge)).1) + } } /// Minimal loop analysis, based on Tarjan's SCC (strongly connected components) @@ -299,6 +411,10 @@ impl std::ops::BitOrAssign for EventualCfgExits { } } +// HACK(eddyb) marker type for an `undef` thunk. +#[derive(Copy, Clone)] +struct Unreachable; + impl<'a> LoopFinder<'a> { pub fn new(cx: &'a Context, func: FuncAt<'a, ()>, cfg: &'a ControlFlowGraph) -> Self { Self { @@ -356,26 +472,17 @@ impl<'a> LoopFinder<'a> { self.scc_stack.push(node); *state_entry = Some(SccState::Pending(scc_stack_idx)); - let control_inst = self - .cfg - .control_inst_on_exit_from - .get(node) - .expect("cfg: missing `ControlInst`, despite having left structured control-flow"); + let targets = self.targets_of(node); let mut eventual_cfg_exits = EventualCfgExits { - may_return_from_func: control_inst.target_thunks.iter().any(|&thunk| { - matches!( - ControlTarget::of_thunk(self.cx, self.func.at(thunk)), - Ok(ControlTarget::Return) - ) - }), + may_return_from_func: targets + .clone() + .any(|target| matches!(target, Ok(ControlTarget::Return))), }; - let earliest_scc_root = control_inst - .target_thunks - .iter() - .flat_map(|&thunk| { - let target = match ControlTarget::of_thunk(self.cx, self.func.at(thunk)) { + let earliest_scc_root = targets + .flat_map(|target| { + let target = match target { Ok(ControlTarget::Region(target)) => target, Ok(ControlTarget::Return) | Err(Unreachable) => { return None.into_iter().chain(None); @@ -435,15 +542,10 @@ impl<'a> LoopFinder<'a> { self.scc_stack[scc_start..] .iter() .flat_map(|&scc_node| { - self.cfg.control_inst_on_exit_from[scc_node] - .target_thunks - .iter() - .filter_map(|&thunk| { - match ControlTarget::of_thunk(self.cx, self.func.at(thunk)) { - Ok(ControlTarget::Region(target)) => Some(target), - Ok(ControlTarget::Return) | Err(Unreachable) => None, - } - }) + self.targets_of(scc_node).filter_map(|target| match target { + Ok(ControlTarget::Region(target)) => Some(target), + Ok(ControlTarget::Return) | Err(Unreachable) => None, + }) }) .filter(|&target| target_is_exit(target)) .collect(), @@ -479,4 +581,18 @@ impl<'a> LoopFinder<'a> { (earliest_scc_root, eventual_cfg_exits) } + + fn targets_of( + &self, + region: Region, + ) -> impl Iterator> + Clone + 'a { + let cx = self.cx; + + self.cfg.targets_of(self.func.at(region)).map(|edge| { + edge.map_err(|ct| match cx[ct].kind { + ConstKind::Undef => Unreachable, + _ => unreachable!(), + }) + }) + } } diff --git a/src/lib.rs b/src/lib.rs index f1cbdb2f..c5b3a120 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -757,9 +757,8 @@ pub struct FuncDefBody { /// [`Node`], or function (the latter being a "structured return") /// * "divergent": execution gets stuck in the region (an infinite loop), /// or is aborted (e.g. `OpTerminateInvocation` from SPIR-V) -/// * "unstructured": [`Region`]s which connect to other [`Region`]s -/// using [`cfg::ControlInst`](crate::cfg::ControlInst)s (as described by a -/// [`cfg::ControlFlowGraph`](crate::cfg::ControlFlowGraph)) +/// * "unstructured": [`Region`]s which connect to other [`Region`]s using "`thunk`s" +/// (as described by [`cfg::ControlFlowGraph`](crate::cfg::ControlFlowGraph)) /// /// When a function's entire body can be described by a single [`Region`], /// that function is said to have (entirely) "structured control-flow". @@ -805,8 +804,7 @@ pub struct FuncDefBody { /// instead of in the merge (where phi nodes require special-casing, as /// their "uses" of all the "source" values would normally be illegal) /// * in unstructured control-flow, region `inputs` are additionally used for -/// representing phi nodes, as [`cfg::ControlInst`](crate::cfg::ControlInst)s -/// passing values to their target regions +/// representing phi nodes, as `thunk`s passing values to their target regions /// * all value uses across unstructured control-flow edges (i.e. not in the /// same region containing the value definition) *require* explicit passing, /// as unstructured control-flow [`Region`](crate::Region)s diff --git a/src/print/mod.rs b/src/print/mod.rs index 993e1174..c0a7fb8d 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -787,13 +787,8 @@ impl DbgScopeDefPlacer<'_> { None => self.scopes_used_in_region(func_def_body.at_body()), Some(cfg) => DbgScopeDefMap::merge_unordered( DbgScopeDefPlace::top_of(func_def_body.at_body()), - cfg.rev_post_order(func_def_body).map(|region| { - let mut scopes = self.scopes_used_in_region(func_def_body.at(region)); - if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { - scopes.prepend_attrs(self.cx, control_inst.attrs); - } - scopes - }), + cfg.rev_post_order(func_def_body) + .map(|region| self.scopes_used_in_region(func_def_body.at(region))), ), } } @@ -3565,7 +3560,7 @@ impl Print for FuncDecl { label_header, pretty::Node::IndentedBlock(vec![def.at(region).print(printer)]) .into(), - cfg.control_inst_on_exit_from[region].print(printer), + cfg.target_thunk_on_exit_from[region].print(printer), ]) }) .intersperse({ @@ -4275,33 +4270,6 @@ impl FuncAt<'_, DataInst> { } } -impl Print for cf::unstructured::ControlInst { - type Output = pretty::Fragment; - fn print(&self, printer: &Printer<'_>) -> pretty::Fragment { - let Self { attrs, kind, inputs, target_thunks } = self; - - let attrs = attrs.print(printer); - - let kw_style = printer.imperative_keyword_style(); - - let mut targets = target_thunks.iter().map(|v| v.print(printer)); - - let def = match kind { - cf::unstructured::ControlInstKind::Branch => { - assert_eq!((targets.len(), inputs.len()), (1, 0)); - targets.next().unwrap() - } - - cf::unstructured::ControlInstKind::SelectBranch(kind) => { - assert_eq!(inputs.len(), 1); - kind.print_with_scrutinee_and_cases(printer, kw_style, inputs[0], targets) - } - }; - - pretty::Fragment::new([attrs, def]) - } -} - impl SelectionKind { fn print_with_scrutinee_and_cases( &self, diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 60051244..76941e03 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -8,8 +8,8 @@ use crate::{ AddrSpace, Attr, AttrSet, Const, ConstDef, ConstKind, Context, DataInst, DataInstDef, DataInstKind, DbgSrcLoc, DeclDef, ExportKey, Exportee, Func, FuncDecl, FuncParam, FxIndexMap, FxIndexSet, GlobalVar, GlobalVarDefBody, Import, Module, ModuleDebugInfo, ModuleDialect, Node, - NodeKind, OrdAssertEq, Region, Type, TypeDef, TypeKind, TypeOrConst, Value, Var, VarDecl, - VarKind, + NodeDef, NodeKind, OrdAssertEq, Region, Type, TypeDef, TypeKind, TypeOrConst, Value, Var, + VarDecl, VarKind, }; use itertools::Itertools; use rustc_hash::FxHashMap; @@ -283,8 +283,8 @@ struct FuncLifting<'a> { /// What determines the values for [`VarKind::RegionInput`]s, for a specific /// region (effectively the subset of "region parents" that support inputs). /// -/// Note that this is not used when a [`cf::unstructured::ControlInst`] has `target_inputs`, -/// and the target [`Region`] itself has phis for its `inputs`. +/// Note that this is not used when an unstructured `thunk` carries input values +/// for its target [`Region`] (which would itself have phis for its `inputs`). enum RegionInputsSource { FuncParams, LoopHeaderPhis(Node), @@ -303,7 +303,8 @@ enum CfgPoint { // HACK(eddyb) this is only needed to recover φ ("phi") semantics for // unstructured CFG edges, while letting SPIR-T use "BB args" semantics // (i.e. potentially different values for the same target `Region`). - UnstructuredEdge { source: Region, edge_idx: u32 }, + // NOTE(eddyb) this is much more relied upon after all `thunk` refactors. + UnstructuredEdge(cf::unstructured::ControlEdge), } struct BlockLifting<'a> { @@ -325,7 +326,7 @@ struct Phi { default_value: Option, } -/// Similar to [`cf::unstructured::ControlInst`], except: +/// Similar to unstructured `thunk`s, except: /// * `targets` use [`CfgPoint`]s instead of [`Region`]s, to be able to /// reach any of the SPIR-V blocks being created during lifting /// * φ ("phi") values can be provided for targets regardless of "which side" of @@ -356,8 +357,8 @@ enum TerminatorKind<'a> { Branch, SelectBranch(&'a cf::SelectionKind), - // HACK(eddyb) this is the only case `cf::unstructured::ControlInst` can't - // itself represent (as it has been moved to `NodeKind::ExitInvocation`). + // HACK(eddyb) this is the only case the unstructured CFG doesn't represent + // using `thunk`s (as it has been moved to `NodeKind::ExitInvocation`). ExitInvocation(&'a cf::ExitInvocationKind), } @@ -439,11 +440,32 @@ impl<'p> FuncAt<'_, CfgCursor<'p>> { /// chain within structured control-flow (i.e. no branching to child regions). fn unique_successor(self) -> Option> { let cursor = self.position; + + // HACK(eddyb) this skips past any tailing `thunk`-producing node, and + // goes straight to the `RegionExit` instead. + let filter_out_tail_thunk = |node| { + let node_def = &self.nodes[node]; + let is_last_node = node_def.next_in_list().is_none(); + let is_tail_thunk = is_last_node + && match node_def.kind { + NodeKind::ThunkBind(_) => true, + NodeKind::Select(_) => node_def.child_regions.iter().all(|&case| { + self.at(case).at_children().into_iter().exactly_one().is_ok_and( + |case_node| matches!(case_node.def().kind, NodeKind::ThunkBind(_)), + ) + }), + _ => false, + }; + (!is_tail_thunk).then_some(node) + }; + match cursor.point { // Entering a `Region` enters its first `Node` child, // or exits the region right away (if it has no children). CfgPoint::RegionEntry(region) => Some(CfgCursor { - point: match self.at(region).def().children.iter().first { + point: match (self.at(region).def().children.iter().first) + .and_then(filter_out_tail_thunk) + { Some(first_child) => CfgPoint::NodeEntry(first_child), None => CfgPoint::RegionExit(region), }, @@ -480,7 +502,7 @@ impl<'p> FuncAt<'_, CfgCursor<'p>> { // Exiting a `Node` chains to a sibling/parent. CfgPoint::NodeExit(node) => { - Some(match self.nodes[node].next_in_list() { + Some(match self.nodes[node].next_in_list().and_then(filter_out_tail_thunk) { // Enter the next sibling in the `Region`, if one exists. Some(next_node) => { CfgCursor { point: CfgPoint::NodeEntry(next_node), parent: cursor.parent } @@ -679,16 +701,17 @@ impl<'a> FuncLifting<'a> { let insts = match point { CfgPoint::NodeEntry(node) => match func_def_body.at(node).def().kind { - NodeKind::Select(_) - | NodeKind::Loop { .. } - | NodeKind::ExitInvocation(_) - | DataInstKind::ThunkBind(_) => SmallVec::new(), + NodeKind::Select(_) | NodeKind::Loop { .. } | NodeKind::ExitInvocation(_) => { + SmallVec::new() + } DataInstKind::FuncCall(_) | DataInstKind::Mem(_) | DataInstKind::QPtr(_) | DataInstKind::SpvInst(_) | DataInstKind::SpvExtInst { .. } => [node].into_iter().collect(), + + DataInstKind::ThunkBind(_) => unreachable!(), }, _ => SmallVec::new(), }; @@ -697,31 +720,47 @@ impl<'a> FuncLifting<'a> { let terminator = match (point, func_def_body.at(point_cursor).unique_successor()) { // Exiting a `Region` w/o a structured parent. (CfgPoint::RegionExit(region), None) => { - let unstructured_terminator = func_def_body + let unstructured_cfg_thunk = func_def_body .unstructured_cfg .as_ref() - .and_then(|cfg| cfg.control_inst_on_exit_from.get(region)); - if let Some(terminator) = unstructured_terminator { - let cf::unstructured::ControlInst { attrs, kind, inputs, target_thunks } = - terminator; - Terminator { - attrs: *attrs, - kind: match kind { - cf::unstructured::ControlInstKind::Branch => TerminatorKind::Branch, - cf::unstructured::ControlInstKind::SelectBranch(kind) => { - TerminatorKind::SelectBranch(kind) + .and_then(|cfg| Some((cfg, *cfg.target_thunk_on_exit_from.get(region)?))); + if let Some((cfg, thunk)) = unstructured_cfg_thunk { + // FIXME(eddyb) this partially overlaps with + // `ControlFlowGraph::edges_from_thunk_tailed_region`. + let thunk_node_def = match thunk { + Value::Var(thunk) => match func_def_body.at(thunk).decl().kind() { + VarKind::NodeOutput { node, output_idx: 0 } => { + Ok(func_def_body.at(node).def()) } + _ => unreachable!(), }, - // FIXME(eddyb) borrow these whenever possible. - inputs: inputs.clone(), + Value::Const(ct) => Err(ct), + }; + let (kind, inputs) = match thunk_node_def { + Ok(NodeDef { kind: NodeKind::ThunkBind(_), .. }) | Err(_) => { + (TerminatorKind::Branch, [].into_iter().collect()) + } + Ok(NodeDef { kind: NodeKind::Select(kind), inputs, .. }) => { + ( + TerminatorKind::SelectBranch(kind), + // FIXME(eddyb) borrow these whenever possible. + inputs.clone(), + ) + } + Ok(_) => unreachable!(), + }; + + Terminator { + attrs: thunk_node_def.map(|def| def.attrs).unwrap_or_default(), + kind, + inputs, // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. - // NOTE(eddyb) this is also now used for returns. - targets: (0..u32::try_from(target_thunks.len()).unwrap()) - .map(|edge_idx| CfgPoint::UnstructuredEdge { - source: region, - edge_idx, - }) + // NOTE(eddyb) this is much more relied upon, + // after all `thunk` refactors. + targets: cfg + .edges_from(func_def_body.at(region)) + .map(CfgPoint::UnstructuredEdge) .collect(), target_phi_values: FxIndexMap::default(), merge: None, @@ -739,42 +778,22 @@ impl<'a> FuncLifting<'a> { } } } - (CfgPoint::UnstructuredEdge { source, edge_idx }, None) => { + (CfgPoint::UnstructuredEdge(edge), None) => { let cfg = func_def_body.unstructured_cfg.as_ref().unwrap(); - let cf::unstructured::ControlInst { attrs, kind: _, inputs: _, target_thunks } = - &cfg.control_inst_on_exit_from[source]; - - // HACK(eddyb) marker type for an `undef` thunk. - struct Unreachable; - - // FIXME(eddyb) deduplicate with `ControlTarget::of_thunk`. - let func = func_def_body.at(()); - let (target, target_inputs) = match target_thunks[edge_idx as usize] { - Value::Var(thunk) => match func.at(thunk).decl().kind() { - VarKind::NodeOutput { node, output_idx: 0 } => { - let thunk_node_def = func.at(node).def(); - match thunk_node_def.kind { - NodeKind::ThunkBind(target) => { - (Ok(target), &thunk_node_def.inputs[..]) - } - _ => unreachable!(), - } - } - _ => unreachable!(), - }, - Value::Const(ct) => match cx[ct].kind { - ConstKind::Undef => (Err(Unreachable), &[][..]), - _ => unreachable!(), - }, - }; + let (attrs, target, target_inputs) = + cfg.edge_attrs_target_and_inputs(func_def_body.at(edge)); + let target = match target { Ok(cf::unstructured::ControlTarget::Region(target)) => Ok(target), Ok(cf::unstructured::ControlTarget::Return) => Err(TerminatorKind::Return), - Err(Unreachable) => Err(TerminatorKind::Unreachable), + Err(ct) => match cx[ct].kind { + ConstKind::Undef => Err(TerminatorKind::Unreachable), + _ => unreachable!(), + }, }; match target { Ok(target) => Terminator { - attrs: *attrs, + attrs, kind: TerminatorKind::Branch, inputs: [].into_iter().collect(), targets: [CfgPoint::RegionEntry(target)].into_iter().collect(), @@ -785,7 +804,7 @@ impl<'a> FuncLifting<'a> { }, Err(terminator_kind) => { Terminator { - attrs: *attrs, + attrs, kind: terminator_kind, // FIXME(eddyb) borrow these whenever possible. inputs: target_inputs.iter().copied().collect(), @@ -971,11 +990,11 @@ impl<'a> FuncLifting<'a> { // FIXME(eddyb) try limiting this to repeated target `Region`s // which *also* pass different value inputs. - // NOTE(eddyb) this is also now used for returns. - let edge_count = cfg.control_inst_on_exit_from[region].target_thunks.len(); - for edge_idx in 0..u32::try_from(edge_count).unwrap() { + // NOTE(eddyb) this is much more relied upon, + // after all `thunk` refactors. + for edge in cfg.edges_from(func_def_body.at(region)) { visit_cfg_point(CfgCursor { - point: CfgPoint::UnstructuredEdge { source: region, edge_idx }, + point: CfgPoint::UnstructuredEdge(edge), parent: None, })?; } diff --git a/src/spv/lower.rs b/src/spv/lower.rs index 160b8fa2..7a648263 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -3,13 +3,14 @@ use crate::cf::{self, SelectionKind}; use crate::spv::{self, spec}; // FIXME(eddyb) import more to avoid `crate::` everywhere. +use crate::func_at::FuncAtMut; use crate::{ AddrSpace, Attr, AttrSet, Const, ConstDef, ConstKind, Context, DataInstDef, DataInstKind, DbgSrcLoc, DeclDef, Diag, EntityDefs, ExportKey, Exportee, Func, FuncDecl, FuncDefBody, FuncParam, FxIndexMap, GlobalVarDecl, GlobalVarDefBody, Import, InternedStr, Module, NodeDef, NodeKind, Region, RegionDef, Type, TypeDef, TypeKind, TypeOrConst, Value, VarDecl, print, }; -use itertools::Either; +use itertools::{Either, Itertools as _}; use rustc_hash::FxHashMap; use smallvec::SmallVec; use std::collections::{BTreeMap, BTreeSet}; @@ -1484,115 +1485,150 @@ impl Module { } } + // FIXME(eddyb) collect targets in this form to + // begin with (instead of recombining them here). + let targets_with_inputs = targets.into_iter().map(|target| { + ( + cf::unstructured::ControlTarget::Region(target), + target_inputs.get(&target).cloned().unwrap_or_default(), + ) + }); + // FIXME(eddyb) cache this. let thunk_ty = cx.intern(TypeKind::Thunk); - let mut build_thunk = |target, target_inputs| { - let thunk_node = func_def_body.nodes.define( + let build_thunk = + |func_at_region: FuncAtMut<'_, Region>, (target, target_inputs)| { + let region = func_at_region.position; + let func = func_at_region.at(()); + + let thunk_node = func.nodes.define( + &cx, + NodeDef { + attrs, + kind: NodeKind::ThunkBind(target), + inputs: target_inputs, + child_regions: [].into_iter().collect(), + outputs: [].into_iter().collect(), + } + .into(), + ); + func.regions[region].children.insert_last(thunk_node, func.nodes); + + let thunk_var = func.vars.define( + &cx, + VarDecl { + attrs: AttrSet::default(), + ty: thunk_ty, + + def_parent: Either::Right(thunk_node), + def_idx: 0, + }, + ); + func.nodes[thunk_node].outputs.push(thunk_var); + + Value::Var(thunk_var) + }; + + let selection_kind = if opcode == wk.OpBranchConditional { + assert_eq!((targets_with_inputs.len(), inputs.len()), (2, 1)); + Some(SelectionKind::BoolCond) + } else if opcode == wk.OpSwitch { + Some(SelectionKind::SpvInst(raw_inst.without_ids.clone())) + } else { + None + }; + + let target_thunk = if let Some(selection_kind) = selection_kind { + let cases = targets_with_inputs + .map(|target_with_inputs| { + let case = func_def_body.regions.define(&cx, RegionDef::default()); + let thunk = + build_thunk(func_def_body.at_mut(case), target_with_inputs); + func_def_body.regions[case].outputs.push(thunk); + case + }) + .collect(); + + let select_node = func_def_body.nodes.define( &cx, NodeDef { - attrs: AttrSet::default(), - kind: NodeKind::ThunkBind(target), - inputs: target_inputs, - child_regions: [].into_iter().collect(), + attrs, + kind: NodeKind::Select(selection_kind), + inputs: mem::take(&mut inputs), + child_regions: cases, outputs: [].into_iter().collect(), } .into(), ); - current_block_region_def + func_def_body.regions[current_block.region] .children - .insert_last(thunk_node, &mut func_def_body.nodes); + .insert_last(select_node, &mut func_def_body.nodes); - let thunk_var = func_def_body.vars.define( + let select_thunk_var = func_def_body.vars.define( &cx, VarDecl { attrs: AttrSet::default(), ty: thunk_ty, - def_parent: Either::Right(thunk_node), + def_parent: Either::Right(select_node), def_idx: 0, }, ); - func_def_body.nodes[thunk_node].outputs.push(thunk_var); - - Value::Var(thunk_var) - }; - - // FIXME(eddyb) collect targets in this form to - // begin with (instead of recombining them here). - let mut target_thunks: SmallVec<[_; 4]> = targets - .into_iter() - .map(|target| { - build_thunk( - cf::unstructured::ControlTarget::Region(target), - target_inputs.get(&target).cloned().unwrap_or_default(), - ) - }) - .collect(); + func_def_body.nodes[select_node].outputs.push(select_thunk_var); - let kind = if opcode == wk.OpUnreachable { - assert!(target_thunks.is_empty() && inputs.is_empty()); - // FIXME(eddyb) cache this. - target_thunks.push(Value::Const(cx.intern(ConstDef { - attrs: AttrSet::default(), - ty: thunk_ty, - kind: ConstKind::Undef, - }))); - cf::unstructured::ControlInstKind::Branch + Value::Var(select_thunk_var) } else if [wk.OpReturn, wk.OpReturnValue].contains(&opcode) { - assert!(target_thunks.is_empty() && inputs.len() <= 1); - target_thunks.push(build_thunk( - cf::unstructured::ControlTarget::Return, - mem::take(&mut inputs), - )); - cf::unstructured::ControlInstKind::Branch - } else if target_thunks.is_empty() { - let node = func_def_body.nodes.define( - &cx, - NodeDef { - attrs, - kind: NodeKind::ExitInvocation(cf::ExitInvocationKind::SpvInst( - raw_inst.without_ids.clone(), - )), - inputs: mem::take(&mut inputs), - child_regions: [].into_iter().collect(), - outputs: [].into_iter().collect(), - } - .into(), - ); - current_block_region_def - .children - .insert_last(node, &mut func_def_body.nodes); + assert!(targets_with_inputs.len() == 0 && inputs.len() <= 1); + build_thunk( + func_def_body.at_mut(current_block.region), + (cf::unstructured::ControlTarget::Return, mem::take(&mut inputs)), + ) + } else if targets_with_inputs.len() == 0 { + if opcode != wk.OpUnreachable { + let node = func_def_body.nodes.define( + &cx, + NodeDef { + attrs, + kind: NodeKind::ExitInvocation( + cf::ExitInvocationKind::SpvInst( + raw_inst.without_ids.clone(), + ), + ), + inputs: mem::take(&mut inputs), + child_regions: [].into_iter().collect(), + outputs: [].into_iter().collect(), + } + .into(), + ); + func_def_body.regions[current_block.region] + .children + .insert_last(node, &mut func_def_body.nodes); + } + // FIXME(eddyb) cache this. - target_thunks.push(Value::Const(cx.intern(ConstDef { + Value::Const(cx.intern(ConstDef { attrs: AttrSet::default(), ty: thunk_ty, kind: ConstKind::Undef, - }))); - cf::unstructured::ControlInstKind::Branch + })) } else if opcode == wk.OpBranch { - assert_eq!((target_thunks.len(), inputs.len()), (1, 0)); - cf::unstructured::ControlInstKind::Branch - } else if opcode == wk.OpBranchConditional { - assert_eq!((target_thunks.len(), inputs.len()), (2, 1)); - cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::BoolCond) - } else if opcode == wk.OpSwitch { - cf::unstructured::ControlInstKind::SelectBranch(SelectionKind::SpvInst( - raw_inst.without_ids.clone(), - )) + build_thunk( + func_def_body.at_mut(current_block.region), + targets_with_inputs.exactly_one().ok().unwrap(), + ) } else { return Err(invalid("unsupported control-flow instruction")); }; + assert_eq!(inputs.len(), 0); + func_def_body .unstructured_cfg .as_mut() .unwrap() - .control_inst_on_exit_from - .insert( - current_block.region, - cf::unstructured::ControlInst { attrs, kind, inputs, target_thunks }, - ); + .target_thunk_on_exit_from + .insert(current_block.region, target_thunk); } else if opcode == wk.OpPhi { if !current_block_region_def.children.is_empty() { return Err(invalid( diff --git a/src/transform.rs b/src/transform.rs index c73b7980..6238e395 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -582,8 +582,8 @@ impl InnerInPlaceTransform for FuncDefBody { transformer.in_place_transform_region_def(self.at_mut(region)); let cfg = self.unstructured_cfg.as_mut().unwrap(); - if let Some(control_inst) = cfg.control_inst_on_exit_from.get_mut(region) { - control_inst.inner_in_place_transform_with(transformer); + if let Some(thunk) = cfg.target_thunk_on_exit_from.get_mut(region) { + transformer.transform_value_use(thunk).apply_to(thunk); } } } @@ -680,26 +680,6 @@ impl InnerInPlaceTransform for VarDecl { } } -impl InnerInPlaceTransform for cf::unstructured::ControlInst { - fn inner_in_place_transform_with(&mut self, transformer: &mut impl Transformer) { - let Self { attrs, kind, inputs, target_thunks } = self; - - transformer.transform_attr_set_use(*attrs).apply_to(attrs); - match kind { - cf::unstructured::ControlInstKind::Branch - | cf::unstructured::ControlInstKind::SelectBranch( - SelectionKind::BoolCond | SelectionKind::SpvInst(_), - ) => {} - } - for v in inputs { - transformer.transform_value_use(v).apply_to(v); - } - for v in target_thunks { - transformer.transform_value_use(v).apply_to(v); - } - } -} - impl InnerTransform for Value { fn inner_transform_with(&self, transformer: &mut impl Transformer) -> Transformed { match self { diff --git a/src/visit.rs b/src/visit.rs index 5907e468..d4bda14c 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -427,8 +427,8 @@ impl InnerVisit for FuncDefBody { for region in cfg.rev_post_order(self) { visitor.visit_region_def(self.at(region)); - if let Some(control_inst) = cfg.control_inst_on_exit_from.get(region) { - control_inst.inner_visit_with(visitor); + if let Some(thunk) = cfg.target_thunk_on_exit_from.get(region) { + visitor.visit_value_use(thunk); } } } @@ -514,26 +514,6 @@ impl InnerVisit for VarDecl { } } -impl InnerVisit for cf::unstructured::ControlInst { - fn inner_visit_with<'a>(&'a self, visitor: &mut impl Visitor<'a>) { - let Self { attrs, kind, inputs, target_thunks } = self; - - visitor.visit_attr_set_use(*attrs); - match kind { - cf::unstructured::ControlInstKind::Branch - | cf::unstructured::ControlInstKind::SelectBranch( - SelectionKind::BoolCond | SelectionKind::SpvInst(_), - ) => {} - } - for v in inputs { - visitor.visit_value_use(v); - } - for v in target_thunks { - visitor.visit_value_use(v); - } - } -} - impl InnerVisit for Value { fn inner_visit_with<'a>(&'a self, visitor: &mut impl Visitor<'a>) { match self { From 477b4ecc2a1db4e4c59b0fe7ceae6fad08733a2b Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Thu, 4 Dec 2025 21:29:58 +0200 Subject: [PATCH 7/7] cf: move the target `thunk` of each unstructured region into its `outputs`. --- src/cf/structurize.rs | 39 ++++----------------------------------- src/cf/unstructured.rs | 42 ++++++++++++++---------------------------- src/print/mod.rs | 11 +++++------ src/spv/lift.rs | 19 +++++++++++++++---- src/spv/lower.rs | 8 ++------ src/transform.rs | 5 ----- src/visit.rs | 4 ---- 7 files changed, 40 insertions(+), 88 deletions(-) diff --git a/src/cf/structurize.rs b/src/cf/structurize.rs index 27073dbe..d8ffa811 100644 --- a/src/cf/structurize.rs +++ b/src/cf/structurize.rs @@ -1065,27 +1065,12 @@ impl<'a> Structurizer<'a> { edge_source_region: Region, thunk_binding_region: Region, ) -> Result { - let thunk = if edge_source_region == thunk_binding_region { - // FIXME(eddyb) make the thunk an actual region output instead. - this.func_def_body - .unstructured_cfg - .as_mut() - .unwrap() - .target_thunk_on_exit_from - .remove(edge_source_region) - .expect( - // FIXME(eddyb) this only really makes sense - "cfg::Structurizer::structurize_region: missing \ - `target_thunk_on_exit_from` \ - (CFG wasn't unstructured in the first place?)", - ) - } else { + let thunk = mem::take(&mut this.func_def_body.at_mut(thunk_binding_region).def().outputs) .into_iter() .exactly_one() .ok() - .unwrap() - }; + .unwrap(); let thunk_node = match thunk { Value::Var(thunk) => match this.func_def_body.at(thunk).decl().kind() { @@ -1974,15 +1959,7 @@ impl<'a> Structurizer<'a> { build_thunk(self.func_def_body.at_mut(branch_source), then_edge) }; - assert!( - self.func_def_body - .unstructured_cfg - .as_mut() - .unwrap() - .target_thunk_on_exit_from - .insert(branch_source, thunk) - .is_none() - ); + self.func_def_body.regions[branch_source].outputs = [thunk].into_iter().collect(); } let final_source = match control_source { @@ -2000,15 +1977,7 @@ impl<'a> Structurizer<'a> { ty: thunk_ty, kind: ConstKind::Undef, })); - assert!( - self.func_def_body - .unstructured_cfg - .as_mut() - .unwrap() - .target_thunk_on_exit_from - .insert(final_source, final_thunk) - .is_none() - ); + self.func_def_body.regions[final_source].outputs = [final_thunk].into_iter().collect(); } /// Create an undefined constant (as a placeholder where a value needs to be diff --git a/src/cf/unstructured.rs b/src/cf/unstructured.rs index bd3efdfb..fec3a3be 100644 --- a/src/cf/unstructured.rs +++ b/src/cf/unstructured.rs @@ -49,10 +49,11 @@ use smallvec::SmallVec; // of the child regions, and therefore any side-effect, more than once etc.) #[derive(Clone, Default)] pub struct ControlFlowGraph { - pub target_thunk_on_exit_from: EntityOrientedDenseMap, - // HACK(eddyb) this currently only comes from `OpLoopMerge`, and cannot be // inferred (because implies too strong of an ownership/uniqueness notion). + // + // FIXME(eddyb) this is the only reason for `ControlFlowGraph` to still + // exist in this state, and should be replaced with one of the ideas above. pub loop_merge_to_loop_header: FxIndexMap, } @@ -132,15 +133,8 @@ impl ControlFlowGraph { let func = func_at_edge_source.at(()); let edge_source_region = func_at_edge_source.position; - let thunk = if edge_source_region == thunk_binding_region { - // FIXME(eddyb) make the thunk an actual region output instead. - *self.target_thunk_on_exit_from.get(edge_source_region).expect( - "cf::unstructured: missing `target_thunk_on_exit_from`, \ - despite having left structured control-flow", - ) - } else { - func.at(thunk_binding_region).def().outputs.iter().copied().exactly_one().ok().unwrap() - }; + let thunk = + func.at(thunk_binding_region).def().outputs.iter().copied().exactly_one().ok().unwrap(); let thunk_node = match thunk { Value::Var(thunk) => match func.at(thunk).decl().kind() { @@ -188,22 +182,15 @@ impl ControlFlowGraph { let func = func_at_edge.at(()); let edge = func_at_edge.position; - let thunk = if edge.source_region == edge.thunk_binding_region { - // FIXME(eddyb) make the thunk an actual region output instead. - *self.target_thunk_on_exit_from.get(edge.thunk_binding_region).expect( - "cf::unstructured: missing `target_thunk_on_exit_from`, \ - despite having left structured control-flow", - ) - } else { - func.at(edge.thunk_binding_region) - .def() - .outputs - .iter() - .copied() - .exactly_one() - .ok() - .unwrap() - }; + let thunk = func + .at(edge.thunk_binding_region) + .def() + .outputs + .iter() + .copied() + .exactly_one() + .ok() + .unwrap(); match thunk { Value::Var(thunk) => match func.at(thunk).decl().kind() { @@ -271,7 +258,6 @@ impl ControlFlowGraph { // Quick sanity check that this is the right CFG for `func_def_body`. assert!(std::ptr::eq(func_def_body.unstructured_cfg.as_ref().unwrap(), self)); - assert!(func_at_body.def().outputs.is_empty()); self.traverse(func_at_body, state); } diff --git a/src/print/mod.rs b/src/print/mod.rs index c0a7fb8d..c7358129 100644 --- a/src/print/mod.rs +++ b/src/print/mod.rs @@ -3543,24 +3543,23 @@ impl Print for FuncDecl { pretty::Fragment::default() }; - // FIXME(eddyb) `:` as used here for C-like "label syntax" - // interferes (in theory) with `e: T` "type ascription syntax". pretty::Fragment::new([ pretty::Node::ForceLineSeparation.into(), label.print_as_def(printer), label_inputs, - ":".into(), - pretty::Node::ForceLineSeparation.into(), ]) + } else if region == def.body { + printer.imperative_keyword_style().apply("entry").into() } else { - pretty::Fragment::default() + printer.error_style().apply("/* undefined label */_").into() }; pretty::Fragment::new([ label_header, + " {".into(), pretty::Node::IndentedBlock(vec![def.at(region).print(printer)]) .into(), - cfg.target_thunk_on_exit_from[region].print(printer), + "}".into(), ]) }) .intersperse({ diff --git a/src/spv/lift.rs b/src/spv/lift.rs index 76941e03..06e32150 100644 --- a/src/spv/lift.rs +++ b/src/spv/lift.rs @@ -720,10 +720,21 @@ impl<'a> FuncLifting<'a> { let terminator = match (point, func_def_body.at(point_cursor).unique_successor()) { // Exiting a `Region` w/o a structured parent. (CfgPoint::RegionExit(region), None) => { - let unstructured_cfg_thunk = func_def_body - .unstructured_cfg - .as_ref() - .and_then(|cfg| Some((cfg, *cfg.target_thunk_on_exit_from.get(region)?))); + let unstructured_cfg_thunk = + func_def_body.unstructured_cfg.as_ref().map(|cfg| { + ( + cfg, + func_def_body + .at(region) + .def() + .outputs + .iter() + .copied() + .exactly_one() + .ok() + .unwrap(), + ) + }); if let Some((cfg, thunk)) = unstructured_cfg_thunk { // FIXME(eddyb) this partially overlaps with // `ControlFlowGraph::edges_from_thunk_tailed_region`. diff --git a/src/spv/lower.rs b/src/spv/lower.rs index 7a648263..9111a098 100644 --- a/src/spv/lower.rs +++ b/src/spv/lower.rs @@ -1623,12 +1623,8 @@ impl Module { assert_eq!(inputs.len(), 0); - func_def_body - .unstructured_cfg - .as_mut() - .unwrap() - .target_thunk_on_exit_from - .insert(current_block.region, target_thunk); + func_def_body.regions[current_block.region].outputs = + [target_thunk].into_iter().collect(); } else if opcode == wk.OpPhi { if !current_block_region_def.children.is_empty() { return Err(invalid( diff --git a/src/transform.rs b/src/transform.rs index 6238e395..148669ad 100644 --- a/src/transform.rs +++ b/src/transform.rs @@ -580,11 +580,6 @@ impl InnerInPlaceTransform for FuncDefBody { for region in rpo { transformer.in_place_transform_region_def(self.at_mut(region)); - - let cfg = self.unstructured_cfg.as_mut().unwrap(); - if let Some(thunk) = cfg.target_thunk_on_exit_from.get_mut(region) { - transformer.transform_value_use(thunk).apply_to(thunk); - } } } } diff --git a/src/visit.rs b/src/visit.rs index d4bda14c..b25d4cbc 100644 --- a/src/visit.rs +++ b/src/visit.rs @@ -426,10 +426,6 @@ impl InnerVisit for FuncDefBody { Some(cfg) => { for region in cfg.rev_post_order(self) { visitor.visit_region_def(self.at(region)); - - if let Some(thunk) = cfg.target_thunk_on_exit_from.get(region) { - visitor.visit_value_use(thunk); - } } } }