"Unintern" DataInstForm (inlining its fields back into `DataInstDef…

…`). (Rust-GPU#12) Effectively undoes: - EmbarkStudios/spirt#28 Some quick unscientific testing reveals no significant perf impact (i.e. the difference is lost in the noise). The motivation for undoing this interning is the prospect of combining `DataInstDef` into `NodeDef` (as mentioned in Rust-GPU#7), and for unrelated pragmatic reasons, `NodeDef` can't have its outputs interned (though long-term maybe we could intern the `kind` field, if really necessary, assuming we first take "child regions" out of it). The one thing I realized too late there's no pre-existing consensus for, is the `output_type`, which used to be between `kind` and `inputs` (matching `DataInstFormDef`, in fact), while `NodeDef`'s `outputs` is the last field. The only argument to have outputs first is the `let (out0, out1) = foo(in0, in1);` style syntax (though pretty-printed SPIR-T omits the `let`), but I'm not sure that's worth flipping the order over.
LykenSol · Nov 6, 2024 · 5c42614 · 5c42614
2 parents 000ec78 + 43524d7
commit 5c42614
Show file tree

Hide file tree

Showing 15 changed files with 140 additions and 317 deletions.
diff --git a/src/context.rs b/src/context.rs
@@ -888,7 +888,6 @@ interners! {
         crate::AttrSetDef,
         crate::TypeDef,
         crate::ConstDef,
-        crate::DataInstFormDef,
     }
 
     // FIXME(eddyb) consider a more uniform naming scheme than the combination
@@ -897,7 +896,6 @@ interners! {
     AttrSet default(crate::AttrSetDef::default()) => crate::AttrSetDef,
     Type => crate::TypeDef,
     Const => crate::ConstDef,
-    DataInstForm => crate::DataInstFormDef,
 }
 
 impl<I: sealed::Interned> InternInCx<I> for I::Def

diff --git a/src/func_at.rs b/src/func_at.rs
@@ -116,7 +116,7 @@ impl FuncAt<'_, Value> {
             Value::NodeOutput { node, output_idx } => {
                 self.at(node).def().outputs[output_idx as usize].ty
             }
-            Value::DataInstOutput(inst) => cx[self.at(inst).def().form].output_type.unwrap(),
+            Value::DataInstOutput(inst) => self.at(inst).def().output_type.unwrap(),
         }
     }
 }

diff --git a/src/lib.rs b/src/lib.rs
@@ -934,26 +934,10 @@ pub use context::DataInst;
 pub struct DataInstDef {
     pub attrs: AttrSet,
 
-    pub form: DataInstForm,
+    pub kind: DataInstKind,
 
     // FIXME(eddyb) change the inline size of this to fit most instructions.
     pub inputs: SmallVec<[Value; 2]>,
-}
-
-/// Interned handle for a [`DataInstFormDef`](crate::DataInstFormDef)
-/// (a "form", or "template", for [`DataInstDef`](crate::DataInstDef)s).
-pub use context::DataInstForm;
-
-/// "Form" (or "template") definition for [`DataInstFormDef`]s, which includes
-/// most of their common *static* information (notably excluding `attrs`, as
-/// they vary more often due to handling diagnostics, debuginfo, refinement etc.).
-//
-// FIXME(eddyb) now that this is interned, try to find all the code that was
-// working around needing to borrow `DataInstKind`, just because it was owned
-// by a `FuncDefBody` (instead of interned in the `Context`).
-#[derive(Clone, PartialEq, Eq, Hash)]
-pub struct DataInstFormDef {
-    pub kind: DataInstKind,
 
     pub output_type: Option<Type>,
 }

diff --git a/src/passes/legalize.rs b/src/passes/legalize.rs
@@ -1,7 +1,5 @@
 use crate::visit::{InnerVisit, Visitor};
