[spirv] Collect global non-resource variables into $Globals (#1138)

This commit changes the behavior of how to handle externally-visiable
non-resource-type stand-alone variables. Previously they are emitted
as stand-alone SPIR-V variables. Now they are grouped into a cbuffer
that named as $Globals. This is more aligned with how DirectX handles
them.

docs/SPIR-V.rst

@@ -840,13 +840,10 @@ According to `Shader Constants <https://msdn.microsoft.com/en-us/library/windows
   the parameter list of a function appear in the $Param constant buffer when a
   shader is compiled outside of the effects framework.
 
-However, when targeting SPIR-V, all externally visible variables are translated
-into stand-alone SPIR-V variables of their original types; they are not grouped
-together into a struct. There is one exception regarding matrix variables,
-though. For an externally visible matrix, we wrap it in a struct; the struct has
-no other members but the matrix. The reason of this behavior is to enable
-translating the ``row_major``/``column_major`` annotation since SPIR-V only
-allows ``RowMajor``/``ColMajor`` decorations to appear on struct members.
+So all global externally-visible non-resource-type stand-alone variables will
+be collected into a cbuffer named as ``$Globals``, no matter whether they are
+statically referenced by the entry point or not. The ``$Globals`` cbuffer
+follows the layout rules like normal cbuffer.
 
 Storage class
 -------------

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -381,22 +381,13 @@ uint32_t DeclResultIdMapper::createFileVar(const VarDecl *var,
 uint32_t DeclResultIdMapper::createExternVar(const VarDecl *var) {
   auto storageClass = spv::StorageClass::UniformConstant;
   auto rule = LayoutRule::Void;
-  bool isMatType = false;     // Whether is matrix that needs struct wrap
   bool isACRWSBuffer = false; // Whether is {Append|Consume|RW}StructuredBuffer
 
   if (var->getAttr<HLSLGroupSharedAttr>()) {
     // For CS groupshared variables
     storageClass = spv::StorageClass::Workgroup;
-  } else if (TypeTranslator::isMxNMatrix(var->getType())) {
-    isMatType = true;
-    // According to HLSL doc:
-    //   Variables that are placed in the global scope are added implicitly to
-    //   the $Global cbuffer, using the same packing method that is used for
-    //   cbuffers.
-    // So we should translate stand-alone matrices like cbuffer.
-    storageClass = spv::StorageClass::Uniform;
-    rule = LayoutRule::GLSLStd140;
-  } else if (auto *t = var->getType()->getAs<RecordType>()) {
+  } else if (TypeTranslator::isResourceType(var)) {
+    const auto *t = var->getType()->getAs<RecordType>();
     const llvm::StringRef typeName = t->getDecl()->getName();
 
     // These types are all translated into OpTypeStruct with BufferBlock
@@ -413,30 +404,22 @@ uint32_t DeclResultIdMapper::createExternVar(const VarDecl *var) {
       rule = LayoutRule::GLSLStd430;
       isACRWSBuffer = true;
     }
-  }
-
-  uint32_t varType = 0;
-
-  if (isMatType) {
-    // For stand-alone matrices, we need to wrap it in a struct so that we can
-    // annotate the majorness decoration.
-    varType = getMatrixStructType(var, storageClass, rule);
   } else {
-    varType = typeTranslator.translateType(var->getType(), rule);
+    // This is a stand-alone externally-visiable non-resource-type variable.
+    // They should be grouped into the $Globals cbuffer. We create that cbuffer
+    // and record all variables inside it upon seeing the first such variable.
+    if (astDecls.count(var) == 0)
+      createGlobalsCBuffer(var);
+
+    return astDecls[var].info;
   }
 
+  uint32_t varType = typeTranslator.translateType(var->getType(), rule);
+
   const uint32_t id = theBuilder.addModuleVar(varType, storageClass,
                                               var->getName(), llvm::None);
   astDecls[var] =
       SpirvEvalInfo(id).setStorageClass(storageClass).setLayoutRule(rule);
-  if (isMatType) {
-    astDecls[var].info.setRowMajor(
-        typeTranslator.isRowMajorMatrix(var->getType(), var));
-
-    // We have wrapped the stand-alone matrix inside a struct. Mark it as
-    // needing an extra index to access.
-    astDecls[var].indexInCTBuffer = 0;
-  }
 
   // Variables in Workgroup do not need descriptor decorations.
   if (storageClass == spv::StorageClass::Workgroup)
@@ -496,15 +479,19 @@ uint32_t DeclResultIdMapper::createVarOfExplicitLayoutStruct(
   // follow GLSL std140 layout rules, and tbuffers follow GLSL std430 layout
   // rules. PushConstants follow GLSL std430 layout rules.
 
+  const bool forCBuffer = usageKind == ContextUsageKind::CBuffer;
+  const bool forTBuffer = usageKind == ContextUsageKind::TBuffer;
+  const bool forGlobals = usageKind == ContextUsageKind::Globals;
+
   auto &context = *theBuilder.getSPIRVContext();
-  const LayoutRule layoutRule = usageKind == ContextUsageKind::CBuffer
+  const LayoutRule layoutRule = (forCBuffer || forGlobals)
                                     ? LayoutRule::GLSLStd140
                                     : LayoutRule::GLSLStd430;
-  const auto *blockDec = usageKind == ContextUsageKind::TBuffer
-                             ? Decoration::getBufferBlock(context)
-                             : Decoration::getBlock(context);
+  const auto *blockDec = forTBuffer ? Decoration::getBufferBlock(context)
+                                    : Decoration::getBlock(context);
 
-  auto decorations = typeTranslator.getLayoutDecorations(decl, layoutRule);
+  auto decorations =
+      typeTranslator.getLayoutDecorations(decl, layoutRule, forGlobals);
   decorations.push_back(blockDec);
 
   // Collect the type and name for each field
@@ -522,6 +509,13 @@ uint32_t DeclResultIdMapper::createVarOfExplicitLayoutStruct(
     // HLSLBufferDecls).
     assert(isa<VarDecl>(subDecl) || isa<FieldDecl>(subDecl));
     const auto *declDecl = cast<DeclaratorDecl>(subDecl);
+
+    // If we are creating the $Globals cbuffer, we only care about
+    // externally-visiable non-resource-type variables.
+    if (forGlobals && (!declDecl->hasExternalFormalLinkage() ||
+                       TypeTranslator::isResourceType(declDecl)))
+      continue;
+
     // All fields are qualified with const. It will affect the debug name.
     // We don't need it here.
     auto varType = declDecl->getType();
@@ -534,7 +528,7 @@ uint32_t DeclResultIdMapper::createVarOfExplicitLayoutStruct(
 
     // tbuffer/TextureBuffers are non-writable SSBOs. OpMemberDecorate
     // NonWritable must be applied to all fields.
-    if (usageKind == ContextUsageKind::TBuffer) {
+    if (forTBuffer) {
       decorations.push_back(Decoration::getNonWritable(
           *theBuilder.getSPIRVContext(), fieldIndex));
     }
@@ -640,6 +634,43 @@ uint32_t DeclResultIdMapper::createPushConstant(const VarDecl *decl) {
   return var;
 }
 
+void DeclResultIdMapper::createGlobalsCBuffer(const VarDecl *var) {
+  if (astDecls.count(var) != 0)
+    return;
+
+  const auto *context = var->getDeclContext();
+  const uint32_t globals = createVarOfExplicitLayoutStruct(
+      context, ContextUsageKind::Globals, "type.$Globals", "$Globals");
+
+  resourceVars.emplace_back(globals, ResourceVar::Category::Other, nullptr,
+                            nullptr, nullptr);
+
+  uint32_t index = 0;
+  for (const auto *decl : context->decls())
+    if (const auto *varDecl = dyn_cast<VarDecl>(decl)) {
+      // We are only interested in explicitly-defined externally-visible
+      // variables here.
+      if (varDecl->isImplicit() || !varDecl->hasExternalFormalLinkage() ||
+          TypeTranslator::isResourceType(varDecl))
+        continue;
+
+      if (const auto *attr = varDecl->getAttr<VKBindingAttr>()) {
+        emitError("variable '%0' will be placed in $Globals so cannot have "
+                  "vk::binding attribute",
+                  attr->getLocation())
+            << var->getName();
+        return;
+      }
+
+      astDecls[varDecl] = SpirvEvalInfo(globals)
+                              .setStorageClass(spv::StorageClass::Uniform)
+                              .setLayoutRule(LayoutRule::GLSLStd140)
+                              .setRowMajor(typeTranslator.isRowMajorMatrix(
+                                  varDecl->getType(), varDecl));
+      astDecls[varDecl].indexInCTBuffer = index++;
+    }
+}
+
 uint32_t DeclResultIdMapper::getOrRegisterFnResultId(const FunctionDecl *fn) {
   if (const auto *info = getDeclSpirvInfo(fn))
     return info->info;

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -332,6 +332,9 @@ public:
   /// \brief Creates a PushConstant block from the given decl.
   uint32_t createPushConstant(const VarDecl *decl);
 
+  /// \brief Creates the $Globals cbuffer.
+  void createGlobalsCBuffer(const VarDecl *var);
+
   /// \brief Returns the suitable type for the given decl, considering the
   /// given decl could possibly be created as an alias variable. If true, a
   /// pointer-to-the-value type will be returned, otherwise, just return the
@@ -510,6 +513,7 @@ private:
     CBuffer,
     TBuffer,
     PushConstant,
+    Globals,
   };
 
   /// Creates a variable of struct type with explicit layout decorations.

tools/clang/lib/SPIRV/SPIRVEmitter.cpp

@@ -163,8 +163,8 @@ inline bool isExternalVar(const VarDecl *var) {
   // groupshared variables are allowed to be declared as "static". But we still
   // need to put them in the Workgroup storage class. That is, when seeing
   // "static groupshared", ignore "static".
-  return var->isExternallyVisible() ? !var->isStaticDataMember()
-                                    : var->getAttr<HLSLGroupSharedAttr>();
+  return var->hasExternalFormalLinkage() ? !var->isStaticDataMember()
+                                         : var->getAttr<HLSLGroupSharedAttr>();
 }
 
 /// Returns the referenced variable's DeclContext if the given expr is
@@ -173,12 +173,8 @@ inline bool isExternalVar(const VarDecl *var) {
 const DeclContext *isConstantTextureBufferDeclRef(const Expr *expr) {
   if (const auto *declRefExpr = dyn_cast<DeclRefExpr>(expr->IgnoreParenCasts()))
     if (const auto *varDecl = dyn_cast<VarDecl>(declRefExpr->getFoundDecl()))
-      if (const auto *bufferDecl =
-              dyn_cast<HLSLBufferDecl>(varDecl->getDeclContext()))
-        // Make sure we are not returning true for VarDecls inside
-        // cbuffer/tbuffer.
-        if (bufferDecl->isConstantBufferView())
-          return varDecl->getType()->getAs<RecordType>()->getDecl();
+      if (TypeTranslator::isConstantTextureBuffer(varDecl))
+        return varDecl->getType()->getAs<RecordType>()->getDecl();
 
   return nullptr;
 }

tools/clang/lib/SPIRV/TypeTranslator.cpp

@@ -944,6 +944,26 @@ bool TypeTranslator::isOrContainsNonFpColMajorMatrix(QualType type,
   return false;
 }
 
+bool TypeTranslator::isConstantTextureBuffer(const Decl *decl) {
+  if (const auto *bufferDecl = dyn_cast<HLSLBufferDecl>(decl->getDeclContext()))
+    // Make sure we are not returning true for VarDecls inside cbuffer/tbuffer.
+    return bufferDecl->isConstantBufferView();
+
+  return false;
+}
+
+bool TypeTranslator::isResourceType(const ValueDecl *decl) {
+  if (isConstantTextureBuffer(decl))
+    return true;
+
+  const QualType declType = decl->getType();
+
+  if (isSubpassInput(declType) || isSubpassInputMS(declType))
+    return true;
+
+  return hlsl::IsHLSLResourceType(declType);
+}
+
 bool TypeTranslator::isRowMajorMatrix(QualType type, const Decl *decl) const {
   if (!isMxNMatrix(type) && !type->isArrayType())
     return false;
@@ -1068,7 +1088,8 @@ TypeTranslator::getCapabilityForStorageImageReadWrite(QualType type) {
 }
 
 llvm::SmallVector<const Decoration *, 4>
-TypeTranslator::getLayoutDecorations(const DeclContext *decl, LayoutRule rule) {
+TypeTranslator::getLayoutDecorations(const DeclContext *decl, LayoutRule rule,
+                                     bool forGlobals) {
   const auto spirvContext = theBuilder.getSPIRVContext();
   llvm::SmallVector<const Decoration *, 4> decorations;
   uint32_t offset = 0, index = 0;
@@ -1081,7 +1102,15 @@ TypeTranslator::getLayoutDecorations(const DeclContext *decl, LayoutRule rule) {
 
     // The field can only be FieldDecl (for normal structs) or VarDecl (for
     // HLSLBufferDecls).
-    auto fieldType = cast<DeclaratorDecl>(field)->getType();
+    const auto *declDecl = cast<DeclaratorDecl>(field);
+    auto fieldType = declDecl->getType();
+
+    // If we are creating the $Globals cbuffer, we only care about
+    // externally-visiable non-resource-type variables.
+    if (forGlobals &&
+        (!declDecl->hasExternalFormalLinkage() || isResourceType(declDecl)))
+      continue;
+
     const bool isRowMajor = isRowMajorMatrix(fieldType, field);
 
     uint32_t memberAlignment = 0, memberSize = 0, stride = 0;

tools/clang/lib/SPIRV/TypeTranslator.h

@@ -187,6 +187,12 @@ public:
   /// matrices.
   bool isOrContainsNonFpColMajorMatrix(QualType type, const Decl *decl) const;
 
+  /// \brief Returns true if the decl is of ConstantBuffer/TextureBuffer type.
+  static bool isConstantTextureBuffer(const Decl *decl);
+
+  /// \brief Returns true if the decl will have a SPIR-V resource type.
+  static bool isResourceType(const ValueDecl *decl);
+
   /// \brief Returns true if the two types are the same scalar or vector type,
   /// regardless of constness and literalness.
   static bool isSameScalarOrVecType(QualType type1, QualType type2);
@@ -238,7 +244,8 @@ public:
   /// according to the spec, must be attached to the array type itself instead
   /// of a struct member.
   llvm::SmallVector<const Decoration *, 4>
-  getLayoutDecorations(const DeclContext *decl, LayoutRule rule);
+  getLayoutDecorations(const DeclContext *decl, LayoutRule rule,
+                       bool forGlobals = false);
 
   /// \brief Returns how many sequential locations are consumed by a given type.
   uint32_t getLocationCount(QualType type);

tools/clang/test/CodeGenSPIRV/var.global-mat.hlsl

@@ -1,32 +0,0 @@
-// Run: %dxc -T vs_6_0 -E main
-
-// CHECK:      OpMemberDecorate %type_gMat1 0 Offset 0
-// CHECK-NEXT: OpMemberDecorate %type_gMat1 0 MatrixStride 16
-// CHECK-NEXT: OpMemberDecorate %type_gMat1 0 RowMajor
-// CHECK-NEXT: OpDecorate %type_gMat1 Block
-
-// CHECK:      OpMemberDecorate %type_gMat2 0 Offset 0
-// CHECK-NEXT: OpMemberDecorate %type_gMat2 0 MatrixStride 16
-// CHECK-NEXT: OpMemberDecorate %type_gMat2 0 ColMajor
-// CHECK-NEXT: OpDecorate %type_gMat2 Block
-
-// CHECK: %gMat1 = OpVariable %_ptr_Uniform_type_gMat1 Uniform
-// CHECK: %gMat2 = OpVariable %_ptr_Uniform_type_gMat2 Uniform
-// CHECK: %gMat3 = OpVariable %_ptr_Uniform_type_gMat1 Uniform
-             float2x3 gMat1;
-row_major    float2x3 gMat2;
-column_major float2x3 gMat3;
-
-void main() {
-// CHECK:      [[mat:%\d+]] = OpAccessChain %_ptr_Uniform_mat2v3float %gMat1 %int_0
-// CHECK-NEXT:     {{%\d+}} = OpLoad %mat2v3float [[mat]]
-    float2x3 mat = gMat1;
-
-// CHECK:      [[mat:%\d+]] = OpAccessChain %_ptr_Uniform_mat2v3float %gMat2 %int_0
-// CHECK-NEXT:     {{%\d+}} = OpAccessChain %_ptr_Uniform_v3float [[mat]] %uint_0
-    float3   vec = gMat2[0];
-
-// CHECK:      [[mat:%\d+]] = OpAccessChain %_ptr_Uniform_mat2v3float %gMat3 %int_0
-// CHECK-NEXT:     {{%\d+}} = OpAccessChain %_ptr_Uniform_float [[mat]] %int_0 %int_0
-    float scalar = gMat3._11;
-}

tools/clang/test/CodeGenSPIRV/var.globals.error.hlsl

@@ -0,0 +1,7 @@
+// Run: %dxc -T vs_6_0 -E main
+
+[[vk::binding(10, 2)]] float4 gVec;
+
+float4 main() : A { return gVec; }
+
+// CHECK: :3:3: error: variable 'gVec' will be placed in $Globals so cannot have vk::binding attribute

tools/clang/test/CodeGenSPIRV/var.globals.hlsl

@@ -0,0 +1,68 @@
+// Run: %dxc -T vs_6_0 -E main
+
+// CHECK: OpName %type__Globals "type.$Globals"
+
+// CHECK: OpMemberName %type__Globals 0 "gScalar"
+// CHECK: OpMemberName %type__Globals 1 "gVec"
+// CHECK: OpMemberName %type__Globals 2 "gMat1"
+// CHECK: OpMemberName %type__Globals 3 "gMat2"
+// CHECK: OpMemberName %type__Globals 4 "gArray"
+// CHECK: OpMemberName %type__Globals 5 "gStruct"
+// CHECK: OpMemberName %type__Globals 6 "gAnonStruct"
+
+// CHECK: OpName %_Globals "$Globals"
+
+// CHECK: OpMemberDecorate %type__Globals 0 Offset 0
+// CHECK: OpMemberDecorate %type__Globals 1 Offset 4
+// CHECK: OpMemberDecorate %type__Globals 2 Offset 16
+// CHECK: OpMemberDecorate %type__Globals 2 MatrixStride 16
+// CHECK: OpMemberDecorate %type__Globals 2 RowMajor
+// CHECK: OpMemberDecorate %type__Globals 3 Offset 64
+// CHECK: OpMemberDecorate %type__Globals 3 MatrixStride 16
+// CHECK: OpMemberDecorate %type__Globals 3 ColMajor
+// CHECK: OpMemberDecorate %type__Globals 4 Offset 96
+// CHECK: OpMemberDecorate %type__Globals 4 MatrixStride 16
+// CHECK: OpMemberDecorate %type__Globals 4 ColMajor
+// CHECK: OpMemberDecorate %type__Globals 5 Offset 160
+// CHECK: OpMemberDecorate %type__Globals 6 Offset 176
+// CHECK: OpDecorate %type__Globals Block
+
+// CHECK: OpDecorate %_Globals DescriptorSet 0
+// CHECK: OpDecorate %_Globals Binding 0
+
+          int           gScalar;   // 0
+          SamplerState  gSampler;  // Not included
+          float2        gVec;      // 1
+          Texture2D     gTex;      // Not included
+          float2x3      gMat1;     // 2
+row_major float2x3      gMat2;     // 3
+
+StructuredBuffer<float> gSBuffer;  // Not included
+
+row_major float2x3      gArray[2]; // 4
+
+struct S {
+    float f;
+};
+
+          S             gStruct;   // 5
+
+ConstantBuffer<S>       gCBuffer;  // Not included
+
+// CHECK: [[v2f_struct:%\w+]] = OpTypeStruct %v2float
+struct {
+    float2 f;
+}                       gAnonStruct; // 6
+
+// CHECK: %type__Globals = OpTypeStruct %int %v2float %mat2v3float %mat2v3float %_arr_mat2v3float_uint_2 %S [[v2f_struct]]
+// CHECK: %_ptr_Uniform_type__Globals = OpTypePointer Uniform %type__Globals
+
+// %_Globals = OpVariable %_ptr_Uniform_type__Globals Uniform
+
+float main() : A {
+
+// CHECK: OpAccessChain %_ptr_Uniform_int %_Globals %int_0
+// CHECK: OpAccessChain %_ptr_Uniform_mat2v3float %_Globals %int_3
+// CHECK: OpAccessChain %_ptr_Uniform_S %_Globals %int_5
+    return gScalar + gMat2[0][0] + gStruct.f;
+}

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -123,7 +123,10 @@ TEST_F(FileTest, StaticVar) { runFileTest("var.static.hlsl"); }
 TEST_F(FileTest, UninitStaticResourceVar) {
   runFileTest("var.static.resource.hlsl");
 }
-TEST_F(FileTest, GlobalMatrixVar) { runFileTest("var.global-mat.hlsl"); }
+TEST_F(FileTest, GlobalsCBuffer) { runFileTest("var.globals.hlsl"); }
+TEST_F(FileTest, GlobalsCBufferError) {
+  runFileTest("var.globals.error.hlsl", Expect::Failure);
+}
 
 // For prefix/postfix increment/decrement
 TEST_F(FileTest, UnaryOpPrefixIncrement) {