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>
diff --git a/src/tint/reader/spirv/function.cc b/src/tint/reader/spirv/function.cc
index 79c55de..9c38818 100644
--- a/src/tint/reader/spirv/function.cc
+++ b/src/tint/reader/spirv/function.cc
@@ -2593,7 +2593,6 @@
// 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 @@
}
case SpvOpFunctionParameter: {
const auto* type = As<Pointer>(parser_impl_.ConvertType(inst.type_id()));
- // TODO(crbug.com/tint/1041): Add access mode.
- // Using kUndefined is ok for now, since the only non-default access mode
- // on a pointer would be for a storage buffer, and baseline SPIR-V doesn't
- // allow passing pointers to buffers as function parameters.
+ // For access mode, kUndefined is ok for now, since the
+ // only non-default access mode on a pointer would be for a storage
+ // buffer, and baseline SPIR-V doesn't allow passing pointers to
+ // 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 @@
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);
+ return ty_.Pointer(ast_ptr_type->type, pi.address_space, pi.access);
}
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);
+ return ty_.Reference(ast_ptr_type->type, pi.address_space, pi.access);
}
return type;
}
diff --git a/src/tint/reader/spirv/function_memory_test.cc b/src/tint/reader/spirv/function_memory_test.cc
index a64aac5..66979ae 100644
--- a/src/tint/reader/spirv/function_memory_test.cc
+++ b/src/tint/reader/spirv/function_memory_test.cc
@@ -858,7 +858,52 @@
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 @@
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 @@
)"));
}
-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 @@
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 @@
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 @@
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;
diff --git a/src/tint/reader/spirv/parser_type.cc b/src/tint/reader/spirv/parser_type.cc
index 256444a..ff4b96e 100644
--- a/src/tint/reader/spirv/parser_type.cc
+++ b/src/tint/reader/spirv/parser_type.cc
@@ -50,11 +50,15 @@
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 @@
// 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 @@
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 @@
return state->i32_;
}
-const spirv::Pointer* TypeManager::Pointer(const Type* el, ast::AddressSpace address_space) {
- return state->pointers_.Get(el, address_space);
+const spirv::Pointer* TypeManager::Pointer(const Type* el,
+ 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) {
- return state->references_.Get(el, address_space);
+const spirv::Reference* TypeManager::Reference(const Type* el,
+ 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) {
diff --git a/src/tint/reader/spirv/parser_type.h b/src/tint/reader/spirv/parser_type.h
index 4c67f7a..48b6f3f 100644
--- a/src/tint/reader/spirv/parser_type.h
+++ b/src/tint/reader/spirv/parser_type.h
@@ -157,12 +157,13 @@
#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 @@
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 @@
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 @@
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
