[spirv] Fix push constant defined from anonymous struct (#1223)

When skipping decls not externally visible, we should make sure
that they are in the translation unit or a namespace, which means
they are for $Globals cbuffer. For other cases, we should not skip.

Fixes https://github.com/Microsoft/DirectXShaderCompiler/issues/1221

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -471,11 +471,10 @@ uint32_t DeclResultIdMapper::createStructOrStructArrayVarOfExplicitLayout(
     llvm::StringRef varName) {
   // cbuffers are translated into OpTypeStruct with Block decoration.
   // tbuffers are translated into OpTypeStruct with BufferBlock decoration.
-  // PushConstants are translated into OpTypeStruct with Block decoration.
+  // Push constants are translated into OpTypeStruct with Block decoration.
   //
-  // Both cbuffers and tbuffers have the SPIR-V Uniform storage class. cbuffers
-  // follow GLSL std140 layout rules, and tbuffers follow GLSL std430 layout
-  // rules. PushConstants follow GLSL std430 layout rules.
+  // Both cbuffers and tbuffers have the SPIR-V Uniform storage class.
+  // Push constants have the SPIR-V PushConstant storage class.
 
   const bool forCBuffer = usageKind == ContextUsageKind::CBuffer;
   const bool forTBuffer = usageKind == ContextUsageKind::TBuffer;

tools/clang/lib/SPIRV/TypeTranslator.cpp

@@ -1133,41 +1133,52 @@ TypeTranslator::getCapabilityForStorageImageReadWrite(QualType type) {
 
 bool TypeTranslator::shouldSkipInStructLayout(const Decl *decl) {
   // Ignore implicit generated struct declarations/constructors/destructors
+  if (decl->isImplicit())
+    return true;
   // Ignore embedded type decls
+  if (isa<TypeDecl>(decl))
+    return true;
   // Ignore embeded function decls
+  if (isa<FunctionDecl>(decl))
+    return true;
   // Ignore empty decls
-  if (decl->isImplicit() || isa<TypeDecl>(decl) || isa<FunctionDecl>(decl) ||
-      isa<EmptyDecl>(decl))
+  if (isa<EmptyDecl>(decl))
     return true;
 
-  // For $Globals (whose "struct" is the TranslationUnit)
-  // Ignore resources in the TranslationUnit "struct"
-
   // For the $Globals cbuffer, we only care about externally-visiable
   // non-resource-type variables. The rest should be filtered out.
 
+  const auto *declContext = decl->getDeclContext();
+
   // Special check for ConstantBuffer/TextureBuffer, whose DeclContext is a
   // HLSLBufferDecl. So that we need to check the HLSLBufferDecl's parent decl
   // to check whether this is a ConstantBuffer/TextureBuffer defined in the
   // global namespace.
+  // Note that we should not be seeing ConstantBuffer/TextureBuffer for normal
+  // cbuffer/tbuffer or push constant blocks. So this case should only happen
+  // for $Globals cbuffer.
   if (isConstantTextureBuffer(decl) &&
-      decl->getDeclContext()->getLexicalParent()->isTranslationUnit())
+      declContext->getLexicalParent()->isTranslationUnit())
     return true;
 
-  // External visibility
-  if (const auto *declDecl = dyn_cast<DeclaratorDecl>(decl))
-    if (!declDecl->hasExternalFormalLinkage())
-      return true;
-
-  // cbuffer/tbuffer
-  if (isa<HLSLBufferDecl>(decl))
-    return true;
+  // $Globals' "struct" is the TranslationUnit, so we should ignore resources
+  // in the TranslationUnit "struct" and its child namespaces.
+  if (declContext->isTranslationUnit() || declContext->isNamespace()) {
+    // External visibility
+    if (const auto *declDecl = dyn_cast<DeclaratorDecl>(decl))
+      if (!declDecl->hasExternalFormalLinkage())
+        return true;
 
-  // Other resource types
-  if (const auto *valueDecl = dyn_cast<ValueDecl>(decl))
-    if (isResourceType(valueDecl))
+    // cbuffer/tbuffer
+    if (isa<HLSLBufferDecl>(decl))
       return true;
 
+    // Other resource types
+    if (const auto *valueDecl = dyn_cast<ValueDecl>(decl))
+      if (isResourceType(valueDecl))
+        return true;
+  }
+
   return false;
 }
 

tools/clang/test/CodeGenSPIRV/type.struct.hlsl

@@ -57,13 +57,13 @@ void main() {
   S s;
   T t;
 
-// CHECK: %_ptr_Function__struct_[[num]] = OpTypePointer Function %_struct_[[num]]
+// CHECK: %R = OpTypeStruct %v2float
 
-// CHECK: %r0 = OpVariable %_ptr_Function__struct_[[num]] Function
+// CHECK: %r0 = OpVariable %_ptr_Function_R Function
   struct R {
     float2 rVal;
   } r0;
 
-// CHECK: %r1 = OpVariable %_ptr_Function__struct_[[num]] Function
+// CHECK: %r1 = OpVariable %_ptr_Function_R Function
   R r1;
 }

tools/clang/test/CodeGenSPIRV/vk.push-constant.anon-struct.hlsl

@@ -0,0 +1,25 @@
+// Run: %dxc -T vs_6_0 -E main
+
+// CHECK: OpName %type_PushConstant_ "type.PushConstant."
+// CHECK: OpMemberName %type_PushConstant_ 0 "a"
+// CHECK: OpMemberName %type_PushConstant_ 1 "b"
+// CHECK: OpMemberName %type_PushConstant_ 2 "c"
+
+// CHECK: %type_PushConstant_ = OpTypeStruct %int %float %v3float
+// CHECK: %_ptr_PushConstant_type_PushConstant_ = OpTypePointer PushConstant %type_PushConstant_
+[[vk::push_constant]]
+struct {
+    int    a;
+    float  b;
+    float3 c;
+}
+// CHECK: %PushConstants = OpVariable %_ptr_PushConstant_type_PushConstant_ PushConstant
+PushConstants;
+
+RWBuffer<int> Output;
+
+[numthreads(1, 1, 1)]
+void main() {
+// CHECK: OpAccessChain %_ptr_PushConstant_int %PushConstants %int_0
+    Output[0] = PushConstants.a;
+}

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -1335,6 +1335,9 @@ TEST_F(FileTest, VulkanPushConstant) { runFileTest("vk.push-constant.hlsl"); }
 TEST_F(FileTest, VulkanPushConstantOffset) {
   runFileTest("vk.push-constant.offset.hlsl");
 }
+TEST_F(FileTest, VulkanPushConstantAnonymousStruct) {
+  runFileTest("vk.push-constant.anon-struct.hlsl");
+}
 TEST_F(FileTest, VulkanMultiplePushConstant) {
   runFileTest("vk.push-constant.multiple.hlsl", Expect::Failure);
 }