-use crate::{
-    AttrSet, Const, Context, DataInstForm, DeclDef, Func, FxIndexSet, GlobalVar, Module, Type, cfg,
-};
+use crate::{AttrSet, Const, Context, DeclDef, Func, FxIndexSet, GlobalVar, Module, Type, cfg};
 
 /// Apply the [`cfg::Structurizer`] algorithm to all function definitions in `module`.
 pub fn structurize_func_cfgs(module: &mut Module) {
@@ -14,7 +12,6 @@ pub fn structurize_func_cfgs(module: &mut Module) {
 
         seen_types: FxIndexSet::default(),
         seen_consts: FxIndexSet::default(),
-        seen_data_inst_forms: FxIndexSet::default(),
         seen_global_vars: FxIndexSet::default(),
         seen_funcs: FxIndexSet::default(),
     };
@@ -37,7 +34,6 @@ struct ReachableUseCollector<'a> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
     seen_types: FxIndexSet<Type>,
     seen_consts: FxIndexSet<Const>,
-    seen_data_inst_forms: FxIndexSet<DataInstForm>,
     seen_global_vars: FxIndexSet<GlobalVar>,
     seen_funcs: FxIndexSet<Func>,
 }
@@ -57,11 +53,6 @@ impl Visitor<'_> for ReachableUseCollector<'_> {
             self.visit_const_def(&self.cx[ct]);
         }
     }
-    fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) {
-        if self.seen_data_inst_forms.insert(data_inst_form) {
-            self.visit_data_inst_form_def(&self.cx[data_inst_form]);
-        }
-    }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
         if self.seen_global_vars.insert(gv) {

diff --git a/src/passes/link.rs b/src/passes/link.rs
@@ -1,8 +1,8 @@
 use crate::transform::{InnerTransform, Transformed, Transformer};
 use crate::visit::{InnerVisit, Visitor};
 use crate::{
-    AttrSet, Const, Context, DataInstForm, DeclDef, ExportKey, Exportee, Func, FxIndexSet,
-    GlobalVar, Import, Module, Type,
+    AttrSet, Const, Context, DeclDef, ExportKey, Exportee, Func, FxIndexSet, GlobalVar, Import,
+    Module, Type,
 };
 use rustc_hash::{FxHashMap, FxHashSet};
 use std::collections::VecDeque;
@@ -33,7 +33,6 @@ pub fn minimize_exports(module: &mut Module, is_root: impl Fn(&ExportKey) -> boo
 
         seen_types: FxHashSet::default(),
         seen_consts: FxHashSet::default(),
-        seen_data_inst_forms: FxHashSet::default(),
         seen_global_vars: FxHashSet::default(),
         seen_funcs: FxHashSet::default(),
     };
@@ -61,7 +60,6 @@ struct LiveExportCollector<'a> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
     seen_types: FxHashSet<Type>,
     seen_consts: FxHashSet<Const>,
-    seen_data_inst_forms: FxHashSet<DataInstForm>,
     seen_global_vars: FxHashSet<GlobalVar>,
     seen_funcs: FxHashSet<Func>,
 }
@@ -81,11 +79,6 @@ impl Visitor<'_> for LiveExportCollector<'_> {
             self.visit_const_def(&self.cx[ct]);
         }
     }
-    fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) {
-        if self.seen_data_inst_forms.insert(data_inst_form) {
-            self.visit_data_inst_form_def(&self.cx[data_inst_form]);
-        }
-    }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
         if self.seen_global_vars.insert(gv) {
@@ -128,7 +121,6 @@ pub fn resolve_imports(module: &mut Module) {
 
             seen_types: FxHashSet::default(),
             seen_consts: FxHashSet::default(),
-            seen_data_inst_forms: FxHashSet::default(),
             seen_global_vars: FxHashSet::default(),
             seen_funcs: FxHashSet::default(),
         };
@@ -144,7 +136,6 @@ pub fn resolve_imports(module: &mut Module) {
 
         transformed_types: FxHashMap::default(),
         transformed_consts: FxHashMap::default(),
-        transformed_data_inst_forms: FxHashMap::default(),
         transformed_global_vars: FxHashMap::default(),
         global_var_queue: VecDeque::new(),
         transformed_funcs: FxHashMap::default(),
@@ -179,7 +170,6 @@ struct ImportResolutionCollector<'a> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
     seen_types: FxHashSet<Type>,
     seen_consts: FxHashSet<Const>,
-    seen_data_inst_forms: FxHashSet<DataInstForm>,
     seen_global_vars: FxHashSet<GlobalVar>,
     seen_funcs: FxHashSet<Func>,
 }
@@ -199,11 +189,6 @@ impl Visitor<'_> for ImportResolutionCollector<'_> {
             self.visit_const_def(&self.cx[ct]);
         }
     }
