[spirv] Add missing capability for storage image read/write (#870)

Refreshed SPIRV-Tools and re-enabled all validations for texure/
buffer methods.

Also added support for -Cc to color the disassembly.

external/SPIRV-Tools

@@ -1 +1 @@
-Subproject commit aec60b815806a0de1de4e56f747c4a9673601d73
+Subproject commit 22582fafa48f3699b477d1fc6f3eb90c539d30cb

tools/clang/include/clang/SPIRV/ModuleBuilder.h

@@ -13,6 +13,7 @@
 #include <string>
 #include <vector>
 
+#include "clang/AST/Type.h"
 #include "clang/SPIRV/InstBuilder.h"
 #include "clang/SPIRV/SPIRVContext.h"
 #include "clang/SPIRV/Structure.h"
@@ -186,13 +187,14 @@ public:
   /// OpImageFetch should be used for sampled images. OpImageRead should be used
   /// for images without a sampler.
   uint32_t createImageFetchOrRead(bool doImageFetch, uint32_t texelType,
-                                  uint32_t image, uint32_t coordinate,
-                                  uint32_t lod, uint32_t constOffset,
-                                  uint32_t varOffset, uint32_t constOffsets,
-                                  uint32_t sample);
+                                  QualType imageType, uint32_t image,
+                                  uint32_t coordinate, uint32_t lod,
+                                  uint32_t constOffset, uint32_t varOffset,
+                                  uint32_t constOffsets, uint32_t sample);
 
   /// \brief Creates SPIR-V instructions for writing to the given image.
-  void createImageWrite(uint32_t imageId, uint32_t coordId, uint32_t texelId);
+  void createImageWrite(QualType imageType, uint32_t imageId, uint32_t coordId,
+                        uint32_t texelId);
 
   /// \brief Creates SPIR-V instructions for gathering the given image.
   ///
