spirv-reader: track access mode for ptr/ref

Fixed: tint:1041 tint:1103
Change-Id: Ief5f3da73c65700fe904e76683b9b25f4eca2169
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/104900
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: David Neto <dneto@google.com>
Reviewed-by: Ryan Harrison <rharrison@chromium.org>
Reviewed-by: Ben Clayton <bclayton@google.com>
Reviewed-by: David Neto <dneto@google.com>

src/tint/reader/spirv/function.cc

@@ -2593,7 +2593,6 @@ TypedExpression FunctionEmitter::MakeExpression(uint32_t id) {
             // Construct the reference type, mapping storage class correctly.
             const auto* type =
                 RemapPointerProperties(parser_impl_.ConvertType(inst->type_id(), PtrAs::Ref), id);
-            // TODO(crbug.com/tint/1041): Fix access mode
             return TypedExpression{type, create<ast::IdentifierExpression>(
                                              Source{}, builder_.Symbols().Register(name))};
         }
@@ -4859,10 +4858,15 @@ DefInfo::Pointer FunctionEmitter::GetPointerInfo(uint32_t id) {
             }
             case SpvOpFunctionParameter: {
                 const auto* type = As<Pointer>(parser_impl_.ConvertType(inst.type_id()));
-                // TODO(crbug.com/tint/1041): Add access mode.
+                // For access mode, kUndefined is ok for now, since the
-                // Using kUndefined is ok for now, since the only non-default access mode
+                // only non-default access mode on a pointer would be for a storage
-                // on a pointer would be for a storage buffer, and baseline SPIR-V doesn't
+                // buffer, and baseline SPIR-V doesn't allow passing pointers to
-                // allow passing pointers to buffers as function parameters.
+                // buffers as function parameters.
+                // If/when the SPIR-V path supports variable pointers, then we
+                // can pointers to read-only storage buffers passed as
+                // parameters.  In that case we need to do a global analysis to
+                // determine what the formal argument parameter type should be,
+                // whether it has read_only or read_write access mode.
                 return DefInfo::Pointer{type->address_space, ast::Access::kUndefined};
             }
             default:
@@ -4899,13 +4903,11 @@ DefInfo::Pointer FunctionEmitter::GetPointerInfo(uint32_t id) {
 const Type* FunctionEmitter::RemapPointerProperties(const Type* type, uint32_t result_id) {
     if (auto* ast_ptr_type = As<Pointer>(type)) {
         const auto pi = GetPointerInfo(result_id);
-        // TODO(crbug.com/tin/t1041): also do access mode
+        return ty_.Pointer(ast_ptr_type->type, pi.address_space, pi.access);
-        return ty_.Pointer(ast_ptr_type->type, pi.address_space);
     }
     if (auto* ast_ptr_type = As<Reference>(type)) {
         const auto pi = GetPointerInfo(result_id);
-        // TODO(crbug.com/tin/t1041): also do access mode
+        return ty_.Reference(ast_ptr_type->type, pi.address_space, pi.access);
-        return ty_.Reference(ast_ptr_type->type, pi.address_space);
     }
     return type;
 }

src/tint/reader/spirv/function_memory_test.cc

@@ -858,7 +858,52 @@ fn main() {
     EXPECT_EQ(got, expected) << got;
 }
 
-std::string OldStorageBufferPreamble() {
+std::string NewStorageBufferPreamble(bool nonwritable = false) {
+    // Declare a buffer with
+    //  StorageBuffer storage class
+    //  Block struct decoration
+    return std::string(R"(
+     OpCapability Shader
+     OpExtension "SPV_KHR_storage_buffer_storage_class"
+     OpMemoryModel Logical Simple
+     OpEntryPoint Fragment %100 "main"
+     OpExecutionMode %100 OriginUpperLeft
+     OpName %myvar "myvar"
+
+     OpDecorate %myvar DescriptorSet 0
+     OpDecorate %myvar Binding 0
+
+     OpDecorate %struct Block
+     OpMemberDecorate %struct 0 Offset 0
+     OpMemberDecorate %struct 1 Offset 4
+     OpDecorate %arr ArrayStride 4
+     )") +
+           (nonwritable ? R"(
+     OpMemberDecorate %struct 0 NonWritable
+     OpMemberDecorate %struct 1 NonWritable)"
+                        : "") +
+           R"(
+     %void = OpTypeVoid
+     %voidfn = OpTypeFunction %void
+     %uint = OpTypeInt 32 0
+
+     %uint_0 = OpConstant %uint 0
+     %uint_1 = OpConstant %uint 1
+
+     %arr = OpTypeRuntimeArray %uint
+     %struct = OpTypeStruct %uint %arr
+     %ptr_struct = OpTypePointer StorageBuffer %struct
+     %ptr_uint = OpTypePointer StorageBuffer %uint
+
+     %myvar = OpVariable %ptr_struct StorageBuffer
+  )";
+}
+
+std::string OldStorageBufferPreamble(bool nonwritable = false) {
+    // Declare a buffer with
+    //  Unifrom storage class
+    //  BufferBlock struct decoration
+    // This is the deprecated way to declare a storage buffer.
     return Preamble() + R"(
      OpName %myvar "myvar"
 
@@ -869,7 +914,12 @@ std::string OldStorageBufferPreamble() {
      OpMemberDecorate %struct 0 Offset 0
      OpMemberDecorate %struct 1 Offset 4
      OpDecorate %arr ArrayStride 4
-
+     )" +
+           (nonwritable ? R"(
+     OpMemberDecorate %struct 0 NonWritable
+     OpMemberDecorate %struct 1 NonWritable)"
+                        : "") +
+           R"(
      %void = OpTypeVoid
      %voidfn = OpTypeFunction %void
      %uint = OpTypeInt 32 0
