Intern, as DataInstForm, the kind and output_type fields of `Da…

…taInstDef`. (#28) This should, in theory, allow reusing one `DataInst` "form" (or "template") for many `DataInst`s, with only runtime inputs (and/or attributes) differing between their "instances". While there is no good reason to have the extra indirection today (and the performance impact seems to fall within noise), there is a refactor I want to try (which would require multiple outputs per `DataInst`, and that would add to the growing size of `DataInstDef`, before this interning change). ~~Will keep this PR as draft until I can confirm that refactor is successful.~~
EmbarkStudios · Jun 7, 2023 · c32ae33 · c32ae33
2 parents 4c07598 + a4681a1
commit c32ae33
Show file tree

Hide file tree

Showing 18 changed files with 341 additions and 139 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased] - ReleaseDate
 
 ### Changed 🛠
+- [PR#28](https://github.com/EmbarkStudios/spirt/pull/28) moved two `DataInstDef`
+  fields (`kind` and `output_type`) to `DataInstForm`, a new interned type
 - [PR#30](https://github.com/EmbarkStudios/spirt/pull/30) replaced the old `spv-lower-dump`
   example (which only dumped plaintext, not HTML) with a more useful `spv-lower-print` one
 

diff --git a/src/context.rs b/src/context.rs
@@ -900,6 +900,7 @@ interners! {
         crate::AttrSetDef,
         crate::TypeDef,
         crate::ConstDef,
+        crate::DataInstFormDef,
     }
 
     // FIXME(eddyb) consider a more uniform naming scheme than the combination
@@ -908,6 +909,7 @@ 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
@@ -117,7 +117,7 @@ impl FuncAt<'_, Value> {
                 control_node,
                 output_idx,
             } => self.at(control_node).def().outputs[output_idx as usize].ty,
-            Value::DataInstOutput(inst) => self.at(inst).def().output_type.unwrap(),
+            Value::DataInstOutput(inst) => cx[self.at(inst).def().form].output_type.unwrap(),
         }
     }
 }

diff --git a/src/lib.rs b/src/lib.rs
@@ -870,15 +870,31 @@ pub use context::DataInst;
 pub struct DataInstDef {
     pub attrs: AttrSet,
 
-    pub kind: DataInstKind,
-
-    pub output_type: Option<Type>,
+    pub form: DataInstForm,
 
     // FIXME(eddyb) change the inline size of this to fit most instructions.
     pub inputs: SmallVec<[Value; 2]>,
 }
 
-#[derive(Clone, PartialEq, Eq)]
+/// 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>,
+}
+
+#[derive(Clone, PartialEq, Eq, Hash)]
 pub enum DataInstKind {
     // FIXME(eddyb) try to split this into recursive and non-recursive calls,
     // to avoid needing special handling for recursion where it's impossible.

diff --git a/src/passes/legalize.rs b/src/passes/legalize.rs
@@ -1,5 +1,7 @@
 use crate::visit::{InnerVisit, Visitor};
-use crate::{cfg, AttrSet, Const, Context, DeclDef, Func, FxIndexSet, GlobalVar, Module, Type};
+use crate::{
+    cfg, AttrSet, Const, Context, DataInstForm, DeclDef, Func, FxIndexSet, GlobalVar, Module, Type,
+};
 
 /// Apply the [`cfg::Structurizer`] algorithm to all function definitions in `module`.
 pub fn structurize_func_cfgs(module: &mut Module) {
@@ -12,6 +14,7 @@ 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(),
     };
@@ -34,13 +37,16 @@ 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>,
 }
 
 impl Visitor<'_> for ReachableUseCollector<'_> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
-    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {}
+    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {
+        // FIXME(eddyb) if `AttrSet`s are ignored, why not `Type`s too?
+    }
     fn visit_type_use(&mut self, ty: Type) {
         if self.seen_types.insert(ty) {
             self.visit_type_def(&self.cx[ty]);
@@ -51,6 +57,11 @@ 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, DeclDef, ExportKey, Exportee, Func, FxIndexSet, GlobalVar, Import,
-    Module, Type,
+    AttrSet, Const, Context, DataInstForm, DeclDef, ExportKey, Exportee, Func, FxIndexSet,
+    GlobalVar, Import, Module, Type,
 };
 use rustc_hash::{FxHashMap, FxHashSet};
 use std::collections::VecDeque;
@@ -33,6 +33,7 @@ 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(),
     };
@@ -60,13 +61,16 @@ 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>,
 }
 
 impl Visitor<'_> for LiveExportCollector<'_> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
-    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {}
+    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {
+        // FIXME(eddyb) if `AttrSet`s are ignored, why not `Type`s too?
+    }
     fn visit_type_use(&mut self, ty: Type) {
         if self.seen_types.insert(ty) {
             self.visit_type_def(&self.cx[ty]);
@@ -77,6 +81,11 @@ 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) {
@@ -119,6 +128,7 @@ 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(),
         };
@@ -134,6 +144,7 @@ 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(),
@@ -170,13 +181,16 @@ 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>,
 }
 
 impl Visitor<'_> for ImportResolutionCollector<'_> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
