[spirv] Legalization: validate with relaxed logical pointer (#975)

We still want to make sure that the generated raw SPIR-V does not
have other validation errors except for allocating variables
containing pointers and returning pointers.

external/SPIRV-Tools

@@ -1 +1 @@
-Subproject commit 3fbbd3c772c53930417b4cc7db949c6e52b6b2a2
+Subproject commit a82a0ea886f59f1987f339b05cca71cc248adc78

tools/clang/lib/SPIRV/SPIRVEmitter.cpp

@@ -249,7 +249,8 @@ bool spirvToolsOptimize(std::vector<uint32_t> *module, std::string *messages) {
   return optimizer.Run(module->data(), module->size(), module);
 }
 
-bool spirvToolsValidate(std::vector<uint32_t> *module, std::string *messages) {
+bool spirvToolsValidate(std::vector<uint32_t> *module, std::string *messages,
+                        bool relaxLogicalPointer) {
   spvtools::SpirvTools tools(SPV_ENV_VULKAN_1_0);
 
   tools.SetMessageConsumer(
@@ -257,7 +258,10 @@ bool spirvToolsValidate(std::vector<uint32_t> *module, std::string *messages) {
                  const spv_position_t & /*position*/,
                  const char *message) { *messages += message; });
 
-  return tools.Validate(module->data(), module->size());
+  spvtools::ValidatorOptions options;
+  options.SetRelaxLogicalPointer(relaxLogicalPointer);
+
+  return tools.Validate(module->data(), module->size(), options);
 }
 
 /// Translates atomic HLSL opcodes into the equivalent SPIR-V opcode.
@@ -395,8 +399,8 @@ SPIRVEmitter::SPIRVEmitter(CompilerInstance &ci,
       seenPushConstantAt(), needsLegalization(false) {
   if (shaderModel.GetKind() == hlsl::ShaderModel::Kind::Invalid)
     emitError("unknown shader module: %0", {}) << shaderModel.GetName();
-  if (options.invertY &&
-      !(shaderModel.IsVS() || shaderModel.IsDS() || shaderModel.IsGS()))
+  if (options.invertY && !shaderModel.IsVS() && !shaderModel.IsDS() &&
+      !shaderModel.IsGS())
     emitError("-fvk-invert-y can only be used in VS/DS/GS", {});
 }
 
@@ -488,7 +492,8 @@ void SPIRVEmitter::HandleTranslationUnit(ASTContext &context) {
   // Validate the generated SPIR-V code
   if (!spirvOptions.disableValidation) {
     std::string messages;
-    if (!spirvToolsValidate(&m, &messages)) {
+    if (!spirvToolsValidate(&m, &messages,
+                            declIdMapper.requiresLegalization())) {
       emitFatalError("generated SPIR-V is invalid: %0", {}) << messages;
       emitNote("please file a bug report on "
                "https://github.com/Microsoft/DirectXShaderCompiler/issues "

tools/clang/test/CodeGenSPIRV/spirv.legal.sbuffer.methods.hlsl

@@ -131,15 +131,15 @@ float4 main() : SV_Target {
 // CHECK:      [[ptr1:%\d+]] = OpLoad %_ptr_Uniform_type_RWByteAddressBuffer %localRWBABuffer
 // CHECK:      [[ptr2:%\d+]] = OpAccessChain %_ptr_Uniform_uint [[ptr1]] %uint_0 {{%\d+}}
 // CHECK-NEXT:                 OpStore [[ptr2]] {{%\d+}}
-    localRWBABuffer.Store(13, byte2);
+    localRWBABuffer.Store2(13, byte2);
 // CHECK:      [[ptr1:%\d+]] = OpLoad %_ptr_Uniform_type_RWByteAddressBuffer %localRWBABuffer
 // CHECK:      [[ptr2:%\d+]] = OpAccessChain %_ptr_Uniform_uint [[ptr1]] %uint_0 {{%\d+}}
 // CHECK-NEXT:                 OpStore [[ptr2]] {{%\d+}}
-    localRWBABuffer.Store(14, byte3);
+    localRWBABuffer.Store3(14, byte3);
 // CHECK:      [[ptr1:%\d+]] = OpLoad %_ptr_Uniform_type_RWByteAddressBuffer %localRWBABuffer
 // CHECK:      [[ptr2:%\d+]] = OpAccessChain %_ptr_Uniform_uint [[ptr1]] %uint_0 {{%\d+}}
 // CHECK-NEXT:                 OpStore [[ptr2]] {{%\d+}}
-    localRWBABuffer.Store(15, byte4);
+    localRWBABuffer.Store4(15, byte4);
 // CHECK:      [[ptr1:%\d+]] = OpLoad %_ptr_Uniform_type_RWByteAddressBuffer %localRWBABuffer
 // CHECK:      [[ptr2:%\d+]] = OpAccessChain %_ptr_Uniform_uint [[ptr1]] %uint_0 {{%\d+}}
     localRWBABuffer.InterlockedAdd(dest, value, origin);

tools/clang/test/CodeGenSPIRV/spirv.legal.tbuffer.hlsl

@@ -52,7 +52,8 @@ float4 main(in float4 pos : SV_Position) : SV_Target
     S buffer3;
     buffer3 = retStuff();
 
-// The underlying struct type has the same layout. Can write out as a whole.
+// TODO: The underlying struct type has the same layout but %type_TextureBuffer_S
+// has an additional BufferBlock decoration. So this causes an validation error.
 // CHECK:      [[ptr:%\d+]] = OpAccessChain %_ptr_Uniform_S %myASBuffer %uint_0 {{%\d+}}
 // CHECK-NEXT: [[val:%\d+]] = OpLoad %type_TextureBuffer_S %myTBuffer
 // CHECK-NEXT:                OpStore [[ptr]] [[val]]

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -49,7 +49,7 @@ TEST_F(FileTest, MatrixTypesMajornessZpr) {
   runFileTest("type.matrix.majorness.zpr.hlsl");
 }
 TEST_F(FileTest, MatrixTypesMajorness) {
-  runFileTest("type.matrix.majorness.hlsl", FileTest::Expect::Warning);
+  runFileTest("type.matrix.majorness.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, StructTypes) { runFileTest("type.struct.hlsl"); }
 TEST_F(FileTest, ClassTypes) { runFileTest("type.class.hlsl"); }
@@ -90,7 +90,7 @@ TEST_F(FileTest, 16BitDisabledScalarConstants) {
 }
 TEST_F(FileTest, 16BitEnabledScalarConstants) {
   // TODO: Fix spirv-val to make sure it respects the 16-bit extension.
-  runFileTest("constant.scalar.16bit.enabled.hlsl", FileTest::Expect::Success,
+  runFileTest("constant.scalar.16bit.enabled.hlsl", Expect::Success,
               /*runValidation*/ false);
 }
 TEST_F(FileTest, 64BitScalarConstants) {
@@ -110,10 +110,10 @@ TEST_F(FileTest, VarInitMatrix1x1) { runFileTest("var.init.matrix.1x1.hlsl"); }
 TEST_F(FileTest, VarInitStruct) { runFileTest("var.init.struct.hlsl"); }
 TEST_F(FileTest, VarInitArray) { runFileTest("var.init.array.hlsl"); }
 TEST_F(FileTest, VarInitCbuffer) {
-  runFileTest("var.init.cbuffer.hlsl", FileTest::Expect::Warning);
+  runFileTest("var.init.cbuffer.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, VarInitTbuffer) {
-  runFileTest("var.init.tbuffer.hlsl", FileTest::Expect::Warning);
+  runFileTest("var.init.tbuffer.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, VarInitOpaque) { runFileTest("var.init.opaque.hlsl"); }
 TEST_F(FileTest, VarInitCrossStorageClass) {
@@ -563,7 +563,7 @@ TEST_F(FileTest, SemanticArbitraryAlphaLocation) {
   runFileTest("semantic.arbitrary.location.alpha.hlsl");
 }
 TEST_F(FileTest, SemanticDuplication) {
-  runFileTest("semantic.duplication.hlsl", FileTest::Expect::Failure);
+  runFileTest("semantic.duplication.hlsl", Expect::Failure);
 }
 TEST_F(FileTest, SemanticOnStruct) { runFileTest("semantic.on-struct.hlsl"); }
 
@@ -931,7 +931,7 @@ TEST_F(FileTest, PrimitiveTriangleAdjGS) {
   runFileTest("primitive.triangleadj.gs.hlsl");
 }
 TEST_F(FileTest, PrimitiveErrorGS) {
-  runFileTest("primitive.error.gs.hlsl", FileTest::Expect::Failure);
+  runFileTest("primitive.error.gs.hlsl", Expect::Failure);
 }
 
 // SPIR-V specific
@@ -980,48 +980,43 @@ TEST_F(FileTest, SpirvInterpolation) {
   runFileTest("spirv.interpolation.hlsl");
 }
 TEST_F(FileTest, SpirvInterpolationError) {
-  runFileTest("spirv.interpolation.error.hlsl", FileTest::Expect::Failure);
+  runFileTest("spirv.interpolation.error.hlsl", Expect::Failure);
 }
 
 TEST_F(FileTest, SpirvLegalizationOpaqueStruct) {
   runFileTest("spirv.legal.opaque-struct.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
-              /*runValidation=*/false);
+              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferUsage) {
   runFileTest("spirv.legal.sbuffer.usage.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
-              /*runValidation=*/false);
+              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferMethods) {
   runFileTest("spirv.legal.sbuffer.methods.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
-              /*runValidation=*/false);
+              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferCounter) {
   runFileTest("spirv.legal.sbuffer.counter.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
-              /*runValidation=*/false);
+              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
 }
 TEST_F(FileTest, SpirvLegalizationStructuredBufferInStruct) {
   runFileTest("spirv.legal.sbuffer.struct.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
-              /*runValidation=*/false);
+              /*runValidation=*/true, /*relaxLogicalPointer=*/true);
 }
 TEST_F(FileTest, SpirvLegalizationConstantBuffer) {
   runFileTest("spirv.legal.cbuffer.hlsl");
 }
 TEST_F(FileTest, SpirvLegalizationTextureBuffer) {
   runFileTest("spirv.legal.tbuffer.hlsl", Expect::Success,
-              // The generated SPIR-V needs legalization.
+              // TODO: fix the different type error for OpStore
               /*runValidation=*/false);
 }
 
 TEST_F(FileTest, VulkanAttributeErrors) {
-  runFileTest("vk.attribute.error.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.attribute.error.hlsl", Expect::Failure);
 }
 TEST_F(FileTest, VulkanAttributeInvalidUsages) {
-  runFileTest("vk.attribute.invalid.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.attribute.invalid.hlsl", Expect::Failure);
 }
 
 TEST_F(FileTest, VulkanCLOptionIgnoreUnusedResources) {
@@ -1047,13 +1042,13 @@ TEST_F(FileTest, VulkanLocationInputImplicitOutputExplicit) {
   runFileTest("vk.location.exp-out.hlsl");
 }
 TEST_F(FileTest, VulkanLocationTooLarge) {
-  runFileTest("vk.location.large.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.location.large.hlsl", Expect::Failure);
 }
 TEST_F(FileTest, VulkanLocationReassigned) {
-  runFileTest("vk.location.reassign.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.location.reassign.hlsl", Expect::Failure);
 }
 TEST_F(FileTest, VulkanLocationPartiallyAssigned) {
-  runFileTest("vk.location.mixed.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.location.mixed.hlsl", Expect::Failure);
 }
 
 TEST_F(FileTest, VulkanExplicitBinding) {
@@ -1074,13 +1069,13 @@ TEST_F(FileTest, VulkanRegisterBindingShift) {
   runFileTest("vk.binding.cl.hlsl");
 }
 TEST_F(FileTest, VulkanExplicitBindingReassigned) {
-  runFileTest("vk.binding.explicit.error.hlsl", FileTest::Expect::Warning);
+  runFileTest("vk.binding.explicit.error.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, VulkanRegisterBindingReassigned) {
-  runFileTest("vk.binding.register.error.hlsl", FileTest::Expect::Warning);
+  runFileTest("vk.binding.register.error.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, VulkanRegisterBindingShiftReassigned) {
-  runFileTest("vk.binding.cl.error.hlsl", FileTest::Expect::Warning);
+  runFileTest("vk.binding.cl.error.hlsl", Expect::Warning);
 }
 TEST_F(FileTest, VulkanStructuredBufferCounter) {
   // [[vk::counter_binding()]] for RWStructuredBuffer, AppendStructuredBuffer,
@@ -1093,7 +1088,7 @@ TEST_F(FileTest, VulkanPushConstantOffset) {
   runFileTest("vk.push-constant.offset.hlsl");
 }
 TEST_F(FileTest, VulkanMultiplePushConstant) {
-  runFileTest("vk.push-constant.multiple.hlsl", FileTest::Expect::Failure);
+  runFileTest("vk.push-constant.multiple.hlsl", Expect::Failure);
 }
 
 TEST_F(FileTest, VulkanLayoutCBufferStd140) {

tools/clang/unittests/SPIRV/FileTestFixture.cpp

@@ -61,7 +61,10 @@ bool FileTest::parseInputFile() {
 }
 
 void FileTest::runFileTest(llvm::StringRef filename, Expect expect,
-                           bool runValidation) {
+                           bool runValidation, bool relaxLogicalPointer) {
+  if (relaxLogicalPointer)
+    assert(runValidation);
+
   inputFilePath = utils::getAbsPathOfInputDataFile(filename);
 
   // Parse the input file.
@@ -125,7 +128,8 @@ void FileTest::runFileTest(llvm::StringRef filename, Expect expect,
 
   // Run SPIR-V validation for successful compilations
   if (runValidation && expect != Expect::Failure) {
-    EXPECT_TRUE(utils::validateSpirvBinary(generatedBinary));
+    EXPECT_TRUE(
+        utils::validateSpirvBinary(generatedBinary, relaxLogicalPointer));
   }
 }
 

tools/clang/unittests/SPIRV/FileTestFixture.h

@@ -27,7 +27,7 @@ public:
 
   /// \brief Runs a File Test! (See class description for more info)
   void runFileTest(llvm::StringRef path, Expect expect = Expect::Success,
-                   bool runValidation = true);
+                   bool runValidation = true, bool relaxLogicalPointer = false);
 
 private:
   /// \brief Reads in the given input file.

tools/clang/unittests/SPIRV/FileTestUtils.cpp

@@ -32,12 +32,15 @@ bool disassembleSpirvBinary(std::vector<uint32_t> &binary,
   return spirvTools.Disassemble(binary, generatedSpirvAsm, options);
 }
 
-bool validateSpirvBinary(std::vector<uint32_t> &binary) {
+bool validateSpirvBinary(std::vector<uint32_t> &binary,
+                         bool relaxLogicalPointer) {
+  spvtools::ValidatorOptions options;
+  options.SetRelaxLogicalPointer(relaxLogicalPointer);
   spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_0);
   spirvTools.SetMessageConsumer(
       [](spv_message_level_t, const char *, const spv_position_t &,
          const char *message) { fprintf(stdout, "%s\n", message); });
-  return spirvTools.Validate(binary);
+  return spirvTools.Validate(binary.data(), binary.size(), options);
 }
 
 bool processRunCommandArgs(const llvm::StringRef runCommandLine,

tools/clang/unittests/SPIRV/FileTestUtils.h

@@ -32,7 +32,8 @@ bool disassembleSpirvBinary(std::vector<uint32_t> &binary,
 
 /// \brief Runs the SPIR-V Tools validation on the given SPIR-V binary.
 /// Returns true if validation is successful; false otherwise.
-bool validateSpirvBinary(std::vector<uint32_t> &binary);
+bool validateSpirvBinary(std::vector<uint32_t> &binary,
+                         bool relaxLogicalPointer);
 
 /// \brief Parses the Target Profile and Entry Point from the Run command
 /// Returns the target profile, entry point, and the rest via arguments.

tools/clang/unittests/SPIRV/WholeFileTestFixture.cpp

@@ -107,7 +107,8 @@ void WholeFileTest::runWholeFileTest(llvm::StringRef filename,
 
   // Run SPIR-V validation if requested.
   if (runSpirvValidation) {
-    EXPECT_TRUE(utils::validateSpirvBinary(generatedBinary));
+    EXPECT_TRUE(utils::validateSpirvBinary(generatedBinary,
+                                           /*relaxLogicalPointer=*/false));
   }
 }