[spirv] Assign locations to stage I/O variables alphabetically (#564)

SV_Target[N] is treated differently; they will always be assigned
the location N. For other semantics that does not map to builtin
variables, assign locations to them alphabetically.

Also extended FileTest fixture to support checking the contents
of error messages.

tools/clang/lib/SPIRV/DeclResultIdMapper.cpp

@@ -9,11 +9,16 @@
 
 #include "DeclResultIdMapper.h"
 
+#include <algorithm>
+#include <cstring>
+
 #include "dxc/HLSL/DxilConstants.h"
 #include "dxc/HLSL/DxilTypeSystem.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/HlslTypes.h"
 #include "clang/AST/RecursiveASTVisitor.h"
+#include "llvm/ADT/SmallBitVector.h"
+#include "llvm/ADT/StringSet.h"
 
 namespace clang {
 namespace spirv {
@@ -140,21 +145,93 @@ std::vector<uint32_t> DeclResultIdMapper::collectStageVars() const {
   return vars;
 }
 
+namespace {
+/// A class for managing stage input/output locations to avoid duplicate uses of
+/// the same location.
+class LocationSet {
+public:
+  // Typically we won't have that many stage input or output variables.
+  // Using 64 should be fine here.
+  // TODO: Emit errors if we need more than 64.
+  LocationSet() : usedLocs(64, false), nextLoc(0) {}
+
+  /// Uses the given location.
+  void useLoc(uint32_t loc) { usedLocs.set(loc); }
+
+  /// Uses the next available location.
+  uint32_t useNextLoc() {
+    while (usedLocs[nextLoc])
+      nextLoc++;
+    usedLocs.set(nextLoc);
+    return nextLoc++;
+  }
+
+private:
+  llvm::SmallBitVector usedLocs; ///< All previously used locations
+  uint32_t nextLoc;              ///< Next available location
+};
+} // namespace
+
 void DeclResultIdMapper::finalizeStageIOLocations() {
-  uint32_t nextInputLocation = 0;
-  uint32_t nextOutputLocation = 0;
+  { // Check semantic duplication
+    llvm::StringSet<> seenInputSemantics;
+    llvm::StringSet<> seenOutputSemantics;
+    bool success = true;
 
-  // TODO: sort the variables according to some criteria first, e.g.,
-  // alphabetical order of semantic names.
-  for (auto &var : stageVars) {
-    if (!var.isSpirvBuitin()) {
+    for (const auto &var : stageVars) {
+      auto s = var.getSemanticStr();
       if (var.getSigPoint()->IsInput()) {
-        theBuilder.decorateLocation(var.getSpirvId(), nextInputLocation++);
-      } else if (var.getSigPoint()->IsOutput()) {
-        theBuilder.decorateLocation(var.getSpirvId(), nextOutputLocation++);
+        if (seenInputSemantics.count(s)) {
+          emitError("input semantic '%0' used more than once") << s;
+          success = false;
+        }
+        seenInputSemantics.insert(s);
+      } else {
+        if (seenOutputSemantics.count(s)) {
+          emitError("output semantic '%0' used more than once") << s;
+          success = false;
+        }
+        seenOutputSemantics.insert(s);
       }
     }
+
+    if (!success)
+      return;
   }
+
+  std::vector<const StageVar *> inputVars;
+  std::vector<const StageVar *> outputVars;
+
+  LocationSet inputLocs;
+  LocationSet outputLocs;
+
+  for (const auto &var : stageVars)
+    if (!var.isSpirvBuitin()) {
+      // Only SV_Target, SV_Depth, SV_DepthLessEqual, SV_DepthGreaterEqual,
+      // SV_StencilRef, SV_Coverage are allowed in the pixel shader.
+      // Arbitrary semantics are disallowed in pixel shader.
+      if (var.getSemantic()->GetKind() == hlsl::Semantic::Kind::Target) {
+        theBuilder.decorateLocation(var.getSpirvId(), var.getSemanticIndex());
+        outputLocs.useLoc(var.getSemanticIndex());
+      } else if (var.getSigPoint()->IsInput()) {
+        inputVars.push_back(&var);
+      } else if (var.getSigPoint()->IsOutput()) {
+        outputVars.push_back(&var);
+      }
+    }
+
+  // Sort stage input/output variables alphabetically
+  const auto comp = [](const StageVar *a, const StageVar *b) {
+    return a->getSemanticStr() < b->getSemanticStr();
+  };
+
+  std::sort(inputVars.begin(), inputVars.end(), comp);
+  std::sort(outputVars.begin(), outputVars.end(), comp);
+
+  for (const auto *var : inputVars)
+    theBuilder.decorateLocation(var->getSpirvId(), inputLocs.useNextLoc());
+  for (const auto *var : outputVars)
+    theBuilder.decorateLocation(var->getSpirvId(), outputLocs.useNextLoc());
 }
 
 QualType
@@ -189,7 +266,12 @@ bool DeclResultIdMapper::createStageVars(const DeclaratorDecl *decl,
         hlsl::SigPoint::GetSigPoint(hlsl::SigPointFromInputQual(
             qual, shaderModel.GetKind(), /*isPC*/ false));
 
-    const auto *semantic = hlsl::Semantic::GetByName(semanticStr);
+    llvm::StringRef semanticName;
+    uint32_t semanticIndex = 0;
+    hlsl::Semantic::DecomposeNameAndIndex(semanticStr, &semanticName,
+                                          &semanticIndex);
+
+    const auto *semantic = hlsl::Semantic::GetByName(semanticName);
 
     // Error out when the given semantic is invalid in this shader model
     if (hlsl::SigPoint::GetInterpretation(
@@ -201,11 +283,8 @@ bool DeclResultIdMapper::createStageVars(const DeclaratorDecl *decl,
       return false;
     }
 
-    StageVar stageVar(sigPoint, semantic, typeId);
-    llvm::Twine name = hlsl::Semantic::HasSVPrefix(semanticStr)
-                           // Canonicalize SV semantics
-                           ? namePrefix + "." + semantic->GetName()
-                           : namePrefix + "." + semanticStr;
+    StageVar stageVar(sigPoint, semanticStr, semantic, semanticIndex, typeId);
+    llvm::Twine name = namePrefix + "." + semanticStr;
     const uint32_t varId = createSpirvStageVar(&stageVar, name);
     if (varId == 0)
       return false;

tools/clang/lib/SPIRV/DeclResultIdMapper.h

@@ -31,7 +31,8 @@ namespace spirv {
 /// (builtin/input/output) variable.
 class StageVar {
 public:
-  inline StageVar(const hlsl::SigPoint *sig, const hlsl::Semantic *sema,
+  inline StageVar(const hlsl::SigPoint *sig, llvm::StringRef semaStr,
+                  const hlsl::Semantic *sema, uint32_t semaIndex,
                   uint32_t type);
 
   const hlsl::SigPoint *getSigPoint() const { return sigPoint; }
@@ -42,6 +43,9 @@ public:
   uint32_t getSpirvId() const { return valueId; }
   void setSpirvId(uint32_t id) { valueId = id; }
 
+  llvm::StringRef getSemanticStr() const { return semanticStr; }
+  uint32_t getSemanticIndex() const { return semanticIndex; }
+
   bool isSpirvBuitin() const { return isBuiltin; }
   void setIsSpirvBuiltin() { isBuiltin = true; }
 
@@ -55,8 +59,12 @@ private:
   /// HLSL SigPoint. It uniquely identifies each set of parameters that may be
   /// input or output for each entry point.
   const hlsl::SigPoint *sigPoint;
+  /// Original HLSL semantic string in the source code.
+  llvm::StringRef semanticStr;
   /// HLSL semantic.
   const hlsl::Semantic *semantic;
+  /// HLSL semantic index.
+  uint32_t semanticIndex;
   /// SPIR-V <type-id>.
   uint32_t typeId;
   /// SPIR-V <result-id>.
@@ -69,9 +77,11 @@ private:
   llvm::Optional<uint32_t> location;
 };
 
-StageVar::StageVar(const hlsl::SigPoint *sig, const hlsl::Semantic *sema,
+StageVar::StageVar(const hlsl::SigPoint *sig, llvm::StringRef semaStr,
+                   const hlsl::Semantic *sema, uint32_t semaIndex,
                    uint32_t type)
-    : sigPoint(sig), semantic(sema), typeId(type), valueId(0), isBuiltin(false),
+    : sigPoint(sig), semanticStr(semaStr), semantic(sema),
+      semanticIndex(semaIndex), typeId(type), valueId(0), isBuiltin(false),
       storageClass(spv::StorageClass::Max), location(llvm::None) {}
 
 /// \brief The class containing mappings from Clang frontend Decls to their

tools/clang/test/CodeGenSPIRV/constant-ps.hlsl2spv

@@ -1,5 +1,6 @@
 // Run: %dxc -T ps_6_0 -E main
-float4 main(): SV_TARGET
+
+float4 main(): SV_Target
 {
   return float4(1.0f, 2.0f, 3.5f, 4.7f);
 }

tools/clang/test/CodeGenSPIRV/passthru-ps.hlsl2spv

@@ -1,6 +1,6 @@
 // Run: %dxc -T ps_6_0 -E main
 
-float4 main(float4 input: COLOR): SV_TARGET
+float4 main(float4 input: COLOR): SV_Target
 {
     return input;
 }
@@ -22,8 +22,8 @@ float4 main(float4 input: COLOR): SV_TARGET
 // OpName %out_var_SV_Target "out.var.SV_Target"
 // OpName %src_main "src.main"
 // OpName %input "input"
-// OpDecorate %in_var_COLOR Location 0
 // OpDecorate %out_var_SV_Target Location 0
+// OpDecorate %in_var_COLOR Location 0
 // %void = OpTypeVoid
 // %3 = OpTypeFunction %void
 // %float = OpTypeFloat 32
@@ -48,4 +48,4 @@ float4 main(float4 input: COLOR): SV_TARGET
 // %bb_entry = OpLabel
 // %19 = OpLoad %v4float %input
 // OpReturnValue %19
-// OpFunctionEnd
+// OpFunctionEnd

tools/clang/test/CodeGenSPIRV/passthru-vs.hlsl2spv

@@ -1,7 +1,7 @@
 // Run: %dxc -T vs_6_0 -E VSmain
 
 struct PSInput {
-  float4 position : SV_POSITION;
+  float4 position : SV_Position;
   float4 color : COLOR;
 };
 
@@ -37,8 +37,8 @@ PSInput VSmain(float4 position: POSITION, float4 color: COLOR) {
 // OpName %color "color"
 // OpName %result "result"
 // OpDecorate %gl_Position BuiltIn Position
-// OpDecorate %in_var_POSITION Location 0
-// OpDecorate %in_var_COLOR Location 1
+// OpDecorate %in_var_COLOR Location 0
+// OpDecorate %in_var_POSITION Location 1
 // OpDecorate %out_var_COLOR Location 0
 // %int = OpTypeInt 32 1
 // %void = OpTypeVoid

tools/clang/test/CodeGenSPIRV/semantic.arbitrary.location.hlsl

@@ -0,0 +1,44 @@
+// Run: %dxc -T vs_6_0 -E main
+
+struct S {
+    float c: C;
+    float a: A;
+    float b: B;
+};
+
+struct T {
+    float e: E;
+    S     s;
+    float d: D;
+};
+
+// CHECK:      OpDecorate %in_var_A Location 0
+// CHECK-NEXT: OpDecorate %in_var_B Location 1
+// CHECK-NEXT: OpDecorate %in_var_C Location 2
+// CHECK-NEXT: OpDecorate %in_var_D Location 3
+// CHECK-NEXT: OpDecorate %in_var_E Location 4
+// CHECK-NEXT: OpDecorate %in_var_M Location 5
+// CHECK-NEXT: OpDecorate %in_var_N Location 6
+
+// CHECK-NEXT: OpDecorate %out_var_A Location 0
+// CHECK-NEXT: OpDecorate %out_var_B Location 1
+// CHECK-NEXT: OpDecorate %out_var_C Location 2
+// CHECK-NEXT: OpDecorate %out_var_D Location 3
+// CHECK-NEXT: OpDecorate %out_var_E Location 4
+// CHECK-NEXT: OpDecorate %out_var_P Location 5
+// CHECK-NEXT: OpDecorate %out_var_Q Location 6
+
+// Input semantics by declaration order: N, E, C, A, B, D, M
+//                 by alphabetic order:  A, B, C, D, E, M, N
+
+// Output semantics by declaration order: E, C, A, B, D, Q, P
+//                  by alphabetic order:  A, B, C, D, E, P, Q
+
+void main(in  float input1 : N,
+          in  T     input2 ,
+          in  float input3 : M,
+          out T     output1,
+          out float output2: Q,
+          out float output3: P
+         ) {
+}

tools/clang/test/CodeGenSPIRV/semantic.duplication.hlsl

@@ -0,0 +1,19 @@
+// Run: %dxc -T vs_6_0 -E main
+
+struct VSIn {
+    float4 f: ABC;
+};
+
+struct VSOut {
+    float4 g: ABC;
+};
+
+void main(    int a: ABC,     int b: ABC,     VSIn c,
+          out int u: ABC, out int v: ABC, out VSOut w) {
+}
+
+// CHECK: error: input semantic 'ABC' used more than once
+// CHECK: error: input semantic 'ABC' used more than once
+
+// CHECK: error: output semantic 'ABC' used more than once
+// CHECK: error: output semantic 'ABC' used more than once

tools/clang/test/CodeGenSPIRV/semantic.target.ps.hlsl

@@ -1,11 +1,29 @@
 // Run: %dxc -T ps_6_0 -E main
 
-// CHECK:              OpEntryPoint Fragment %main "main" %in_var_A %out_var_SV_Target
+// CHECK:      OpEntryPoint Fragment %main "main" %in_var_A %out_var_SV_Target2 %out_var_SV_Target %out_var_SV_Target1 %out_var_SV_Target3
 
-// CHECK:              OpDecorate %out_var_SV_Target Location 0
+// CHECK:      OpDecorate %out_var_SV_Target2 Location 2
+// CHECK-NEXT: OpDecorate %out_var_SV_Target Location 0
+// CHECK-NEXT: OpDecorate %out_var_SV_Target1 Location 1
+// CHECK-NEXT: OpDecorate %out_var_SV_Target3 Location 3
 
-// CHECK: %out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
+// CHECK:      %out_var_SV_Target2 = OpVariable %_ptr_Output_v4float Output
+// CHECK-NEXT: %out_var_SV_Target = OpVariable %_ptr_Output_v4float Output
+// CHECK-NEXT: %out_var_SV_Target1 = OpVariable %_ptr_Output_v4float Output
+// CHECK-NEXT: %out_var_SV_Target3 = OpVariable %_ptr_Output_v4float Output
 
-float4 main(float4 input: A) : SV_Target {
-    return input;
+struct PSOut {
+    float4 color2 : SV_Target2;
+    float4 color0 : SV_Target;
+    float4 color1 : SV_Target1;
+    float4 color3 : SV_Target3;
+};
+
+PSOut main(float4 input: A) {
+    PSOut ret;
+    ret.color2 = input;
+    ret.color0 = input;
+    ret.color1 = input;
+    ret.color3 = input;
+    return ret;
 }

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -279,6 +279,12 @@ TEST_F(FileTest, SemanticIsFrontFacePS) {
   runFileTest("semantic.is-front-face.ps.hlsl");
 }
 TEST_F(FileTest, SemanticArbitrary) { runFileTest("semantic.arbitrary.hlsl"); }
+TEST_F(FileTest, SemanticArbitraryLocation) {
+  runFileTest("semantic.arbitrary.location.hlsl");
+}
+TEST_F(FileTest, SemanticDuplication) {
+  runFileTest("semantic.duplication.hlsl", /*expectSuccess*/ false);
+}
 
 // For intrinsic functions
 TEST_F(FileTest, IntrinsicsDot) { runFileTest("intrinsics.dot.hlsl"); }

tools/clang/unittests/SPIRV/FileTestFixture.cpp

@@ -60,23 +60,40 @@ bool FileTest::parseInputFile() {
   return true;
 }
 
-void FileTest::runFileTest(llvm::StringRef filename, bool runSpirvValidation) {
+void FileTest::runFileTest(llvm::StringRef filename, bool expectSuccess,
+                           bool runSpirvValidation) {
   inputFilePath = utils::getAbsPathOfInputDataFile(filename);
 
   // Parse the input file.
   ASSERT_TRUE(parseInputFile());
 
+  std::string errorMessages;
+
   // Feed the HLSL source into the Compiler.
-  ASSERT_TRUE(utils::runCompilerWithSpirvGeneration(
-      inputFilePath, entryPoint, targetProfile, &generatedBinary));
+  const bool compileOk = utils::runCompilerWithSpirvGeneration(
+      inputFilePath, entryPoint, targetProfile, &generatedBinary,
+      &errorMessages);
+
+  effcee::Result result(effcee::Result::Status::Ok);
+
+  if (expectSuccess) {
+    ASSERT_TRUE(compileOk);
+
+    // Disassemble the generated SPIR-V binary.
+    ASSERT_TRUE(utils::disassembleSpirvBinary(
+        generatedBinary, &generatedSpirvAsm, true /* generateHeader */));
 
-  // Disassemble the generated SPIR-V binary.
-  ASSERT_TRUE(utils::disassembleSpirvBinary(generatedBinary, &generatedSpirvAsm,
-                                            true /* generateHeader */));
+    // Run CHECK commands via effcee on disassembly.
+    result = effcee::Match(generatedSpirvAsm, checkCommands,
+                           effcee::Options().SetInputName(filename.str()));
 
-  // Run CHECK commands via effcee.
-  auto result = effcee::Match(generatedSpirvAsm, checkCommands,
-                              effcee::Options().SetInputName(filename.str()));
+  } else {
+    ASSERT_FALSE(compileOk);
+
+    // Run CHECK commands via effcee on error messages.
+    result = effcee::Match(errorMessages, checkCommands,
+                           effcee::Options().SetInputName("<error-message>"));
+  }
 
   // Print effcee's error message (if any).
   if (result.status() != effcee::Result::Status::Ok) {
@@ -87,7 +104,7 @@ void FileTest::runFileTest(llvm::StringRef filename, bool runSpirvValidation) {
   ASSERT_EQ(result.status(), effcee::Result::Status::Ok);
 
   // Run SPIR-V validation if requested.
-  if (runSpirvValidation) {
+  if (expectSuccess && runSpirvValidation) {
     EXPECT_TRUE(utils::validateSpirvBinary(generatedBinary));
   }
 }

tools/clang/unittests/SPIRV/FileTestFixture.h

@@ -19,11 +19,14 @@ namespace spirv {
 class FileTest : public ::testing::Test {
 public:
   /// \brief Runs a File Test! (See class description for more info)
-  /// Returns true if the test passes; false otherwise.
+  ///
+  /// If the compilation is expected to fail, expectedSuccess should be
+  /// set to false.
   /// It is important that all generated SPIR-V code is valid. Users of
   /// FileTest may choose not to run the SPIR-V validator (for cases where
   /// a certain feature has not been added to the validator yet).
-  void runFileTest(llvm::StringRef path, bool runSpirvValidation = true);
+  void runFileTest(llvm::StringRef path, bool expectSuccess = true,
+                   bool runSpirvValidation = true);
 
 private:
   /// \brief Reads in the given input file.

tools/clang/unittests/SPIRV/FileTestUtils.cpp

@@ -7,9 +7,10 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include "FileTestUtils.h"
+
 #include <algorithm>
 
-#include "FileTestUtils.h"
 #include "SpirvTestOptions.h"
 #include "gtest/gtest.h"
 
@@ -99,7 +100,8 @@ std::string getAbsPathOfInputDataFile(const llvm::StringRef filename) {
 bool runCompilerWithSpirvGeneration(const llvm::StringRef inputFilePath,
                                     const llvm::StringRef entryPoint,
                                     const llvm::StringRef targetProfile,
-                                    std::vector<uint32_t> *generatedBinary) {
+                                    std::vector<uint32_t> *generatedBinary,
+                                    std::string *errorMessages) {
   std::wstring srcFile(inputFilePath.begin(), inputFilePath.end());
   std::wstring entry(entryPoint.begin(), entryPoint.end());
   std::wstring profile(targetProfile.begin(), targetProfile.end());
@@ -140,6 +142,7 @@ bool runCompilerWithSpirvGeneration(const llvm::StringRef inputFilePath,
     const std::string diagnostics((char *)pErrorBuffer->GetBufferPointer(),
                                   pErrorBuffer->GetBufferSize());
     fprintf(stderr, "%s\n", diagnostics.c_str());
+    *errorMessages = diagnostics;
 
     if (SUCCEEDED(resultStatus)) {
       CComPtr<IDxcBlobEncoding> pStdErr;

tools/clang/unittests/SPIRV/FileTestUtils.h

@@ -10,6 +10,7 @@
 #ifndef LLVM_CLANG_UNITTESTS_SPIRV_FILETESTUTILS_H
 #define LLVM_CLANG_UNITTESTS_SPIRV_FILETESTUTILS_H
 
+#include <string>
 #include <vector>
 
 #include "dxc/Support/Global.h"
@@ -51,11 +52,13 @@ std::string getAbsPathOfInputDataFile(const llvm::StringRef filename);
 
 /// \brief Passes the HLSL input file to the DXC compiler with SPIR-V CodeGen.
 /// Returns the generated SPIR-V binary via 'generatedBinary' argument.
-/// Returns true on success, and false on failure.
+/// Returns true on success, and false on failure. Writes error messages to
+/// errorMessages and stderr on failure.
 bool runCompilerWithSpirvGeneration(const llvm::StringRef inputFilePath,
                                     const llvm::StringRef entryPoint,
                                     const llvm::StringRef targetProfile,
-                                    std::vector<uint32_t> *generatedBinary);
+                                    std::vector<uint32_t> *generatedBinary,
+                                    std::string *errorMessages);
 
 } // end namespace utils
 } // end namespace spirv

tools/clang/unittests/SPIRV/WholeFileTestFixture.cpp

@@ -90,9 +90,12 @@ void WholeFileTest::runWholeFileTest(llvm::StringRef filename,
   // Parse the input file.
   ASSERT_TRUE(parseInputFile());
 
+  std::string errorMessages;
+
   // Feed the HLSL source into the Compiler.
   ASSERT_TRUE(utils::runCompilerWithSpirvGeneration(
-      inputFilePath, entryPoint, targetProfile, &generatedBinary));
+      inputFilePath, entryPoint, targetProfile, &generatedBinary,
+      &errorMessages));
 
   // Disassemble the generated SPIR-V binary.
   ASSERT_TRUE(utils::disassembleSpirvBinary(generatedBinary, &generatedSpirvAsm,