-    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {}
+    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {
+        // FIXME(eddyb) if `AttrSet`s are ignored, why not `Type`s too?
+    }
     fn visit_type_use(&mut self, ty: Type) {
         if self.seen_types.insert(ty) {
             self.visit_type_def(&self.cx[ty]);
@@ -187,6 +201,11 @@ 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) {
@@ -233,6 +252,7 @@ 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>>,
@@ -261,6 +281,20 @@ 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,7 +1,7 @@
 //! [`QPtr`](crate::TypeCtor::QPtr) transforms.
 
-use crate::qptr;
 use crate::visit::{InnerVisit, Visitor};
+use crate::{qptr, DataInstForm};
 use crate::{AttrSet, Const, Context, Func, FxIndexSet, GlobalVar, Module, Type};
 
 pub fn lower_from_spv_ptrs(module: &mut Module, layout_config: &qptr::LayoutConfig) {
@@ -15,6 +15,7 @@ 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(),
         };
@@ -49,6 +50,7 @@ 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(),
         };
@@ -73,13 +75,16 @@ 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>,
 }
 
 impl Visitor<'_> for ReachableUseCollector<'_> {
     // FIXME(eddyb) build some automation to avoid ever repeating these.
-    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {}
+    fn visit_attr_set_use(&mut self, _attrs: AttrSet) {
+        // FIXME(eddyb) if `AttrSet`s are ignored, why not `Type`s too?
+    }
     fn visit_type_use(&mut self, ty: Type) {
         if self.seen_types.insert(ty) {
             self.visit_type_def(&self.cx[ty]);
@@ -90,6 +95,11 @@ 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
@@ -26,10 +26,11 @@ use crate::visit::{DynVisit, InnerVisit, Visit, Visitor};
 use crate::{
     cfg, spv, AddrSpace, Attr, AttrSet, AttrSetDef, Const, ConstCtor, ConstDef, Context,
     ControlNode, ControlNodeDef, ControlNodeKind, ControlNodeOutputDecl, ControlRegion,
-    ControlRegionDef, ControlRegionInputDecl, DataInst, DataInstDef, DataInstKind, DeclDef, Diag,
-    DiagLevel, DiagMsgPart, EntityListIter, ExportKey, Exportee, Func, FuncDecl, FuncParam,
-    FxIndexMap, GlobalVar, GlobalVarDecl, GlobalVarDefBody, Import, Module, ModuleDebugInfo,
-    ModuleDialect, OrdAssertEq, SelectionKind, Type, TypeCtor, TypeCtorArg, TypeDef, Value,
+    ControlRegionDef, ControlRegionInputDecl, DataInst, DataInstDef, DataInstForm, DataInstFormDef,
+    DataInstKind, DeclDef, Diag, DiagLevel, DiagMsgPart, EntityListIter, ExportKey, Exportee, Func,
+    FuncDecl, FuncParam, FxIndexMap, GlobalVar, GlobalVarDecl, GlobalVarDefBody, Import, Module,
+    ModuleDebugInfo, ModuleDialect, OrdAssertEq, SelectionKind, Type, TypeCtor, TypeCtorArg,
+    TypeDef, Value,
 };
 use rustc_hash::FxHashMap;
 use smallvec::SmallVec;
@@ -385,6 +386,12 @@ 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 {
@@ -785,7 +792,7 @@ impl<'a> Printer<'a> {
                         if let ControlNodeKind::Block { insts } = *kind {
                             for func_at_inst in func_def_body.at(insts) {
                                 define(Use::AlignmentAnchorForDataInst(func_at_inst.position));
-                                if func_at_inst.def().output_type.is_some() {
+                                if cx[func_at_inst.def().form].output_type.is_some() {
                                     define(Use::DataInstOutput(func_at_inst.position));
                                 }
                             }
@@ -808,6 +815,7 @@ 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) {}
 
@@ -2687,7 +2695,7 @@ impl Print for FuncAt<'_, ControlNode> {
                                 data_inst_attrs_and_def.def_without_name,
                             ]);
                             data_inst_attrs_and_def.insert_name_before_def(
-                                if data_inst_def.output_type.is_none() {
+                                if printer.cx[data_inst_def.form].output_type.is_none() {
                                     pretty::Fragment::default()
                                 } else {
                                     pretty::Fragment::new([
@@ -2834,13 +2842,14 @@ impl Print for DataInstDef {
     fn print(&self, printer: &Printer<'_>) -> AttrsAndDef {
         let Self {
             attrs,
-            kind,
-            output_type,
+            form,
             inputs,
         } = self;
 
         let attrs = attrs.print(printer);
 
+        let DataInstFormDef { kind, output_type } = &printer.cx[*form];
+
         let header = match kind {
             &DataInstKind::FuncCall(func) => pretty::Fragment::new([
                 printer.declarative_keyword_style().apply("call").into(),