[SPIRV] Fix declaration order of values in decorations (microsoft#7672)

The value that decorates another value must be declared before the value
it decorates. See KhronosGroup/SPIRV-Tools#6227.

Author: s-perron

external/SPIRV-Tools

@@ -678,12 +678,23 @@ class SpirvBuilder {
                bool isPrecise, bool isNointerp, llvm::StringRef name = "",
                llvm::Optional<SpirvInstruction *> init = llvm::None,
                SourceLocation loc = {});
+
+  // Adds a variable to the module.
   SpirvVariable *
   addModuleVar(const SpirvType *valueType, spv::StorageClass storageClass,
                bool isPrecise, bool isNointerp, llvm::StringRef name = "",
                llvm::Optional<SpirvInstruction *> init = llvm::None,
                SourceLocation loc = {});
 
+  // Adds a variable to the module. It will be placed in the variable list
+  // before `pos`.
+  SpirvVariable *
+  addModuleVar(const SpirvType *valueType, spv::StorageClass storageClass,
+               bool isPrecise, bool isNointerp, SpirvInstruction *before,
+               llvm::StringRef name = "",
+               llvm::Optional<SpirvInstruction *> init = llvm::None,
+               SourceLocation loc = {});
+
   /// \brief Decorates the given target with the given location.
   void decorateLocation(SpirvInstruction *target, uint32_t location);
 

tools/clang/include/clang/SPIRV/SpirvBuilder.h

@@ -139,6 +139,10 @@ class SpirvModule {
   // Adds a variable to the module.
   void addVariable(SpirvVariable *);
 
+  // Adds a variable to the module immediately before `pos`.
+  // If `pos` is not found, `var` is added at the end of the variable list.
+  void addVariable(SpirvVariable *var, SpirvInstruction *pos);
+
   // Adds a decoration to the module.
   void addDecoration(SpirvDecoration *);
 

tools/clang/include/clang/SPIRV/SpirvModule.h

@@ -1861,7 +1861,7 @@ void DeclResultIdMapper::createCounterVar(
   }
 
   SpirvVariable *counterInstr = spvBuilder.addModuleVar(
-      counterType, sc, /*isPrecise*/ false, false, counterName);
+      counterType, sc, /*isPrecise*/ false, false, declInstr, counterName);
 
   if (!isAlias) {
     // Non-alias counter variables should be put in to resourceVars so that

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -2644,80 +2644,15 @@ uint32_t EmitTypeHandler::emitType(const SpirvType *type) {
   // NodePayloadArray types
   else if (const auto *npaType = dyn_cast<NodePayloadArrayType>(type)) {
     const uint32_t elemTypeId = emitType(npaType->getElementType());
+
+    // Output the decorations for the type first. This will create other values
+    // that are on the decorations, and they must appear before the type.
+    emitDecorationsForNodePayloadArrayTypes(npaType, id);
+
     initTypeInstruction(spv::Op::OpTypeNodePayloadArrayAMDX);
     curTypeInst.push_back(id);
     curTypeInst.push_back(elemTypeId);
     finalizeTypeInstruction();
-
-    // Emit decorations
-    const ParmVarDecl *nodeDecl = npaType->getNodeDecl();
-    if (hlsl::IsHLSLNodeOutputType(nodeDecl->getType())) {
-      StringRef name = nodeDecl->getName();
-      unsigned index = 0;
-      if (auto nodeID = nodeDecl->getAttr<HLSLNodeIdAttr>()) {
-        name = nodeID->getName();
-        index = nodeID->getArrayIndex();
-      }
-
-      auto *str = new (context) SpirvConstantString(name);
-      uint32_t nodeName = getOrCreateConstantString(str);
-      emitDecoration(id, spv::Decoration::PayloadNodeNameAMDX, {nodeName},
-                     llvm::None, true);
-      if (index) {
-        uint32_t baseIndex = getOrCreateConstantInt(
-            llvm::APInt(32, index), context.getUIntType(32), false);
-        emitDecoration(id, spv::Decoration::PayloadNodeBaseIndexAMDX,
-                       {baseIndex}, llvm::None, true);
-      }
-    }
-
-    uint32_t maxRecords;
-    if (const auto *attr = nodeDecl->getAttr<HLSLMaxRecordsAttr>()) {
-      maxRecords = getOrCreateConstantInt(llvm::APInt(32, attr->getMaxCount()),
-                                          context.getUIntType(32), false);
-    } else {
-      maxRecords = getOrCreateConstantInt(llvm::APInt(32, 1),
-                                          context.getUIntType(32), false);
-    }
-    emitDecoration(id, spv::Decoration::NodeMaxPayloadsAMDX, {maxRecords},
-                   llvm::None, true);
-
-    if (const auto *attr = nodeDecl->getAttr<HLSLMaxRecordsSharedWithAttr>()) {
-      const DeclContext *dc = nodeDecl->getParentFunctionOrMethod();
-      if (const auto *funDecl = dyn_cast_or_null<FunctionDecl>(dc)) {
-        IdentifierInfo *ii = attr->getName();
-        bool alreadyExists = false;
-        for (auto *paramDecl : funDecl->params()) {
-          if (paramDecl->getIdentifier() == ii) {
-            assert(paramDecl != nodeDecl);
-            auto otherType = context.getNodeDeclPayloadType(paramDecl);
-            const uint32_t otherId =
-                getResultIdForType(otherType, &alreadyExists);
-            assert(alreadyExists && "forward references not allowed in "
-                                    "MaxRecordsSharedWith attribute");
-            emitDecoration(id, spv::Decoration::NodeSharesPayloadLimitsWithAMDX,
-                           {otherId}, llvm::None, true);
-            break;
-          }
-        }
-        assert(alreadyExists &&
-               "invalid reference in MaxRecordsSharedWith attribute");
-      }
-    }
-    if (const auto *attr = nodeDecl->getAttr<HLSLAllowSparseNodesAttr>()) {
-      emitDecoration(id, spv::Decoration::PayloadNodeSparseArrayAMDX, {},
-                     llvm::None);
-    }
-    if (const auto *attr = nodeDecl->getAttr<HLSLUnboundedSparseNodesAttr>()) {
-      emitDecoration(id, spv::Decoration::PayloadNodeSparseArrayAMDX, {},
-                     llvm::None);
-    }
-    if (const auto *attr = nodeDecl->getAttr<HLSLNodeArraySizeAttr>()) {
-      uint32_t arraySize = getOrCreateConstantInt(
-          llvm::APInt(32, attr->getCount()), context.getUIntType(32), false);
-      emitDecoration(id, spv::Decoration::PayloadNodeArraySizeAMDX, {arraySize},
-                     llvm::None, true);
-    }
   }
   // Structure types
   else if (const auto *structType = dyn_cast<StructType>(type)) {
@@ -2998,5 +2933,78 @@ void EmitTypeHandler::emitNameForType(llvm::StringRef name,
                               nameInstr.end());
 }
 
+void EmitTypeHandler::emitDecorationsForNodePayloadArrayTypes(
+    const NodePayloadArrayType *npaType, uint32_t id) {
+  // Emit decorations
+  const ParmVarDecl *nodeDecl = npaType->getNodeDecl();
+  if (hlsl::IsHLSLNodeOutputType(nodeDecl->getType())) {
+    StringRef name = nodeDecl->getName();
+    unsigned index = 0;
+    if (auto nodeID = nodeDecl->getAttr<HLSLNodeIdAttr>()) {
+      name = nodeID->getName();
+      index = nodeID->getArrayIndex();
+    }
+
+    auto *str = new (context) SpirvConstantString(name);
+    uint32_t nodeName = getOrCreateConstantString(str);
+    emitDecoration(id, spv::Decoration::PayloadNodeNameAMDX, {nodeName},
+                   llvm::None, true);
+    if (index) {
+      uint32_t baseIndex = getOrCreateConstantInt(
+          llvm::APInt(32, index), context.getUIntType(32), false);
+      emitDecoration(id, spv::Decoration::PayloadNodeBaseIndexAMDX, {baseIndex},
+                     llvm::None, true);
+    }
+  }
+
+  uint32_t maxRecords;
+  if (const auto *attr = nodeDecl->getAttr<HLSLMaxRecordsAttr>()) {
+    maxRecords = getOrCreateConstantInt(llvm::APInt(32, attr->getMaxCount()),
+                                        context.getUIntType(32), false);
+  } else {
+    maxRecords = getOrCreateConstantInt(llvm::APInt(32, 1),
+                                        context.getUIntType(32), false);
+  }
+  emitDecoration(id, spv::Decoration::NodeMaxPayloadsAMDX, {maxRecords},
+                 llvm::None, true);
+
+  if (const auto *attr = nodeDecl->getAttr<HLSLMaxRecordsSharedWithAttr>()) {
+    const DeclContext *dc = nodeDecl->getParentFunctionOrMethod();
+    if (const auto *funDecl = dyn_cast_or_null<FunctionDecl>(dc)) {
+      IdentifierInfo *ii = attr->getName();
+      bool alreadyExists = false;
+      for (auto *paramDecl : funDecl->params()) {
+        if (paramDecl->getIdentifier() == ii) {
+          assert(paramDecl != nodeDecl);
+          auto otherType = context.getNodeDeclPayloadType(paramDecl);
+          const uint32_t otherId =
+              getResultIdForType(otherType, &alreadyExists);
+          assert(alreadyExists && "forward references not allowed in "
+                                  "MaxRecordsSharedWith attribute");
+          emitDecoration(id, spv::Decoration::NodeSharesPayloadLimitsWithAMDX,
+                         {otherId}, llvm::None, true);
+          break;
+        }
+      }
+      assert(alreadyExists &&
+             "invalid reference in MaxRecordsSharedWith attribute");
+    }
+  }
+  if (const auto *attr = nodeDecl->getAttr<HLSLAllowSparseNodesAttr>()) {
+    emitDecoration(id, spv::Decoration::PayloadNodeSparseArrayAMDX, {},
+                   llvm::None);
+  }
+  if (const auto *attr = nodeDecl->getAttr<HLSLUnboundedSparseNodesAttr>()) {
+    emitDecoration(id, spv::Decoration::PayloadNodeSparseArrayAMDX, {},
+                   llvm::None);
+  }
+  if (const auto *attr = nodeDecl->getAttr<HLSLNodeArraySizeAttr>()) {
+    uint32_t arraySize = getOrCreateConstantInt(
+        llvm::APInt(32, attr->getCount()), context.getUIntType(32), false);
+    emitDecoration(id, spv::Decoration::PayloadNodeArraySizeAMDX, {arraySize},
+                   llvm::None, true);
+  }
+}
+
 } // end namespace spirv
 } // end namespace clang

tools/clang/lib/SPIRV/EmitVisitor.cpp

@@ -135,6 +135,10 @@ class EmitTypeHandler {
   void emitNameForType(llvm::StringRef name, uint32_t targetTypeId,
                        llvm::Optional<uint32_t> memberIndex = llvm::None);
 
+  void
+  emitDecorationsForNodePayloadArrayTypes(const NodePayloadArrayType *npaType,
+                                          uint32_t id);
+
   // There is no guarantee that an instruction or a function or a basic block
   // has been assigned result-id. This method returns the result-id for the
   // given object. If a result-id has not been assigned yet, it'll assign

tools/clang/lib/SPIRV/EmitVisitor.h

@@ -1675,6 +1675,21 @@ SpirvVariable *SpirvBuilder::addModuleVar(
   return var;
 }
 
+SpirvVariable *SpirvBuilder::addModuleVar(
+    const SpirvType *type, spv::StorageClass storageClass, bool isPrecise,
+    bool isNointerp, SpirvInstruction *pos, llvm::StringRef name,
+    llvm::Optional<SpirvInstruction *> init, SourceLocation loc) {
+  assert(storageClass != spv::StorageClass::Function);
+  // Note: We store the underlying type in the variable, *not* the pointer type.
+  auto *var = new (context)
+      SpirvVariable(type, loc, storageClass, isPrecise, isNointerp,
+                    init.hasValue() ? init.getValue() : nullptr);
+  var->setResultType(type);
+  var->setDebugName(name);
+  mod->addVariable(var, pos);
+  return var;
+}
+
 void SpirvBuilder::decorateLocation(SpirvInstruction *target,
                                     uint32_t location) {
   auto *decor =

tools/clang/lib/SPIRV/SpirvBuilder.cpp

@@ -341,6 +341,12 @@ void SpirvModule::addVariable(SpirvVariable *var) {
   variables.push_back(var);
 }
 
+void SpirvModule::addVariable(SpirvVariable *var, SpirvInstruction *pos) {
+  assert(var && "cannot add null variable to the module");
+  auto location = std::find(variables.begin(), variables.end(), pos);
+  variables.insert(location, var);
+}
+
 void SpirvModule::addDecoration(SpirvDecoration *decor) {
   assert(decor && "cannot add null decoration to the module");
   decorations.insert(decor);

tools/clang/lib/SPIRV/SpirvModule.cpp

@@ -1,7 +1,7 @@
 // RUN: %dxc -T vs_6_0 -E main -fcgl  %s -spirv | FileCheck %s
 
-// CHECK: %counter_var_t_1_0 = OpVariable %_ptr_Private__ptr_Uniform_type_ACSBuffer_counter Private
 // CHECK: %counter_var_rw = OpVariable %_ptr_Uniform_type_ACSBuffer_counter Uniform
+// CHECK: %counter_var_t_1_0 = OpVariable %_ptr_Private__ptr_Uniform_type_ACSBuffer_counter Private
 // CHECK: %counter_var_s_0 = OpVariable %_ptr_Private__ptr_Uniform_type_ACSBuffer_counter Private
 
 RWStructuredBuffer<uint> rw : register(u0); 

tools/clang/test/CodeGenSPIRV/spirv.legal.counter.nested-struct.hlsl

@@ -26,6 +26,7 @@ RWStructuredBuffer<S1>      selectRWSBuffer(RWStructuredBuffer<S1>    paramRWSBu
 AppendStructuredBuffer<S2>  selectASBuffer(AppendStructuredBuffer<S2>  paramASBuffer,  bool selector);
 ConsumeStructuredBuffer<S3> selectCSBuffer(ConsumeStructuredBuffer<S3> paramCSBuffer,  bool selector);
 
+// CHECK: %counter_var_globalRWSBuffer = OpVariable %_ptr_Uniform_type_ACSBuffer_counter Uniform
 RWStructuredBuffer<S1>      globalRWSBuffer;
 // CHECK: %counter_var_globalASBuffer = OpVariable %_ptr_Uniform_type_ACSBuffer_counter Uniform
 AppendStructuredBuffer<S2>  globalASBuffer;
@@ -38,7 +39,6 @@ static RWStructuredBuffer<S1>      staticgRWSBuffer = globalRWSBuffer;
 // CHECK: %counter_var_staticgASBuffer = OpVariable %_ptr_Private__ptr_Uniform_type_ACSBuffer_counter Private
 static AppendStructuredBuffer<S2>  staticgASBuffer  = globalASBuffer;
 // CHECK: %counter_var_staticgCSBuffer = OpVariable %_ptr_Private__ptr_Uniform_type_ACSBuffer_counter Private
-// CHECK: %counter_var_globalRWSBuffer = OpVariable %_ptr_Uniform_type_ACSBuffer_counter Uniform
 static ConsumeStructuredBuffer<S3> staticgCSBuffer  = globalCSBuffer;
 
 // Counter variables for function returns, function parameters, and local variables have an extra level of pointer.