[spirv] Add support for semantics on structs (#831)

All fields will inherit the semantic attached to the struct, with
index increasing sequentially.

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -11,6 +11,7 @@
 
 #include <algorithm>
 #include <cstring>
+#include <sstream>
 #include <unordered_map>
 
 #include "dxc/HLSL/DxilConstants.h"
@@ -25,25 +26,6 @@ namespace clang {
 namespace spirv {
 
 namespace {
-/// \brief Returns true if the given decl has a semantic string attached and
-/// writes the info to *semanticStr, *semantic, *semanticIndex, and
-/// *semanticLoc.
-bool getStageVarSemantic(const NamedDecl *decl, llvm::StringRef *semanticStr,
-                         const hlsl::Semantic **semantic,
-                         uint32_t *semanticIndex, SourceLocation *semanticLoc) {
-  for (auto *annotation : decl->getUnusualAnnotations()) {
-    if (auto *sema = dyn_cast<hlsl::SemanticDecl>(annotation)) {
-      *semanticStr = sema->SemanticName;
-      llvm::StringRef semanticName;
-      hlsl::Semantic::DecomposeNameAndIndex(*semanticStr, &semanticName,
-                                            semanticIndex);
-      *semantic = hlsl::Semantic::GetByName(semanticName);
-      *semanticLoc = sema->Loc;
-      return true;
-    }
-  }
-  return false;
-}
 
 /// \brief Returns the stage variable's register assignment for the given Decl.
 const hlsl::RegisterAssignment *getResourceBinding(const NamedDecl *decl) {
@@ -113,6 +95,34 @@ inline QualType getTypeOrFnRetType(const DeclaratorDecl *decl) {
 }
 } // anonymous namespace
 
+std::string StageVar::getSemanticStr() const {
+  // A special case for zero index, which is equivalent to no index.
+  // Use what is in the source code.
+  // TODO: this looks like a hack to make the current tests happy.
+  // Should consider remove it and fix all tests.
+  if (semanticIndex == 0)
+    return semanticStr;
+
+  std::ostringstream ss;
+  ss << semanticName.str() << semanticIndex;
+  return ss.str();
+}
+
+DeclResultIdMapper::SemanticInfo
+DeclResultIdMapper::getStageVarSemantic(const NamedDecl *decl) {
+  for (auto *annotation : decl->getUnusualAnnotations()) {
+    if (auto *sema = dyn_cast<hlsl::SemanticDecl>(annotation)) {
+      llvm::StringRef semanticStr = sema->SemanticName;
+      llvm::StringRef semanticName;
+      uint32_t index = 0;
+      hlsl::Semantic::DecomposeNameAndIndex(semanticStr, &semanticName, &index);
+      const auto *semantic = hlsl::Semantic::GetByName(semanticName);
+      return {semanticStr, semantic, semanticName, index, sema->Loc};
+    }
+  }
+  return {};
+}
+
 bool DeclResultIdMapper::createStageOutputVar(const DeclaratorDecl *decl,
                                               uint32_t storedValue,
                                               bool forPCF) {
@@ -130,13 +140,15 @@ bool DeclResultIdMapper::createStageOutputVar(const DeclaratorDecl *decl,
   // none of them should be created as arrays.
   assert(sigPoint->GetKind() != hlsl::DXIL::SigPointKind::HSCPOut);
 
+  SemanticInfo inheritSemantic = {};
+
   return createStageVars(
       sigPoint, decl, /*asInput=*/false, type,
       /*arraySize=*/0, "out.var", llvm::None, &storedValue,
       // Write back of stage output variables in GS is manually controlled by
       // .Append() intrinsic method, implemented in writeBackOutputStream().
       // So noWriteBack should be set to true for GS.
-      shaderModel.IsGS());
+      shaderModel.IsGS(), &inheritSemantic);
 }
 
 bool DeclResultIdMapper::createStageOutputVar(const DeclaratorDecl *decl,
@@ -150,9 +162,11 @@ bool DeclResultIdMapper::createStageOutputVar(const DeclaratorDecl *decl,
   const auto *sigPoint =
       hlsl::SigPoint::GetSigPoint(hlsl::DXIL::SigPointKind::HSCPOut);
 
+  SemanticInfo inheritSemantic = {};
+
   return createStageVars(sigPoint, decl, /*asInput=*/false, type, arraySize,
                          "out.var", invocationId, &storedValue,
-                         /*noWriteBack=*/false);
+                         /*noWriteBack=*/false, &inheritSemantic);
 }
 
 bool DeclResultIdMapper::createStageInputVar(const ParmVarDecl *paramDecl,
@@ -179,9 +193,11 @@ bool DeclResultIdMapper::createStageInputVar(const ParmVarDecl *paramDecl,
   const auto *sigPoint = deduceSigPoint(paramDecl, /*asInput=*/true,
                                         shaderModel.GetKind(), forPCF);
 
+  SemanticInfo inheritSemantic = {};
+
   return createStageVars(sigPoint, paramDecl, /*asInput=*/true, type, arraySize,
                          "in.var", llvm::None, loadedValue,
-                         /*noWriteBack=*/false);
+                         /*noWriteBack=*/false, &inheritSemantic);
 }
 
 const DeclResultIdMapper::DeclSpirvInfo *
@@ -827,30 +843,58 @@ bool DeclResultIdMapper::decorateResourceBindings() {
 bool DeclResultIdMapper::createStageVars(
     const hlsl::SigPoint *sigPoint, const DeclaratorDecl *decl, bool asInput,
     QualType type, uint32_t arraySize, const llvm::Twine &namePrefix,
-    llvm::Optional<uint32_t> invocationId, uint32_t *value, bool noWriteBack) {
+    llvm::Optional<uint32_t> invocationId, uint32_t *value, bool noWriteBack,
+    SemanticInfo *inheritSemantic) {
   // invocationId should only be used for handling HS per-vertex output.
   if (invocationId.hasValue()) {
     assert(shaderModel.IsHS() && arraySize != 0 && !asInput);
   }
 
+  assert(inheritSemantic);
+
   if (type->isVoidType()) {
     // No stage variables will be created for void type.
     return true;
   }
 
   uint32_t typeId = typeTranslator.translateType(type);
-  llvm::StringRef semanticStr;
-  const hlsl::Semantic *semantic = {};
-  uint32_t semanticIndex = {};
-  SourceLocation semanticLoc = {};
 
-  if (getStageVarSemantic(decl, &semanticStr, &semantic, &semanticIndex,
-                          &semanticLoc)) {
-    const auto semanticKind = semantic->GetKind();
+  // We have several cases regarding HLSL semantics to handle here:
+  // * If the currrent decl inherits a semantic from some enclosing entity,
+  //   use the inherited semantic no matter whether there is a semantic
+  //   attached to the current decl.
+  // * If there is no semantic to inherit,
+  //   * If the current decl is a struct,
+  //     * If the current decl has a semantic, all its members inhert this
+  //       decl's semantic, with the index sequentially increasing;
+  //     * If the current decl does not have a semantic, all its members
+  //       should have semantics attached;
+  //   * If the current decl is not a struct, it should have semantic attached.
+
+  auto thisSemantic = getStageVarSemantic(decl);
+
+  // Which semantic we should use for this decl
+  auto *semanticToUse = &thisSemantic;
+
+  // Enclosing semantics override internal ones
+  if (inheritSemantic->isValid()) {
+    if (thisSemantic.isValid()) {
+      emitWarning(
+          "internal semantic '%0' overridden by enclosing semantic '%1'",
+          thisSemantic.loc)
+          << thisSemantic.str << inheritSemantic->str;
+    }
+    semanticToUse = inheritSemantic;
+  }
 
+  if (semanticToUse->isValid() &&
+      // Structs with attached semantics will be handled later.
+      !type->isStructureType()) {
     // Found semantic attached directly to this Decl. This means we need to
     // map this decl to a single stage variable.
 
+    const auto semanticKind = semanticToUse->semantic->GetKind();
+
     // Error out when the given semantic is invalid in this shader model
     if (hlsl::SigPoint::GetInterpretation(semanticKind, sigPoint->GetKind(),
                                           shaderModel.GetMajor(),
@@ -858,7 +902,7 @@ bool DeclResultIdMapper::createStageVars(
         hlsl::DXIL::SemanticInterpretationKind::NA) {
       emitError("invalid usage of semantic '%0' in shader profile %1",
                 decl->getLocation())
-          << semanticStr << shaderModel.GetName();
+          << semanticToUse->str << shaderModel.GetName();
       return false;
     }
 
@@ -879,7 +923,7 @@ bool DeclResultIdMapper::createStageVars(
     //   SampleMask, must be an array of integers.
 
     if (glPerVertex.tryToAccess(sigPoint->GetKind(), semanticKind,
-                                semanticIndex, invocationId, value,
+                                semanticToUse->index, invocationId, value,
                                 noWriteBack))
       return true;
 
@@ -908,9 +952,14 @@ bool DeclResultIdMapper::createStageVars(
       typeId = theBuilder.getArrayType(typeId,
                                        theBuilder.getConstantUint32(arraySize));
 
-    StageVar stageVar(sigPoint, semanticStr, semantic, semanticIndex, typeId);
+    StageVar stageVar(sigPoint, semanticToUse->str, semanticToUse->semantic,
+                      semanticToUse->name, semanticToUse->index, typeId);
+    // Note: we must have a separate variable here otherwise name
+    // (of Twine type) won't work.
+    const std::string semanticStr = stageVar.getSemanticStr();
     llvm::Twine name = namePrefix + "." + semanticStr;
-    const uint32_t varId = createSpirvStageVar(&stageVar, name, semanticLoc);
+    const uint32_t varId =
+        createSpirvStageVar(&stageVar, name, semanticToUse->loc);
 
     if (varId == 0)
       return false;
@@ -920,6 +969,9 @@ bool DeclResultIdMapper::createStageVars(
     stageVars.push_back(stageVar);
     stageVarIds[decl] = varId;
 
+    // Mark that we have used one index for this semantic
+    ++semanticToUse->index;
+
     // TODO: the following may not be correct?
     if (sigPoint->GetSignatureKind() ==
         hlsl::DXIL::SignatureKind::PatchConstant)
@@ -1039,9 +1091,10 @@ bool DeclResultIdMapper::createStageVars(
     return true;
   }
 
-  // If the decl itself doesn't have semantic string attached, it should be
-  // a struct having all its fields with semantic strings.
-  if (!type->isStructureType()) {
+  // If the decl itself doesn't have semantic string attached and there is no
+  // one to inherit, it should be a struct having all its fields with semantic
+  // strings.
+  if (!semanticToUse->isValid() && !type->isStructureType()) {
     emitError("semantic string missing for shader %select{output|input}0 "
               "variable '%1'",
               decl->getLocation())
@@ -1060,7 +1113,7 @@ bool DeclResultIdMapper::createStageVars(
       uint32_t subValue = 0;
       if (!createStageVars(sigPoint, field, asInput, field->getType(),
                            arraySize, namePrefix, invocationId, &subValue,
-                           noWriteBack))
+                           noWriteBack, semanticToUse))
         return false;
       subValues.push_back(subValue);
     }
@@ -1120,7 +1173,7 @@ bool DeclResultIdMapper::createStageVars(
 
       if (!createStageVars(sigPoint, field, asInput, field->getType(),
                            arraySize, namePrefix, invocationId, &subValue,
-                           noWriteBack))
+                           noWriteBack, semanticToUse))
         return false;
     }
   }
@@ -1139,22 +1192,18 @@ bool DeclResultIdMapper::writeBackOutputStream(const ValueDecl *decl,
   if (hasGSPrimitiveTypeQualifier(decl))
     type = astContext.getAsConstantArrayType(type)->getElementType();
 
-  llvm::StringRef semanticStr;
-  const hlsl::Semantic *semantic = {};
-  uint32_t semanticIndex = {};
-  SourceLocation semanticLoc = {};
+  auto semanticInfo = getStageVarSemantic(decl);
 
-  if (getStageVarSemantic(decl, &semanticStr, &semantic, &semanticIndex,
-                          &semanticLoc)) {
+  if (semanticInfo.isValid()) {
     // Found semantic attached directly to this Decl. Write the value for this
     // Decl to the corresponding stage output variable.
 
     const uint32_t srcTypeId = typeTranslator.translateType(type);
 
     // Handle SV_Position, SV_ClipDistance, and SV_CullDistance
-    if (glPerVertex.tryToAccess(hlsl::DXIL::SigPointKind::GSOut,
-                                semantic->GetKind(), semanticIndex, llvm::None,
-                                &value, /*noWriteBack=*/false))
+    if (glPerVertex.tryToAccess(
+            hlsl::DXIL::SigPointKind::GSOut, semanticInfo.semantic->GetKind(),
+            semanticInfo.index, llvm::None, &value, /*noWriteBack=*/false))
       return true;
 
     // Query the <result-id> for the stage output variable generated out

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -37,10 +37,12 @@ namespace spirv {
 class StageVar {
 public:
   inline StageVar(const hlsl::SigPoint *sig, llvm::StringRef semaStr,
-                  const hlsl::Semantic *sema, uint32_t semaIndex, uint32_t type)
+                  const hlsl::Semantic *sema, llvm::StringRef semaName,
+                  uint32_t semaIndex, uint32_t type)
       : sigPoint(sig), semanticStr(semaStr), semantic(sema),
-        semanticIndex(semaIndex), typeId(type), valueId(0), isBuiltin(false),
-        storageClass(spv::StorageClass::Max), location(nullptr) {}
+        semanticName(semaName), semanticIndex(semaIndex), typeId(type),
+        valueId(0), isBuiltin(false), storageClass(spv::StorageClass::Max),
+        location(nullptr) {}
 
   const hlsl::SigPoint *getSigPoint() const { return sigPoint; }
   const hlsl::Semantic *getSemantic() const { return semantic; }
@@ -50,7 +52,7 @@ public:
   uint32_t getSpirvId() const { return valueId; }
   void setSpirvId(uint32_t id) { valueId = id; }
 
-  llvm::StringRef getSemanticStr() const { return semanticStr; }
+  std::string getSemanticStr() const;
   uint32_t getSemanticIndex() const { return semanticIndex; }
 
   bool isSpirvBuitin() const { return isBuiltin; }
@@ -70,6 +72,8 @@ private:
   llvm::StringRef semanticStr;
   /// HLSL semantic.
   const hlsl::Semantic *semantic;
+  /// Original HLSL semantic string (without index) in the source code.
+  llvm::StringRef semanticName;
   /// HLSL semantic index.
   uint32_t semanticIndex;
   /// SPIR-V <type-id>.
@@ -345,6 +349,20 @@ private:
                                            llvm::StringRef typeName,
                                            llvm::StringRef varName);
 
+  /// A struct containing information about a particular HLSL semantic.
+  struct SemanticInfo {
+    llvm::StringRef str;            ///< The original semantic string
+    const hlsl::Semantic *semantic; ///< The unique semantic object
+    llvm::StringRef name;           ///< The semantic string without index
+    uint32_t index;                 ///< The semantic index
+    SourceLocation loc;             ///< Source code location
+
+    bool isValid() const { return semantic != nullptr; }
+  };
+
+  /// Returns the given decl's HLSL semantic information.
+  static SemanticInfo getStageVarSemantic(const NamedDecl *decl);
+
   /// Creates all the stage variables mapped from semantics on the given decl.
   /// Returns true on sucess.
   ///
@@ -366,11 +384,14 @@ private:
   /// array element to write to.
   ///
   /// Assumes the decl has semantic attached to itself or to its fields.
+  /// If inheritSemantic is valid, it will override all semantics attached to
+  /// the children of this decl, and the children of this decl will be using
+  /// the semantic in inheritSemantic, with index increasing sequentially.
   bool createStageVars(const hlsl::SigPoint *sigPoint,
                        const DeclaratorDecl *decl, bool asInput, QualType type,
                        uint32_t arraySize, const llvm::Twine &namePrefix,
                        llvm::Optional<uint32_t> invocationId, uint32_t *value,
-                       bool noWriteBack);
+                       bool noWriteBack, SemanticInfo *inheritSemantic);
 
   /// Creates the SPIR-V variable instruction for the given StageVar and returns
   /// the <result-id>. Also sets whether the StageVar is a SPIR-V builtin and

tools/clang/test/CodeGenSPIRV/semantic.on-struct.hlsl

@@ -0,0 +1,56 @@
+// Run: %dxc -T vs_6_0 -E main
+
+struct T1 {
+    float2 a;
+    float3 b: BBB;
+};
+
+struct T2 {
+    float2 a;
+    float3 b;
+};
+
+struct S {
+    float  h;
+    float1 i : III;
+    T1     j;
+    float4 k : JJJ;
+    T2     l : LLL;
+};
+
+// CHECK:    %in_var_X = OpVariable %_ptr_Input_float Input
+// CHECK:   %in_var_X1 = OpVariable %_ptr_Input_float Input
+// CHECK:   %in_var_X2 = OpVariable %_ptr_Input_v2float Input
+// CHECK:   %in_var_X3 = OpVariable %_ptr_Input_v3float Input
+// CHECK:   %in_var_X4 = OpVariable %_ptr_Input_v4float Input
+// CHECK:   %in_var_X5 = OpVariable %_ptr_Input_v2float Input
+// CHECK:   %in_var_X6 = OpVariable %_ptr_Input_v3float Input
+
+// CHECK:  %in_var_Y10 = OpVariable %_ptr_Input_float Input
+// CHECK:  %in_var_Y11 = OpVariable %_ptr_Input_float Input
+// CHECK:  %in_var_Y12 = OpVariable %_ptr_Input_v2float Input
+// CHECK:  %in_var_Y13 = OpVariable %_ptr_Input_v3float Input
+// CHECK:  %in_var_Y14 = OpVariable %_ptr_Input_v4float Input
+// CHECK:  %in_var_Y15 = OpVariable %_ptr_Input_v2float Input
+// CHECK:  %in_var_Y16 = OpVariable %_ptr_Input_v3float Input
+
+// CHECK: %out_var_W20 = OpVariable %_ptr_Output_float Output
+// CHECK: %out_var_W21 = OpVariable %_ptr_Output_float Output
+// CHECK: %out_var_W22 = OpVariable %_ptr_Output_v2float Output
+// CHECK: %out_var_W23 = OpVariable %_ptr_Output_v3float Output
+// CHECK: %out_var_W24 = OpVariable %_ptr_Output_v4float Output
+// CHECK: %out_var_W25 = OpVariable %_ptr_Output_v2float Output
+// CHECK: %out_var_W26 = OpVariable %_ptr_Output_v3float Output
+
+// CHECK:   %out_var_Z = OpVariable %_ptr_Output_float Output
+// CHECK:  %out_var_Z1 = OpVariable %_ptr_Output_float Output
+// CHECK:  %out_var_Z2 = OpVariable %_ptr_Output_v2float Output
+// CHECK:  %out_var_Z3 = OpVariable %_ptr_Output_v3float Output
+// CHECK:  %out_var_Z4 = OpVariable %_ptr_Output_v4float Output
+// CHECK:  %out_var_Z5 = OpVariable %_ptr_Output_v2float Output
+// CHECK:  %out_var_Z6 = OpVariable %_ptr_Output_v3float Output
+
+S main(S input1: X, S input2: Y10, out S output1: Z) : W20 {
+    output1 = input1;
+    return input2;
+}

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -402,16 +402,6 @@ TEST_F(FileTest, SemanticIsFrontFaceGS) {
 TEST_F(FileTest, SemanticIsFrontFacePS) {
   runFileTest("semantic.is-front-face.ps.hlsl");
 }
-TEST_F(FileTest, SemanticArbitrary) { runFileTest("semantic.arbitrary.hlsl"); }
-TEST_F(FileTest, SemanticArbitraryDeclLocation) {
-  runFileTest("semantic.arbitrary.location.decl.hlsl");
-}
-TEST_F(FileTest, SemanticArbitraryAlphaLocation) {
-  runFileTest("semantic.arbitrary.location.alpha.hlsl");
-}
-TEST_F(FileTest, SemanticDuplication) {
-  runFileTest("semantic.duplication.hlsl", FileTest::Expect::Failure);
-}
 TEST_F(FileTest, SemanticDispatchThreadId) {
   runFileTest("semantic.dispatch-thread-id.cs.hlsl");
 }
@@ -506,6 +496,17 @@ TEST_F(FileTest, SemanticCoverageTypeMismatchPS) {
 TEST_F(FileTest, SemanticInnerCoveragePS) {
   runFileTest("semantic.inner-coverage.ps.hlsl", Expect::Failure);
 }
+TEST_F(FileTest, SemanticArbitrary) { runFileTest("semantic.arbitrary.hlsl"); }
+TEST_F(FileTest, SemanticArbitraryDeclLocation) {
+  runFileTest("semantic.arbitrary.location.decl.hlsl");
+}
+TEST_F(FileTest, SemanticArbitraryAlphaLocation) {
+  runFileTest("semantic.arbitrary.location.alpha.hlsl");
+}
+TEST_F(FileTest, SemanticDuplication) {
+  runFileTest("semantic.duplication.hlsl", FileTest::Expect::Failure);
+}
+TEST_F(FileTest, SemanticOnStruct) { runFileTest("semantic.on-struct.hlsl"); }
 
 // For texture methods
 TEST_F(FileTest, TextureSample) { runFileTest("texture.sample.hlsl"); }