@@ -424,7 +426,8 @@ void ModuleBuilder::setMemoryModel(spv::MemoryModel mm) {
 }
 
 void ModuleBuilder::requireCapability(spv::Capability cap) {
-  theModule.addCapability(cap);
+  if (cap != spv::Capability::Max)
+    theModule.addCapability(cap);
 }
 
 void ModuleBuilder::addEntryPoint(spv::ExecutionModel em, uint32_t targetId,

tools/clang/lib/SPIRV/ModuleBuilder.cpp

@@ -9,6 +9,7 @@
 
 #include "clang/SPIRV/ModuleBuilder.h"
 
+#include "TypeTranslator.h"
 #include "spirv/1.0//spirv.hpp11"
 #include "clang/SPIRV/InstBuilder.h"
 #include "llvm/llvm_assert/assert.h"
@@ -395,16 +396,18 @@ uint32_t ModuleBuilder::createImageSample(
   return texelId;
 }
 
-void ModuleBuilder::createImageWrite(uint32_t imageId, uint32_t coordId,
-                                     uint32_t texelId) {
+void ModuleBuilder::createImageWrite(QualType imageType, uint32_t imageId,
+                                     uint32_t coordId, uint32_t texelId) {
   assert(insertPoint && "null insert point");
+  requireCapability(
+      TypeTranslator::getCapabilityForStorageImageReadWrite(imageType));
   instBuilder.opImageWrite(imageId, coordId, texelId, llvm::None).x();
   insertPoint->appendInstruction(std::move(constructSite));
 }
 
 uint32_t ModuleBuilder::createImageFetchOrRead(
-    bool doImageFetch, uint32_t texelType, uint32_t image, uint32_t coordinate,
-    uint32_t lod, uint32_t constOffset, uint32_t varOffset,
+    bool doImageFetch, uint32_t texelType, QualType imageType, uint32_t image,
+    uint32_t coordinate, uint32_t lod, uint32_t constOffset, uint32_t varOffset,
     uint32_t constOffsets, uint32_t sample) {
   assert(insertPoint && "null insert point");
 
@@ -415,10 +418,13 @@ uint32_t ModuleBuilder::createImageFetchOrRead(
           constOffsets, sample, &params));
 
   const uint32_t texelId = theContext.takeNextId();
-  if (doImageFetch)
+  if (doImageFetch) {
     instBuilder.opImageFetch(texelType, texelId, image, coordinate, mask);
-  else
+  } else {
+    requireCapability(
+        TypeTranslator::getCapabilityForStorageImageReadWrite(imageType));
     instBuilder.opImageRead(texelType, texelId, image, coordinate, mask);
+  }
 
   for (const auto param : params)
     instBuilder.idRef(param);

tools/clang/lib/SPIRV/SPIRVEmitter.cpp

@@ -2294,9 +2294,10 @@ SpirvEvalInfo SPIRVEmitter::processBufferTextureLoad(const Expr *object,
 
   // OpImageFetch and OpImageRead can only fetch a vector of 4 elements.
   const uint32_t texelTypeId = theBuilder.getVecType(elemTypeId, 4u);
-  const uint32_t texel = theBuilder.createImageFetchOrRead(
-      doFetch, texelTypeId, objectId, locationId, lod, constOffset, varOffset,
-      /*constOffsets*/ 0, sampleNumber);
+  const uint32_t texel =
+      theBuilder.createImageFetchOrRead(doFetch, texelTypeId, type, objectId,
+                                        locationId, lod, constOffset, varOffset,
+                                        /*constOffsets*/ 0, sampleNumber);
 
   uint32_t retVal = texel;
   // If the result type is a vec1, vec2, or vec3, some extra processing
@@ -3916,9 +3917,10 @@ SPIRVEmitter::tryToAssignToRWBufferRWTexture(const Expr *lhs,
   const auto lhsExpr = dyn_cast<CXXOperatorCallExpr>(lhs);
   if (isBufferTextureIndexing(lhsExpr, &baseExpr, &indexExpr)) {
     const uint32_t locId = doExpr(indexExpr);
+    const QualType imageType = baseExpr->getType();
     const uint32_t imageId = theBuilder.createLoad(
-        typeTranslator.translateType(baseExpr->getType()), doExpr(baseExpr));
-    theBuilder.createImageWrite(imageId, locId, rhs);
+        typeTranslator.translateType(imageType), doExpr(baseExpr));
+    theBuilder.createImageWrite(imageType, imageId, locId, rhs);
     return rhs;
   }
   return 0;

tools/clang/lib/SPIRV/TypeTranslator.cpp

@@ -584,6 +584,20 @@ uint32_t TypeTranslator::getComponentVectorType(QualType matrixType) {
   return theBuilder.getVecType(elemType, colCount);
 }
 
+spv::Capability
+TypeTranslator::getCapabilityForStorageImageReadWrite(QualType type) {
+  if (const auto *rt = type->getAs<RecordType>()) {
+    const auto name = rt->getDecl()->getName();
+    // RWBuffer translates into OpTypeImage Buffer with Sampled = 2
+    if (name == "RWBuffer")
+      return spv::Capability::ImageBuffer;
+    // RWBuffer translates into OpTypeImage 1D with Sampled = 2
+    if (name == "RWTexture1D" || name == "RWTexture1DArray")
+      return spv::Capability::Image1D;
+  }
+  return spv::Capability::Max;
+}
+
 llvm::SmallVector<const Decoration *, 4>
 TypeTranslator::getLayoutDecorations(const DeclContext *decl, LayoutRule rule) {
   const auto spirvContext = theBuilder.getSPIRVContext();

tools/clang/lib/SPIRV/TypeTranslator.h

@@ -167,6 +167,10 @@ public:
   /// matrix type.
   uint32_t getComponentVectorType(QualType matrixType);
 
+  /// \brief Returns the capability required for the given storage image type.
+  /// Returns Capability::Max to mean no capability requirements.
+  static spv::Capability getCapabilityForStorageImageReadWrite(QualType type);
+
   /// \brief Generates layout decorations (Offset, MatrixStride, RowMajor,
   /// ColMajor) for the given type.
   ///

tools/clang/test/CodeGenSPIRV/method.buffer.load.hlsl

@@ -1,5 +1,7 @@
 // Run: %dxc -T ps_6_0 -E main
 
+// CHECK: OpCapability ImageBuffer
+
 Buffer<int> intbuf;
 Buffer<uint> uintbuf;
 Buffer<float> floatbuf;

tools/clang/test/CodeGenSPIRV/op.buffer.access.hlsl

@@ -1,5 +1,7 @@
 // Run: %dxc -T ps_6_0 -E main
 
+// CHECK: OpCapability ImageBuffer
+
 Buffer<int> intbuf;
 Buffer<uint> uintbuf;
 Buffer<float> floatbuf;

tools/clang/test/CodeGenSPIRV/op.rwbuffer.access.hlsl

@@ -1,5 +1,7 @@
 // Run: %dxc -T ps_6_0 -E main
 
+// CHECK: OpCapability ImageBuffer
+
 RWBuffer<int> intbuf;
 RWBuffer<uint> uintbuf;
 RWBuffer<float> floatbuf;

tools/clang/tools/dxc/dxc.cpp

@@ -69,7 +69,7 @@
 #include "spirv-tools/libspirv.hpp"
 
 static bool DisassembleSpirv(IDxcBlob *binaryBlob, IDxcLibrary *library,
-                             IDxcBlobEncoding **assemblyBlob,
+                             IDxcBlobEncoding **assemblyBlob, bool withColor,
                              bool withByteOffset) {
   if (!binaryBlob)
     return true;
@@ -87,6 +87,8 @@ static bool DisassembleSpirv(IDxcBlob *binaryBlob, IDxcLibrary *library,
   spvtools::SpirvTools spirvTools(SPV_ENV_VULKAN_1_0);
   uint32_t options = (SPV_BINARY_TO_TEXT_OPTION_FRIENDLY_NAMES |
                       SPV_BINARY_TO_TEXT_OPTION_INDENT);
+  if (withColor)
+    options |= SPV_BINARY_TO_TEXT_OPTION_COLOR;
   if (withByteOffset)
     options |= SPV_BINARY_TO_TEXT_OPTION_SHOW_BYTE_OFFSET;
 
@@ -321,6 +323,7 @@ int DxcContext::ActOnBlob(IDxcBlob *pBlob, IDxcBlob *pDebugBlob, LPCWSTR pDebugB
     IFT(m_dxcSupport.CreateInstance(CLSID_DxcLibrary, &pLibrary));
 
     if (DisassembleSpirv(pBlob, pLibrary, &pDisassembleResult,
+                         m_Opts.ColorCodeAssembly,
                          m_Opts.DisassembleByteOffset)) {
       if (!m_Opts.AssemblyCode.empty()) {
         WriteBlobToFile(pDisassembleResult, m_Opts.AssemblyCode);

tools/clang/unittests/SPIRV/CodeGenSPIRVTest.cpp

@@ -248,14 +248,8 @@ TEST_F(FileTest, OpStructAccess) { runFileTest("op.struct.access.hlsl"); }
 TEST_F(FileTest, OpArrayAccess) { runFileTest("op.array.access.hlsl"); }
 
 // For buffer accessing operator
-TEST_F(FileTest, OpBufferAccess) {
-  runFileTest("op.buffer.access.hlsl", Expect::Success,
-              /*runValidation=*/false);
-}
-TEST_F(FileTest, OpRWBufferAccess) {
-  runFileTest("op.rwbuffer.access.hlsl", Expect::Success,
-              /*runValidation=*/false);
-}
+TEST_F(FileTest, OpBufferAccess) { runFileTest("op.buffer.access.hlsl"); }
+TEST_F(FileTest, OpRWBufferAccess) { runFileTest("op.rwbuffer.access.hlsl"); }
 TEST_F(FileTest, OpCBufferAccess) { runFileTest("op.cbuffer.access.hlsl"); }
 TEST_F(FileTest, OpConstantBufferAccess) {
   runFileTest("op.constant-buffer.access.hlsl");
@@ -277,8 +271,7 @@ TEST_F(FileTest, OpRWTextureAccessRead) {
   runFileTest("op.rwtexture.access.read.hlsl");
 }
 TEST_F(FileTest, OpRWTextureAccessWrite) {
-  runFileTest("op.rwtexture.access.write.hlsl", Expect::Success,
-              /*runValidation=*/false);
+  runFileTest("op.rwtexture.access.write.hlsl");
 }
 
 // For Texture.mips[][] operator
@@ -601,12 +594,10 @@ TEST_F(FileTest, TextureArrayGatherCmp) {
   runFileTest("texture.array.gather-cmp.hlsl");
 }
 TEST_F(FileTest, TextureGatherCmpRed) {
-  runFileTest("texture.gather-cmp-red.hlsl", Expect::Success,
-              /*runValidation=*/false);
+  runFileTest("texture.gather-cmp-red.hlsl");
 }
 TEST_F(FileTest, TextureArrayGatherCmpRed) {
-  runFileTest("texture.array.gather-cmp-red.hlsl", Expect::Success,
-              /*runValidation=*/false);
+  runFileTest("texture.array.gather-cmp-red.hlsl");
 }
 TEST_F(FileTest, TextureArrayGatherCmpGreen) {
   runFileTest("texture.gather-cmp-green.hlsl", Expect::Failure);
@@ -680,19 +671,13 @@ TEST_F(FileTest, RWByteAddressBufferAtomicMethods) {
 }
 
 // For Buffer/RWBuffer methods
-TEST_F(FileTest, BufferLoad) {
-  runFileTest("method.buffer.load.hlsl", Expect::Success,
-              /*runValidation=*/false);
-}
+TEST_F(FileTest, BufferLoad) { runFileTest("method.buffer.load.hlsl"); }
 TEST_F(FileTest, BufferGetDimensions) {
   runFileTest("method.buffer.get-dimensions.hlsl");
 }
 
 // For RWTexture methods
-TEST_F(FileTest, RWTextureLoad) {
-  runFileTest("method.rwtexture.load.hlsl", Expect::Success,
-              /*runValidation=*/false);
-}
+TEST_F(FileTest, RWTextureLoad) { runFileTest("method.rwtexture.load.hlsl"); }
 TEST_F(FileTest, RWTextureGetDimensions) {
   runFileTest("method.rwtexture.get-dimensions.hlsl");
 }