-    fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) {
-        if self.seen_data_inst_forms.insert(data_inst_form) {
-            self.visit_data_inst_form_def(&self.cx[data_inst_form]);
-        }
-    }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
         if self.seen_global_vars.insert(gv) {
@@ -250,7 +235,6 @@ struct ImportResolver<'a> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
     transformed_types: FxHashMap<Type, Transformed<Type>>,
     transformed_consts: FxHashMap<Const, Transformed<Const>>,
-    transformed_data_inst_forms: FxHashMap<DataInstForm, Transformed<DataInstForm>>,
     transformed_global_vars: FxHashMap<GlobalVar, Transformed<GlobalVar>>,
     global_var_queue: VecDeque<GlobalVar>,
     transformed_funcs: FxHashMap<Func, Transformed<Func>>,
@@ -277,19 +261,6 @@ impl Transformer for ImportResolver<'_> {
         self.transformed_consts.insert(ct, transformed);
         transformed
     }
-    fn transform_data_inst_form_use(
-        &mut self,
-        data_inst_form: DataInstForm,
-    ) -> Transformed<DataInstForm> {
-        if let Some(&cached) = self.transformed_data_inst_forms.get(&data_inst_form) {
-            return cached;
-        }
-        let transformed = self
-            .transform_data_inst_form_def(&self.cx[data_inst_form])
-            .map(|data_inst_form_def| self.cx.intern(data_inst_form_def));
-        self.transformed_data_inst_forms.insert(data_inst_form, transformed);
-        transformed
-    }
 
     fn transform_global_var_use(&mut self, gv: GlobalVar) -> Transformed<GlobalVar> {
         if let Some(&cached) = self.transformed_global_vars.get(&gv) {

diff --git a/src/passes/qptr.rs b/src/passes/qptr.rs
@@ -1,8 +1,8 @@
 //! [`QPtr`](crate::TypeKind::QPtr) transforms.
 
+use crate::qptr;
 use crate::visit::{InnerVisit, Visitor};
 use crate::{AttrSet, Const, Context, Func, FxIndexSet, GlobalVar, Module, Type};
-use crate::{DataInstForm, qptr};
 
 pub fn lower_from_spv_ptrs(module: &mut Module, layout_config: &qptr::LayoutConfig) {
     let cx = &module.cx();
@@ -15,7 +15,6 @@ pub fn lower_from_spv_ptrs(module: &mut Module, layout_config: &qptr::LayoutConf
 
             seen_types: FxIndexSet::default(),
             seen_consts: FxIndexSet::default(),
-            seen_data_inst_forms: FxIndexSet::default(),
             seen_global_vars: FxIndexSet::default(),
             seen_funcs: FxIndexSet::default(),
         };
@@ -50,7 +49,6 @@ pub fn lift_to_spv_ptrs(module: &mut Module, layout_config: &qptr::LayoutConfig)
 
             seen_types: FxIndexSet::default(),
             seen_consts: FxIndexSet::default(),
-            seen_data_inst_forms: FxIndexSet::default(),
             seen_global_vars: FxIndexSet::default(),
             seen_funcs: FxIndexSet::default(),
         };
@@ -75,7 +73,6 @@ struct ReachableUseCollector<'a> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
     seen_types: FxIndexSet<Type>,
     seen_consts: FxIndexSet<Const>,
-    seen_data_inst_forms: FxIndexSet<DataInstForm>,
     seen_global_vars: FxIndexSet<GlobalVar>,
     seen_funcs: FxIndexSet<Func>,
 }
@@ -95,11 +92,6 @@ impl Visitor<'_> for ReachableUseCollector<'_> {
             self.visit_const_def(&self.cx[ct]);
         }
     }
