specializer: allow unused fully-constrained interface variables.

Author: eddyb

crates/rustc_codegen_spirv/src/linker/specializer.rs

@@ -51,7 +51,7 @@
 
 use crate::linker::ipo::CallGraph;
 use crate::spirv_type_constraints::{self, InstSig, StorageClassPat, TyListPat, TyPat};
-use indexmap::IndexMap;
+use indexmap::{IndexMap, IndexSet};
 use rspirv::dr::{Builder, Function, Instruction, Module, Operand};
 use rspirv::spirv::{Op, StorageClass, Word};
 use rustc_data_structures::captures::Captures;
@@ -138,6 +138,35 @@ pub fn specialize(module: Module, specialization: impl Specialization) -> Module
 
     specializer.collect_generics(&module);
 
+    // "Generic" module-scoped variables can be fully constrained to the point
+    // where we could theoretically always add an instance for them, in order
+    // to preserve them, even if they would appear to otherwise be unused.
+    // We do this here for fully-constrained variables used by `OpEntryPoint`s,
+    // in order to avoid a failure in `Expander::expand_module` (see #723).
+    let mut interface_concrete_instances = IndexSet::new();
+    for inst in &module.entry_points {
+        for interface_operand in &inst.operands[3..] {
+            let interface_id = interface_operand.unwrap_id_ref();
+            if let Some(generic) = specializer.generics.get(&interface_id) {
+                if let Some(param_values) = &generic.param_values {
+                    if param_values.iter().all(|v| matches!(v, Value::Known(_))) {
+                        interface_concrete_instances.insert(Instance {
+                            generic_id: interface_id,
+                            generic_args: param_values
+                                .iter()
+                                .copied()
+                                .map(|v| match v {
+                                    Value::Known(v) => v,
+                                    _ => unreachable!(),
+                                })
+                                .collect(),
+                        });
+                    }
+                }
+            }
+        }
+    }
+
     let call_graph = CallGraph::collect(&module);
     let mut non_generic_replacements = vec![];
     for func_idx in call_graph.post_order() {
@@ -148,6 +177,11 @@ pub fn specialize(module: Module, specialization: impl Specialization) -> Module
 
     let mut expander = Expander::new(&specializer, module);
 
+    // See comment above on the loop collecting `interface_concrete_instances`.
+    for interface_instance in interface_concrete_instances {
+        expander.alloc_instance_id(interface_instance);
+    }
+
     // For non-"generic" functions, we can apply `replacements` right away,
     // though not before finishing inference for all functions first
     // (because `expander` needs to borrow `specializer` immutably).

tests/ui/dis/issue-723-indirect-input.rs

@@ -0,0 +1,17 @@
+// Test that interface (global) `OpVariable`s mentioned by `OpEntryPoint` don't
+// have to be used by the shader, for storage class inference to succeed.
+
+// NOTE(eddyb) this test will likely become useless (won't fail without the fix)
+// once we start doing the copy out of the `Input` and into a `Function`-scoped
+// `OpVariable` (see #731), that's why there is another `issue-723-*` test.
+
+// build-pass
+// compile-flags: -C llvm-args=--disassemble-globals
+// normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> ""
+// normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> ""
+// normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple"
+
+use spirv_std as _;
+
+#[spirv(fragment)]
+pub fn main(/* unused Input */ _: [f32; 3]) {}

tests/ui/dis/issue-723-indirect-input.stderr

@@ -0,0 +1,20 @@
+OpCapability Float64
+OpCapability Int16
+OpCapability Int64
+OpCapability Int8
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint Fragment %1 "main" %2
+OpExecutionMode %1 OriginUpperLeft
+%3 = OpString "$OPSTRING_FILENAME/issue-723-indirect-input.rs"
+OpName %4 "issue_723_indirect_input::main"
+OpDecorate %5 ArrayStride 4
+OpDecorate %2 Location 0
+%6 = OpTypeVoid
+%7 = OpTypeFloat 32
+%8 = OpTypeInt 32 0
+%9 = OpConstant  %8  3
+%5 = OpTypeArray %7 %9
+%10 = OpTypePointer Input %5
+%11 = OpTypeFunction %6
+%2 = OpVariable  %10  Input

tests/ui/dis/issue-723-output.rs

@@ -0,0 +1,22 @@
+// Test that interface (global) `OpVariable`s mentioned by `OpEntryPoint` don't
+// have to be used by the shader, for storage class inference to succeed.
+
+// NOTE(eddyb) this relies on two subtleties (in order to fail without the fix):
+// * disabling debuginfo (to prevent `%x.dbg.spill` stack slot generation)
+//   * this could be alleviated in the future if we clean up how debuginfo
+//     is handled in `rustc_codegen_ssa`, to not assume LLVM limitations
+//   * it probably needs to stay like this, to ensure the parameter is unused
+// * `Output`s being handled with `&mut`, instead of by-value returns
+//   * if this changes, this test will likely need >=1.4 SPIR-V, which supports
+//     all interface `OpVariables` in `OpEntryPoint`, not just `Input`/`Output`
+
+// build-pass
+// compile-flags: -C debuginfo=0 -C llvm-args=--disassemble-globals
+// normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> ""
+// normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> ""
+// normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple"
+
+use spirv_std as _;
+
+#[spirv(fragment)]
+pub fn main(/* unused Output */ _: &mut glam::Vec4) {}

tests/ui/dis/issue-723-output.stderr

@@ -0,0 +1,17 @@
+OpCapability Float64
+OpCapability Int16
+OpCapability Int64
+OpCapability Int8
+OpCapability Shader
+OpMemoryModel Logical Simple
+OpEntryPoint Fragment %1 "main" %2
+OpExecutionMode %1 OriginUpperLeft
+%3 = OpString "$OPSTRING_FILENAME/issue-723-output.rs"
+OpName %4 "issue_723_output::main"
+OpDecorate %2 Location 0
+%5 = OpTypeVoid
+%6 = OpTypeFloat 32
+%7 = OpTypeVector %6 4
+%8 = OpTypePointer Output %7
+%9 = OpTypeFunction %5
+%2 = OpVariable  %8  Output