@@ -938,7 +988,7 @@ myvar.field1[1u] = 0u;
 )"));
 }
 
-TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice) {
+TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice_ReadWrite) {
     // Use the pointer value twice, which provokes the spirv-reader
     // to make a let declaration for the pointer.  The storage class
     // must be 'storage', not 'uniform'.
@@ -966,15 +1016,130 @@ TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_Us
     EXPECT_TRUE(fe.EmitBody()) << p->error();
     auto ast_body = fe.ast_body();
     const auto got = test::ToString(p->program(), ast_body);
-    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32> = &(myvar.field0);
+    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32, read_write> = &(myvar.field0);
 *(x_1) = 0u;
 *(x_1) = 0u;
-let x_2 : ptr<storage, u32> = &(myvar.field1[1u]);
+let x_2 : ptr<storage, u32, read_write> = &(myvar.field1[1u]);
 let x_3 : u32 = *(x_2);
 *(x_2) = 0u;
 )"));
 }
 
+TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice_ReadOnly) {
+    // Like the previous test, but make the storage buffer read_only.
+    // The pointer type must also be read-only.
+    const auto assembly = OldStorageBufferPreamble(true) + R"(
+  %100 = OpFunction %void None %voidfn
+  %entry = OpLabel
+
+  ; the scalar element
+  %1 = OpAccessChain %ptr_uint %myvar %uint_0
+  OpStore %1 %uint_0
+  OpStore %1 %uint_0
+
+  ; element in the runtime array
+  %2 = OpAccessChain %ptr_uint %myvar %uint_1 %uint_1
+  ; Use the pointer twice
+  %3 = OpLoad %uint %2
+  OpStore %2 %uint_0
+
+  OpReturn
+  OpFunctionEnd
+)";
+    auto p = parser(test::Assemble(assembly));
+    ASSERT_TRUE(p->BuildAndParseInternalModule()) << assembly << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    const auto got = test::ToString(p->program(), ast_body);
+    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32, read> = &(myvar.field0);
+*(x_1) = 0u;
+*(x_1) = 0u;
+let x_2 : ptr<storage, u32, read> = &(myvar.field1[1u]);
+let x_3 : u32 = *(x_2);
+*(x_2) = 0u;
+)")) << got
+     << assembly;
+}
+
+TEST_F(SpvParserMemoryTest, StorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice_ReadWrite) {
+    // Use new style storage buffer declaration:
+    //   StorageBuffer storage class,
+    //   Block decoration
+    // The pointer type must use 'storage' address space, and should use defaulted access
+    const auto assembly = NewStorageBufferPreamble() + R"(
+  %100 = OpFunction %void None %voidfn
+  %entry = OpLabel
+
+  ; the scalar element
+  %1 = OpAccessChain %ptr_uint %myvar %uint_0
+  OpStore %1 %uint_0
+  OpStore %1 %uint_0
+
+  ; element in the runtime array
+  %2 = OpAccessChain %ptr_uint %myvar %uint_1 %uint_1
+  ; Use the pointer twice
+  %3 = OpLoad %uint %2
+  OpStore %2 %uint_0
+
+  OpReturn
+  OpFunctionEnd
+)";
+    auto p = parser(test::Assemble(assembly));
+    ASSERT_TRUE(p->BuildAndParseInternalModule()) << assembly << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    const auto got = test::ToString(p->program(), ast_body);
+    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32, read_write> = &(myvar.field0);
+*(x_1) = 0u;
+*(x_1) = 0u;
+let x_2 : ptr<storage, u32, read_write> = &(myvar.field1[1u]);
+let x_3 : u32 = *(x_2);
+*(x_2) = 0u;
+)")) << got;
+}
+
+TEST_F(SpvParserMemoryTest, StorageBuffer_ThroughAccessChain_NonCascaded_UsedTwice_ReadOnly) {
+    // Like the previous test, but make the storage buffer read_only.
+    // Use new style storage buffer declaration:
+    //   StorageBuffer storage class,
+    //   Block decoration
+    // The pointer type must use 'storage' address space, and must use read_only
+    // access
+    const auto assembly = NewStorageBufferPreamble(true) + R"(
+  %100 = OpFunction %void None %voidfn
+  %entry = OpLabel
+
+  ; the scalar element
+  %1 = OpAccessChain %ptr_uint %myvar %uint_0
+  OpStore %1 %uint_0
+  OpStore %1 %uint_0
+
+  ; element in the runtime array
+  %2 = OpAccessChain %ptr_uint %myvar %uint_1 %uint_1
+  ; Use the pointer twice
+  %3 = OpLoad %uint %2
+  OpStore %2 %uint_0
+
+  OpReturn
+  OpFunctionEnd
+)";
+    auto p = parser(test::Assemble(assembly));
+    ASSERT_TRUE(p->BuildAndParseInternalModule()) << assembly << p->error();
+    auto fe = p->function_emitter(100);
+    EXPECT_TRUE(fe.EmitBody()) << p->error();
+    auto ast_body = fe.ast_body();
+    const auto got = test::ToString(p->program(), ast_body);
+    EXPECT_THAT(got, HasSubstr(R"(let x_1 : ptr<storage, u32, read> = &(myvar.field0);
+*(x_1) = 0u;
+*(x_1) = 0u;
+let x_2 : ptr<storage, u32, read> = &(myvar.field1[1u]);
+let x_3 : u32 = *(x_2);
+*(x_2) = 0u;
+)")) << got;
+}
+
 TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughAccessChain_NonCascaded_InBoundsAccessChain) {
     // Like the previous test, but using OpInBoundsAccessChain.
     const auto assembly = OldStorageBufferPreamble() + R"(
@@ -1050,7 +1215,7 @@ TEST_F(SpvParserMemoryTest, RemapStorageBuffer_ThroughCopyObject_WithoutHoisting
     EXPECT_TRUE(fe.EmitBody()) << p->error();
     auto ast_body = fe.ast_body();
     EXPECT_THAT(test::ToString(p->program(), ast_body),
-                HasSubstr(R"(let x_2 : ptr<storage, u32> = &(myvar.field1[1u]);
+                HasSubstr(R"(let x_2 : ptr<storage, u32, read_write> = &(myvar.field1[1u]);
 *(x_2) = 0u;
 )")) << p->error();
 
@@ -1128,7 +1293,7 @@ TEST_F(SpvParserMemoryTest, ArrayLength_FromCopyObject) {
     EXPECT_TRUE(fe.EmitBody()) << p->error();
     auto ast_body = fe.ast_body();
     const auto body_str = test::ToString(p->program(), ast_body);
-    EXPECT_THAT(body_str, HasSubstr(R"(let x_2 : ptr<storage, S> = &(myvar);
+    EXPECT_THAT(body_str, HasSubstr(R"(let x_2 : ptr<storage, S, read_write> = &(myvar);
 let x_1 : u32 = arrayLength(&((*(x_2)).rtarr));
 )")) << body_str;
 

src/tint/reader/spirv/parser_type.cc

@@ -50,11 +50,15 @@ namespace tint::reader::spirv {
 
 namespace {
 struct PointerHasher {
-    size_t operator()(const Pointer& t) const { return utils::Hash(t.type, t.address_space); }
+    size_t operator()(const Pointer& t) const {
+        return utils::Hash(t.type, t.address_space, t.access);
+    }
 };
 
 struct ReferenceHasher {
-    size_t operator()(const Reference& t) const { return utils::Hash(t.type, t.address_space); }
+    size_t operator()(const Reference& t) const {
+        return utils::Hash(t.type, t.address_space, t.access);
+    }
 };
 
 struct VectorHasher {
@@ -107,10 +111,10 @@ struct StorageTextureHasher {
 // Equality operators
 //! @cond Doxygen_Suppress
 static bool operator==(const Pointer& a, const Pointer& b) {
-    return a.type == b.type && a.address_space == b.address_space;
+    return a.type == b.type && a.address_space == b.address_space && a.access == b.access;
 }
 static bool operator==(const Reference& a, const Reference& b) {
-    return a.type == b.type && a.address_space == b.address_space;
+    return a.type == b.type && a.address_space == b.address_space && a.access == b.access;
 }
 static bool operator==(const Vector& a, const Vector& b) {
     return a.type == b.type && a.size == b.size;
@@ -170,14 +174,16 @@ Type::~Type() = default;
 
 Texture::~Texture() = default;
 
-Pointer::Pointer(const Type* t, ast::AddressSpace s) : type(t), address_space(s) {}
+Pointer::Pointer(const Type* t, ast::AddressSpace s, ast::Access a)
+    : type(t), address_space(s), access(a) {}
 Pointer::Pointer(const Pointer&) = default;
 
 const ast::Type* Pointer::Build(ProgramBuilder& b) const {
-    return b.ty.pointer(type->Build(b), address_space);
+    return b.ty.pointer(type->Build(b), address_space, access);
 }
 
-Reference::Reference(const Type* t, ast::AddressSpace s) : type(t), address_space(s) {}
+Reference::Reference(const Type* t, ast::AddressSpace s, ast::Access a)
+    : type(t), address_space(s), access(a) {}
 Reference::Reference(const Reference&) = default;
 
 const ast::Type* Reference::Build(ProgramBuilder& b) const {
@@ -438,12 +444,16 @@ const spirv::I32* TypeManager::I32() {
     return state->i32_;
 }
 
-const spirv::Pointer* TypeManager::Pointer(const Type* el, ast::AddressSpace address_space) {
+const spirv::Pointer* TypeManager::Pointer(const Type* el,
-    return state->pointers_.Get(el, address_space);
+                                           ast::AddressSpace address_space,
+                                           ast::Access access) {
+    return state->pointers_.Get(el, address_space, access);
 }
 
-const spirv::Reference* TypeManager::Reference(const Type* el, ast::AddressSpace address_space) {
+const spirv::Reference* TypeManager::Reference(const Type* el,
-    return state->references_.Get(el, address_space);
+                                               ast::AddressSpace address_space,
+                                               ast::Access access) {
+    return state->references_.Get(el, address_space, access);
 }
 
 const spirv::Vector* TypeManager::Vector(const Type* el, uint32_t size) {

src/tint/reader/spirv/parser_type.h

@@ -157,12 +157,13 @@ struct I32 final : public Castable<I32, Type> {
 #endif  // NDEBUG
 };
 
-/// `ptr<SC, T>` type
+/// `ptr<SC, T, AM>` type
 struct Pointer final : public Castable<Pointer, Type> {
     /// Constructor
     /// @param ty the store type
     /// @param sc the pointer address space
-    Pointer(const Type* ty, ast::AddressSpace sc);
+    /// @param access the declared access mode
+    Pointer(const Type* ty, ast::AddressSpace sc, ast::Access access);
 
     /// Copy constructor
     /// @param other the other type to copy
@@ -181,16 +182,19 @@ struct Pointer final : public Castable<Pointer, Type> {
     Type const* const type;
     /// the pointer address space
     ast::AddressSpace const address_space;
+    /// the pointer declared access mode
+    ast::Access const access;
 };
 
-/// `ref<SC, T>` type
+/// `ref<SC, T, AM>` type
 /// Note this has no AST representation, but is used for type tracking in the
 /// reader.
 struct Reference final : public Castable<Reference, Type> {
     /// Constructor
     /// @param ty the referenced type
     /// @param sc the reference address space
-    Reference(const Type* ty, ast::AddressSpace sc);
+    /// @param access the reference declared access mode
+    Reference(const Type* ty, ast::AddressSpace sc, ast::Access access);
 
     /// Copy constructor
     /// @param other the other type to copy
@@ -209,6 +213,8 @@ struct Reference final : public Castable<Reference, Type> {
     Type const* const type;
     /// the pointer address space
     ast::AddressSpace const address_space;
+    /// the pointer declared access mode
+    ast::Access const access;
 };
 
 /// `vecN<T>` type
@@ -535,14 +541,20 @@ class TypeManager {
     const spirv::I32* I32();
     /// @param ty the store type
     /// @param address_space the pointer address space
+    /// @param access the declared access mode
     /// @return a Pointer type. Repeated calls with the same arguments will return
     /// the same pointer.
-    const spirv::Pointer* Pointer(const Type* ty, ast::AddressSpace address_space);
+    const spirv::Pointer* Pointer(const Type* ty,
+                                  ast::AddressSpace address_space,
+                                  ast::Access access = ast::Access::kUndefined);
     /// @param ty the referenced type
     /// @param address_space the reference address space
+    /// @param access the declared access mode
     /// @return a Reference type. Repeated calls with the same arguments will
     /// return the same pointer.
-    const spirv::Reference* Reference(const Type* ty, ast::AddressSpace address_space);
+    const spirv::Reference* Reference(const Type* ty,
+                                      ast::AddressSpace address_space,
+                                      ast::Access access = ast::Access::kUndefined);
     /// @param ty the element type
     /// @param sz the number of elements in the vector
     /// @return a Vector type. Repeated calls with the same arguments will return