spirv-reader: apply access control to storage buffers

- Apply the AccessControlType wrappar around the struct type for any
  variable in the StorageBuffer storage class.
- Drop the NonWritable member decorations for the struct type.

Bug: tint:108
Change-Id: I6496c8c3e8b5d92b2ed0071385915d2b8065a80d
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/31020
Reviewed-by: dan sinclair <dsinclair@chromium.org>
Commit-Queue: dan sinclair <dsinclair@chromium.org>

src/reader/spirv/function_memory_test.cc

@@ -808,7 +808,7 @@ TEST_F(SpvParserTest, RemapStorageBuffer_TypesAndVarDeclarations) {
   Variable{
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   })"));
 }
 

src/reader/spirv/parser_impl.cc

@@ -51,6 +51,7 @@
 #include "src/ast/struct_member.h"
 #include "src/ast/struct_member_decoration.h"
 #include "src/ast/struct_member_offset_decoration.h"
+#include "src/ast/type/access_control_type.h"
 #include "src/ast/type/alias_type.h"
 #include "src/ast/type/array_type.h"
 #include "src/ast/type/bool_type.h"
@@ -391,9 +392,10 @@ ParserImpl::ConvertMemberDecoration(uint32_t struct_type_id,
       }
       return std::make_unique<ast::StructMemberOffsetDecoration>(decoration[1]);
     case SpvDecorationNonReadable:
+      // WGSL doesn't have a member decoration for this.  Silently drop it.
+      return nullptr;
     case SpvDecorationNonWritable:
-      // TODO(dneto): Drop these for now.
+      // WGSL doesn't have a member decoration for this.
-      // https://github.com/gpuweb/gpuweb/issues/935
       return nullptr;
     case SpvDecorationColMajor:
       // WGSL only supports column major matrices.
@@ -813,6 +815,7 @@ ast::type::Type* ParserImpl::ConvertType(
   // Compute members
   ast::StructMemberList ast_members;
   const auto members = struct_ty->element_types();
+  unsigned num_non_writable_members = 0;
   for (uint32_t member_index = 0; member_index < members.size();
        ++member_index) {
     const auto member_type_id = type_mgr_->GetId(members[member_index]);
@@ -822,6 +825,7 @@ ast::type::Type* ParserImpl::ConvertType(
       return nullptr;
     }
     ast::StructMemberDecorationList ast_member_decorations;
+    bool is_non_writable = false;
     for (auto& decoration : GetDecorationsForMember(type_id, member_index)) {
       if (decoration.empty()) {
         Fail() << "malformed SPIR-V decoration: it's empty";
@@ -844,6 +848,11 @@ ast::type::Type* ParserImpl::ConvertType(
         }
         Fail() << "unrecognized builtin " << decoration[1];
         return nullptr;
+      } else if (decoration[0] == SpvDecorationNonWritable) {
+        // WGSL doesn't represent individual members as non-writable. Instead,
+        // apply the ReadOnly access control to the containing struct if all
+        // the members are non-writable.
+        is_non_writable = true;
       } else {
         auto ast_member_decoration =
             ConvertMemberDecoration(type_id, member_index, decoration);
@@ -855,6 +864,11 @@ ast::type::Type* ParserImpl::ConvertType(
         }
       }
     }
+    if (is_non_writable) {
+      // Count a member as non-writable only once, no matter how many
+      // NonWritable decorations are applied to it.
+      ++num_non_writable_members;
+    }
     const auto member_name = namer_.GetMemberName(type_id, member_index);
     auto ast_struct_member = std::make_unique<ast::StructMember>(
         member_name, ast_member_ty, std::move(ast_member_decorations));
@@ -871,6 +885,9 @@ ast::type::Type* ParserImpl::ConvertType(
 
   auto* result = ctx_.type_mgr().Get(std::move(ast_struct_type));
   id_to_type_[type_id] = result;
+  if (num_non_writable_members == members.size()) {
+    read_only_struct_types_.insert(result);
+  }
   ast_module_.AddConstructedType(result);
   return result;
 }
@@ -1058,6 +1075,16 @@ std::unique_ptr<ast::Variable> ParserImpl::MakeVariable(uint32_t id,
     Fail() << "internal error: can't make ast::Variable for null type";
     return nullptr;
   }
+
+  if (sc == ast::StorageClass::kStorageBuffer) {
+    // Apply the access(read) or access(read_write) modifier.
+    auto access = read_only_struct_types_.count(type)
+                      ? ast::AccessControl::kReadOnly
+                      : ast::AccessControl::kReadWrite;
+    type = ctx_.type_mgr().Get(
+        std::make_unique<ast::type::AccessControlType>(access, type));
+  }
+
   auto ast_var = std::make_unique<ast::Variable>(namer_.Name(id), sc, type);
 
   ast::VariableDecorationList ast_decorations;

src/reader/spirv/parser_impl.h

@@ -485,6 +485,9 @@ class ParserImpl : Reader {
   // and Block decoration.
   std::unordered_set<uint32_t> remap_buffer_block_type_;
 
+  // The struct types with only read-only members.
+  std::unordered_set<ast::type::Type*> read_only_struct_types_;
+
   // Maps function_id to a list of entrypoint information
   std::unordered_map<uint32_t, std::vector<EntryPointInfo>>
       function_to_ep_info_;

src/reader/spirv/parser_impl_module_var_test.cc

@@ -1203,7 +1203,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_DescriptorSetDecoration_Valid) {
     }
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   })"))
       << module_str;
 }
