From 1a8084e115dde162825a1f99dce0f04a28943212 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Wed, 18 Aug 2021 19:05:59 +0300 Subject: [PATCH] specializer: allow unused fully-constrained interface variables. --- .../src/linker/specializer.rs | 36 ++++++++++++++++++- tests/ui/dis/issue-723-indirect-input.rs | 17 +++++++++ tests/ui/dis/issue-723-indirect-input.stderr | 20 +++++++++++ tests/ui/dis/issue-723-output.rs | 22 ++++++++++++ tests/ui/dis/issue-723-output.stderr | 17 +++++++++ 5 files changed, 111 insertions(+), 1 deletion(-) create mode 100644 tests/ui/dis/issue-723-indirect-input.rs create mode 100644 tests/ui/dis/issue-723-indirect-input.stderr create mode 100644 tests/ui/dis/issue-723-output.rs create mode 100644 tests/ui/dis/issue-723-output.stderr diff --git a/crates/rustc_codegen_spirv/src/linker/specializer.rs b/crates/rustc_codegen_spirv/src/linker/specializer.rs index e133c46592..7b27fb0f8c 100644 --- a/crates/rustc_codegen_spirv/src/linker/specializer.rs +++ b/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). diff --git a/tests/ui/dis/issue-723-indirect-input.rs b/tests/ui/dis/issue-723-indirect-input.rs new file mode 100644 index 0000000000..6995b37e0b --- /dev/null +++ b/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]) {} diff --git a/tests/ui/dis/issue-723-indirect-input.stderr b/tests/ui/dis/issue-723-indirect-input.stderr new file mode 100644 index 0000000000..f43153fa79 --- /dev/null +++ b/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 diff --git a/tests/ui/dis/issue-723-output.rs b/tests/ui/dis/issue-723-output.rs new file mode 100644 index 0000000000..ca1f6d2068 --- /dev/null +++ b/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) {} diff --git a/tests/ui/dis/issue-723-output.stderr b/tests/ui/dis/issue-723-output.stderr new file mode 100644 index 0000000000..d98cfff1ec --- /dev/null +++ b/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