From 1f162e2230158e8b3d6c70ea31817c1a90a2de22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Fri, 8 Mar 2024 12:48:54 +0100 Subject: [PATCH] [SPIR-V] Block semantic annotation reuse for input (#6396) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MSDN spec is not very clear regarding input parameter aliasing, BUT DXIL & MS agrees (See #3737) using the same semantic annotation twice should be disallowed. DXIL currently disallows it for most, and due to a bug, allows some compute related semantics to be aliased. SPIR-V did allowed aliasing, but it was a implemented as a hack causing typing issues if the type changed between the 2 parameters using the same semantic annotation. To align more closely with DXIL, and the "spec", we are now disallowing all semantic attribute reuse for input parameters. Fixes #3737 Signed-off-by: Nathan Gauër --- tools/clang/lib/SPIRV/DeclResultIdMapper.cpp | 19 +++++++------------ .../test/CodeGenSPIRV/decoration.unique.hlsl | 10 ---------- .../spirv.interface.alias-builtin.hlsl | 13 ++----------- 3 files changed, 9 insertions(+), 33 deletions(-) delete mode 100644 tools/clang/test/CodeGenSPIRV/decoration.unique.hlsl diff --git a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp index 656d0ee16e..1603cc6046 100644 --- a/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp +++ b/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp @@ -1836,7 +1836,6 @@ DeclResultIdMapper::collectStageVars(SpirvFunction *entryPoint) const { for (auto var : glPerVertex.getStageOutVars()) vars.push_back(var); - llvm::DenseSet seenVars; for (const auto &var : stageVars) { // We must collect stage variables that are included in entryPoint and stage // variables that are not included in any specific entryPoint i.e., @@ -1847,10 +1846,7 @@ DeclResultIdMapper::collectStageVars(SpirvFunction *entryPoint) const { auto *instr = var.getSpirvInstr(); if (instr->getStorageClass() == spv::StorageClass::Private) continue; - if (seenVars.count(instr) == 0) { - vars.push_back(instr); - seenVars.insert(instr); - } + vars.push_back(instr); } return vars; @@ -2043,23 +2039,22 @@ bool DeclResultIdMapper::checkSemanticDuplication(bool forInput) { continue; } - // Allow builtin variables to alias each other. We already have uniqify - // mechanism in SpirvBuilder. - if (var.isSpirvBuitin()) - continue; - if (forInput && var.getSigPoint()->IsInput()) { bool insertionSuccess = insertSeenSemanticsForEntryPointIfNotExist( &seenSemanticsForEntryPoints, var.getEntryPoint(), s); if (!insertionSuccess) { - emitError("input semantic '%0' used more than once", {}) << s; + emitError("input semantic '%0' used more than once", + var.getSemanticInfo().loc) + << s; success = false; } } else if (!forInput && var.getSigPoint()->IsOutput()) { bool insertionSuccess = insertSeenSemanticsForEntryPointIfNotExist( &seenSemanticsForEntryPoints, var.getEntryPoint(), s); if (!insertionSuccess) { - emitError("output semantic '%0' used more than once", {}) << s; + emitError("output semantic '%0' used more than once", + var.getSemanticInfo().loc) + << s; success = false; } } diff --git a/tools/clang/test/CodeGenSPIRV/decoration.unique.hlsl b/tools/clang/test/CodeGenSPIRV/decoration.unique.hlsl deleted file mode 100644 index 895bacd0d1..0000000000 --- a/tools/clang/test/CodeGenSPIRV/decoration.unique.hlsl +++ /dev/null @@ -1,10 +0,0 @@ -// RUN: %dxc -T ps_6_2 -E main -fspv-reflect -fcgl %s -spirv | FileCheck %s - -// Make sure the same decoration is not applied twice. -// -// CHECK: OpDecorateString %gl_FragCoord UserSemantic "SV_POSITION" -// CHECK-NOT: OpDecorateString %gl_FragCoord UserSemantic "SV_POSITION" - -float4 main(float4 pix_pos : SV_POSITION, float4 pix_pos2: SV_POSITION): SV_Target { - return 0; -} diff --git a/tools/clang/test/CodeGenSPIRV/spirv.interface.alias-builtin.hlsl b/tools/clang/test/CodeGenSPIRV/spirv.interface.alias-builtin.hlsl index 214fa0886e..f826b78389 100644 --- a/tools/clang/test/CodeGenSPIRV/spirv.interface.alias-builtin.hlsl +++ b/tools/clang/test/CodeGenSPIRV/spirv.interface.alias-builtin.hlsl @@ -1,21 +1,12 @@ -// RUN: %dxc -T ps_6_0 -E main -fcgl %s -spirv | FileCheck %s - -// CHECK: OpEntryPoint Fragment %main "main" %gl_FragCoord %out_var_SV_Target - -// CHECK: %gl_FragCoord = OpVariable %_ptr_Input_v4float Input -// CHECK-NOT: {{%[0-9]+}} = OpVariable %_ptr_Input_v4float Input +// RUN: not %dxc -T ps_6_0 -E main %s -spirv 2>&1 | FileCheck %s struct PSInput { float4 a : SV_Position; float4 b : SV_Position; +// CHECK: :5:14: error: input semantic 'SV_Position' used more than once }; -// CHECK: [[a:%[0-9]+]] = OpLoad %v4float %gl_FragCoord -// CHECK-NEXT: [[b:%[0-9]+]] = OpLoad %v4float %gl_FragCoord -// CHECK-NEXT: {{%[0-9]+}} = OpCompositeConstruct %PSInput [[a]] [[b]] - float4 main(PSInput input) : SV_Target { return input.a + input.b; } -