@@ -1257,7 +1257,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_BindingDecoration_Valid) {
     }
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   })"))
       << module_str;
 }
@@ -1292,7 +1292,8 @@ TEST_F(SpvParserTest,
          "instruction, found '4'."));
 }
 
-TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) {
+TEST_F(SpvParserTest,
+       ModuleScopeVar_StructMember_NonReadableDecoration_Dropped) {
   auto* p = parser(test::Assemble(R"(
      OpName %myvar "myvar"
      OpDecorate %strct Block
@@ -1301,7 +1302,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) {
      %ptr_sb_strct = OpTypePointer StorageBuffer %strct
      %myvar = OpVariable %ptr_sb_strct StorageBuffer
   )"));
-  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions());
   EXPECT_TRUE(p->error().empty());
   const auto module_str = p->module().to_str();
   EXPECT_THAT(module_str, HasSubstr(R"(
@@ -1314,36 +1315,9 @@ TEST_F(SpvParserTest, ModuleScopeVar_NonReadableDecoration_DroppedForNow) {
   Variable{
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   }
-})")) << module_str;
+)")) << module_str;
-}
-
-TEST_F(SpvParserTest, ModuleScopeVar_NonWritableDecoration_DroppedForNow) {
-  auto* p = parser(test::Assemble(R"(
-     OpName %myvar "myvar"
-     OpDecorate %strct Block
-     OpMemberDecorate %strct 0 NonWritable
-)" + CommonTypes() + R"(
-     %ptr_sb_strct = OpTypePointer StorageBuffer %strct
-     %myvar = OpVariable %ptr_sb_strct StorageBuffer
-  )"));
-  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
-  EXPECT_TRUE(p->error().empty());
-  const auto module_str = p->module().to_str();
-  EXPECT_THAT(module_str, HasSubstr(R"(
-  S Struct{
-    [[block]]
-    StructMember{field0: __u32}
-    StructMember{field1: __f32}
-    StructMember{field2: __array__u32_2}
-  }
-  Variable{
-    myvar
-    storage_buffer
-    __struct_S
-  }
-})")) << module_str;
 }
 
 TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) {
@@ -1370,7 +1344,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) {
   Variable{
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   }
 })")) << module_str;
 }
