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>
diff --git a/src/reader/spirv/function_memory_test.cc b/src/reader/spirv/function_memory_test.cc
index 984ee84..7291980 100644
--- a/src/reader/spirv/function_memory_test.cc
+++ b/src/reader/spirv/function_memory_test.cc
@@ -808,7 +808,7 @@
Variable{
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
})"));
}
diff --git a/src/reader/spirv/parser_impl.cc b/src/reader/spirv/parser_impl.cc
index 0f43943..d57ee6c 100644
--- a/src/reader/spirv/parser_impl.cc
+++ b/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 @@
}
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.
- // https://github.com/gpuweb/gpuweb/issues/935
+ // WGSL doesn't have a member decoration for this.
return nullptr;
case SpvDecorationColMajor:
// WGSL only supports column major matrices.
@@ -813,6 +815,7 @@
// 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 @@
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 @@
}
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 @@
}
}
}
+ 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 @@
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 @@
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;
diff --git a/src/reader/spirv/parser_impl.h b/src/reader/spirv/parser_impl.h
index 555c77f..7becccd 100644
--- a/src/reader/spirv/parser_impl.h
+++ b/src/reader/spirv/parser_impl.h
@@ -485,6 +485,9 @@
// 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_;
diff --git a/src/reader/spirv/parser_impl_module_var_test.cc b/src/reader/spirv/parser_impl_module_var_test.cc
index 253ec6a..467d79a 100644
--- a/src/reader/spirv/parser_impl_module_var_test.cc
+++ b/src/reader/spirv/parser_impl_module_var_test.cc
@@ -1203,7 +1203,7 @@
}
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
})"))
<< module_str;
}
@@ -1257,7 +1257,7 @@
}
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
})"))
<< module_str;
}
@@ -1292,7 +1292,8 @@
"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 @@
%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 @@
Variable{
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
}
-})")) << 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;
+)")) << module_str;
}
TEST_F(SpvParserTest, ModuleScopeVar_ColMajorDecoration_Dropped) {
@@ -1370,7 +1344,7 @@
Variable{
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
}
})")) << module_str;
}
@@ -1399,7 +1373,7 @@
Variable{
myvar
storage_buffer
- __struct_S
+ __access_control_read_write__struct_S
}
})")) << module_str;
}
@@ -1424,6 +1398,97 @@
<< 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
