Browse Source

Move the check for Subpass into to uses (#5252)

We currently check if the shader is a pixel shader when we see a
declaration of a SubpassInput type. However, it is possible that there
are multiple shader in the same hlsl file, and that the SubpassInput is
only used in the pixel shader.

So that we do not get an error in this case, I moved the check for the
shader type to the use of the variable. I only has a single member
function, so identifying uses of that member function is a good proxy.

Fixes #4704
Steven Perron 2 years ago
parent
commit
adc0363539

+ 13 - 6
tools/clang/lib/SPIRV/SpirvEmitter.cpp

@@ -1520,12 +1520,6 @@ bool SpirvEmitter::validateVKAttributes(const NamedDecl *decl) {
   }
   }
 
 
   if (decl->getAttr<VKInputAttachmentIndexAttr>()) {
   if (decl->getAttr<VKInputAttachmentIndexAttr>()) {
-    if (!spvContext.isPS()) {
-      emitError("SubpassInput(MS) only allowed in pixel shader",
-                decl->getLocation());
-      success = false;
-    }
-
     if (!decl->isExternallyVisible()) {
     if (!decl->isExternallyVisible()) {
       emitError("SubpassInput(MS) must be externally visible",
       emitError("SubpassInput(MS) must be externally visible",
                 decl->getLocation());
                 decl->getLocation());
@@ -1795,6 +1789,14 @@ void SpirvEmitter::doVarDecl(const VarDecl *decl) {
     return;
     return;
   }
   }
 
 
+  if (decl->getAttr<VKInputAttachmentIndexAttr>()) {
+    if (!spvContext.isPS()) {
+      // SubpassInput(MS) variables are only allowed in pixel shaders. In this
+      // case, we avoid create the declaration because it should not be used.
+      return;
+    }
+  }
+
   SpirvVariable *var = nullptr;
   SpirvVariable *var = nullptr;
 
 
   // The contents in externally visible variables can be updated via the
   // The contents in externally visible variables can be updated via the
@@ -3860,6 +3862,11 @@ SpirvEmitter::processGetSamplePosition(const CXXMemberCallExpr *expr) {
 
 
 SpirvInstruction *
 SpirvInstruction *
 SpirvEmitter::processSubpassLoad(const CXXMemberCallExpr *expr) {
 SpirvEmitter::processSubpassLoad(const CXXMemberCallExpr *expr) {
+  if (!spvContext.isPS()) {
+    emitError("SubpassInput(MS) only allowed in pixel shader",
+              expr->getExprLoc());
+    return nullptr;
+  }
   const auto *object = expr->getImplicitObjectArgument()->IgnoreParens();
   const auto *object = expr->getImplicitObjectArgument()->IgnoreParens();
   SpirvInstruction *sample =
   SpirvInstruction *sample =
       expr->getNumArgs() == 1 ? doExpr(expr->getArg(0)) : nullptr;
       expr->getNumArgs() == 1 ? doExpr(expr->getArg(0)) : nullptr;

+ 12 - 0
tools/clang/test/CodeGenSPIRV/vk.subpass-input.shader.type.error.hlsl

@@ -0,0 +1,12 @@
+// Test that an error is emitted when a SubpassInput variable is used in
+// something other than a pixel shader.
+
+// RUN: %dxc -T cs_6_0 -E CsTest
+
+[[vk::input_attachment_index (0)]] SubpassInput<float4> subInput;
+[numthreads (8,1,1)]
+void CsTest() {
+	subInput.SubpassLoad();
+}
+
+// CHECK: :9:2: error: SubpassInput(MS) only allowed in pixel shader

+ 22 - 0
tools/clang/test/CodeGenSPIRV/vk.subpass-input.unused.hlsl

@@ -0,0 +1,22 @@
+// Test that the we can correctly compile the compute shader when the
+// subpass input is used in  PsSubpassTest, but not in the compute shader.
+
+// RUN: %dxc -T cs_6_0 -E CsTest
+
+// The subpass input variable should not be in the module.
+// CHECK-NOT: %subInput
+
+[[vk::input_attachment_index (0)]] SubpassInput<float4> subInput;
+
+float4 PsSubpassTest() : SV_TARGET
+{
+	return subInput.SubpassLoad();
+}
+
+float4 f()
+{
+	return subInput.SubpassLoad();
+}
+
+[numthreads (8,1,1)]
+void CsTest() {}

+ 6 - 0
tools/clang/unittests/SPIRV/CodeGenSpirvTest.cpp

@@ -2464,6 +2464,9 @@ TEST_F(FileTest, VulkanLayoutRegisterCError) {
 }
 }
 
 
 TEST_F(FileTest, VulkanSubpassInput) { runFileTest("vk.subpass-input.hlsl"); }
 TEST_F(FileTest, VulkanSubpassInput) { runFileTest("vk.subpass-input.hlsl"); }
+TEST_F(FileTest, VulkanSubpassInputUnused) {
+  runFileTest("vk.subpass-input.unused.hlsl");
+}
 TEST_F(FileTest, VulkanSubpassInputBinding) {
 TEST_F(FileTest, VulkanSubpassInputBinding) {
   runFileTest("vk.subpass-input.binding.hlsl");
   runFileTest("vk.subpass-input.binding.hlsl");
 }
 }
@@ -2476,6 +2479,9 @@ TEST_F(FileTest, VulkanSubpassInputError2) {
 TEST_F(FileTest, VulkanSubpassInputError3) {
 TEST_F(FileTest, VulkanSubpassInputError3) {
   runFileTest("vk.subpass-input.static.error.hlsl", Expect::Failure);
   runFileTest("vk.subpass-input.static.error.hlsl", Expect::Failure);
 }
 }
+TEST_F(FileTest, VulkanSubpassInputError4) {
+  runFileTest("vk.subpass-input.shader.type.error.hlsl", Expect::Failure);
+}
 
 
 TEST_F(FileTest, NonFpColMajorError) {
 TEST_F(FileTest, NonFpColMajorError) {
   runFileTest("vk.layout.non-fp-matrix.error.hlsl", Expect::Failure);
   runFileTest("vk.layout.non-fp-matrix.error.hlsl", Expect::Failure);