@@ -1399,7 +1373,7 @@ TEST_F(SpvParserTest, ModuleScopeVar_MatrixStrideDecoration_Dropped) {
   Variable{
     myvar
     storage_buffer
-    __struct_S
+    __access_control_read_write__struct_S
   }
 })")) << module_str;
 }
@@ -1424,6 +1398,97 @@ TEST_F(SpvParserTest, ModuleScopeVar_RowMajorDecoration_IsError) {
       << p->error();
 }
 
+TEST_F(SpvParserTest, ModuleScopeVar_StorageBuffer_NonWritable_AllMembers) {
+  // Variable should have access(read)
+  auto* p = parser(test::Assemble(R"(
+     OpName %myvar "myvar"
+     OpDecorate %s Block
+     OpMemberDecorate %s 0 NonWritable
+     OpMemberDecorate %s 1 NonWritable
+     %float = OpTypeFloat 32
+
+     %s = OpTypeStruct %float %float
+     %ptr_sb_s = OpTypePointer StorageBuffer %s
+     %myvar = OpVariable %ptr_sb_s StorageBuffer
+  )"));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  EXPECT_TRUE(p->error().empty());
+  const auto module_str = p->module().to_str();
+  EXPECT_THAT(module_str, HasSubstr(R"(
+  S Struct{
+    [[block]]
+    StructMember{field0: __f32}
+    StructMember{field1: __f32}
+  }
+  Variable{
+    myvar
+    storage_buffer
+    __access_control_read_only__struct_S
+  }
+})")) << module_str;
+}
+
+TEST_F(SpvParserTest, ModuleScopeVar_StorageBuffer_NonWritable_NotAllMembers) {
+  // Variable should have access(read_write)
+  auto* p = parser(test::Assemble(R"(
+     OpName %myvar "myvar"
+     OpDecorate %s Block
+     OpMemberDecorate %s 0 NonWritable
+     %float = OpTypeFloat 32
+
+     %s = OpTypeStruct %float %float
+     %ptr_sb_s = OpTypePointer StorageBuffer %s
+     %myvar = OpVariable %ptr_sb_s StorageBuffer
+  )"));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  EXPECT_TRUE(p->error().empty());
+  const auto module_str = p->module().to_str();
+  EXPECT_THAT(module_str, HasSubstr(R"(
+  S Struct{
+    [[block]]
+    StructMember{field0: __f32}
+    StructMember{field1: __f32}
+  }
+  Variable{
+    myvar
+    storage_buffer
+    __access_control_read_write__struct_S
+  }
+})")) << module_str;
+}
+
+TEST_F(
+    SpvParserTest,
+    ModuleScopeVar_StorageBuffer_NonWritable_NotAllMembers_DuplicatedOnSameMember) {
+  // Variable should have access(read_write)
+  auto* p = parser(test::Assemble(R"(
+     OpName %myvar "myvar"
+     OpDecorate %s Block
+     OpMemberDecorate %s 0 NonWritable
+     OpMemberDecorate %s 0 NonWritable ; same member. Don't double-count it
+     %float = OpTypeFloat 32
+
+     %s = OpTypeStruct %float %float
+     %ptr_sb_s = OpTypePointer StorageBuffer %s
+     %myvar = OpVariable %ptr_sb_s StorageBuffer
+  )"));
+  ASSERT_TRUE(p->BuildAndParseInternalModuleExceptFunctions()) << p->error();
+  EXPECT_TRUE(p->error().empty());
+  const auto module_str = p->module().to_str();
+  EXPECT_THAT(module_str, HasSubstr(R"(
+  S Struct{
+    [[block]]
+    StructMember{field0: __f32}
+    StructMember{field1: __f32}
+  }
+  Variable{
+    myvar
+    storage_buffer
+    __access_control_read_write__struct_S
+  }
+})")) << module_str;
+}
+
 }  // namespace
 }  // namespace spirv
 }  // namespace reader