-    fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) {
-        if self.seen_data_inst_forms.insert(data_inst_form) {
-            self.visit_data_inst_form_def(&self.cx[data_inst_form]);
-        }
-    }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
         if self.seen_global_vars.insert(gv) {

diff --git a/src/print/mod.rs b/src/print/mod.rs
@@ -25,11 +25,11 @@ use crate::qptr::{self, QPtrAttr, QPtrMemUsage, QPtrMemUsageKind, QPtrOp, QPtrUs
 use crate::visit::{InnerVisit, Visit, Visitor};
 use crate::{
     AddrSpace, Attr, AttrSet, AttrSetDef, Const, ConstDef, ConstKind, Context, DataInst,
-    DataInstDef, DataInstForm, DataInstFormDef, DataInstKind, DbgSrcLoc, DeclDef, Diag, DiagLevel,
-    DiagMsgPart, EntityListIter, ExportKey, Exportee, Func, FuncDecl, FuncParam, FxIndexMap,
-    FxIndexSet, GlobalVar, GlobalVarDecl, GlobalVarDefBody, Import, Module, ModuleDebugInfo,
-    ModuleDialect, Node, NodeDef, NodeKind, NodeOutputDecl, OrdAssertEq, Region, RegionDef,
-    RegionInputDecl, SelectionKind, Type, TypeDef, TypeKind, TypeOrConst, Value, cfg, spv,
+    DataInstDef, DataInstKind, DbgSrcLoc, DeclDef, Diag, DiagLevel, DiagMsgPart, EntityListIter,
+    ExportKey, Exportee, Func, FuncDecl, FuncParam, FxIndexMap, FxIndexSet, GlobalVar,
+    GlobalVarDecl, GlobalVarDefBody, Import, Module, ModuleDebugInfo, ModuleDialect, Node, NodeDef,
+    NodeKind, NodeOutputDecl, OrdAssertEq, Region, RegionDef, RegionInputDecl, SelectionKind, Type,
+    TypeDef, TypeKind, TypeOrConst, Value, cfg, spv,
 };
 use arrayvec::ArrayVec;
 use itertools::Either;
@@ -463,12 +463,6 @@ impl<'a> Visitor<'a> for Plan<'a> {
     fn visit_const_use(&mut self, ct: Const) {
         self.use_interned(CxInterned::Const(ct));
     }
-    fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) {
-        // NOTE(eddyb) this contains no deduplication because each `DataInstDef`
-        // will be pretty-printed separately, so everything in its `form` also
-        // needs to get use counts incremented separately per-`DataInstDef`.
-        self.visit_data_inst_form_def(&self.cx[data_inst_form]);
-    }
 
     fn visit_global_var_use(&mut self, gv: GlobalVar) {
         if let Some(module) = self.current_module {
@@ -974,7 +968,7 @@ impl<'a> Printer<'a> {
                                     None,
                                 );
                                 let inst_def = func_at_inst.def();
-                                if cx[inst_def.form].output_type.is_some() {
+                                if inst_def.output_type.is_some() {
                                     define(
                                         Use::DataInstOutput(func_at_inst.position),
                                         Some(inst_def.attrs),
@@ -1000,7 +994,6 @@ impl<'a> Printer<'a> {
                     fn visit_attr_set_use(&mut self, _: AttrSet) {}
                     fn visit_type_use(&mut self, _: Type) {}
                     fn visit_const_use(&mut self, _: Const) {}
-                    fn visit_data_inst_form_use(&mut self, _: DataInstForm) {}
                     fn visit_global_var_use(&mut self, _: GlobalVar) {}
                     fn visit_func_use(&mut self, _: Func) {}
 
@@ -3153,12 +3146,10 @@ impl Print for NodeOutputDecl {
 impl Print for FuncAt<'_, DataInst> {
     type Output = pretty::Fragment;
     fn print(&self, printer: &Printer<'_>) -> pretty::Fragment {
-        let DataInstDef { attrs, form, inputs } = self.def();
+        let DataInstDef { attrs, kind, inputs, output_type } = self.def();
 
         let attrs = attrs.print(printer);
 
-        let DataInstFormDef { kind, output_type } = &printer.cx[*form];
-
         let mut output_use_to_print_as_lhs =
             output_type.map(|_| Use::DataInstOutput(self.position));
 

diff --git a/src/qptr/analyze.rs b/src/qptr/analyze.rs
@@ -7,9 +7,9 @@ use super::{QPtrAttr, QPtrMemUsage, QPtrOp, QPtrUsage, shapes};
 use crate::func_at::FuncAt;
 use crate::visit::{InnerVisit, Visitor};
 use crate::{
-    AddrSpace, Attr, AttrSet, AttrSetDef, Const, ConstKind, Context, DataInst, DataInstForm,
-    DataInstKind, DeclDef, Diag, EntityList, ExportKey, Exportee, Func, FxIndexMap, GlobalVar,
-    Module, Node, NodeKind, OrdAssertEq, Type, TypeKind, Value,
+    AddrSpace, Attr, AttrSet, AttrSetDef, Const, ConstKind, Context, DataInst, DataInstKind,
+    DeclDef, Diag, EntityList, ExportKey, Exportee, Func, FxIndexMap, GlobalVar, Module, Node,
+    NodeKind, OrdAssertEq, Type, TypeKind, Value,
 };
 use itertools::Either;
 use rustc_hash::FxHashMap;
@@ -866,7 +866,6 @@ impl<'a> InferUsage<'a> {
             for func_at_inst in func_def_body.at(insts).into_iter().rev() {
                 let data_inst = func_at_inst.position;
                 let data_inst_def = func_at_inst.def();
-                let data_inst_form_def = &cx[data_inst_def.form];
                 let output_usage = data_inst_output_usages.remove(&data_inst).flatten();
 
                 let mut generate_usage = |this: &mut Self, ptr: Value, new_usage| {
@@ -904,7 +903,7 @@ impl<'a> InferUsage<'a> {
                         None => new_usage,
                     });
                 };
-                match &data_inst_form_def.kind {
+                match &data_inst_def.kind {
                     &DataInstKind::FuncCall(callee) => {
                         match self.infer_usage_in_func(module, callee) {
                             FuncInferUsageState::Complete(callee_results) => {
@@ -925,7 +924,7 @@ impl<'a> InferUsage<'a> {
                                 ));
                             }
                         };
-                        if data_inst_form_def.output_type.map_or(false, is_qptr) {
+                        if data_inst_def.output_type.map_or(false, is_qptr) {
                             if let Some(usage) = output_usage {
                                 usage_or_err_attrs_to_attach
                                     .push((Value::DataInstOutput(data_inst), usage));
@@ -1108,7 +1107,7 @@ impl<'a> InferUsage<'a> {
                     }
                     DataInstKind::QPtr(op @ (QPtrOp::Load | QPtrOp::Store)) => {
                         let (op_name, access_type) = match op {
-                            QPtrOp::Load => ("Load", data_inst_form_def.output_type.unwrap()),
+                            QPtrOp::Load => ("Load", data_inst_def.output_type.unwrap()),
                             QPtrOp::Store => {
                                 ("Store", func_at_inst.at(data_inst_def.inputs[1]).type_of(&cx))
                             }
@@ -1250,7 +1249,6 @@ impl Visitor<'_> for CollectAllDataInsts {
     fn visit_attr_set_use(&mut self, _: AttrSet) {}
     fn visit_type_use(&mut self, _: Type) {}
     fn visit_const_use(&mut self, _: Const) {}
-    fn visit_data_inst_form_use(&mut self, _: DataInstForm) {}
     fn visit_global_var_use(&mut self, _: GlobalVar) {}
     fn visit_func_use(&mut self, _: Func) {}