validation: atomics access mode and storage class
Bug: tint:901 tint:909
Change-Id: Ibbcdd80ddbe2aa9940bbd73bb404349afc633836
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/60080
Auto-Submit: Sarah Mashayekhi <sarahmashay@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Sarah Mashayekhi <sarahmashay@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
diff --git a/src/resolver/atomics_validation_test.cc b/src/resolver/atomics_validation_test.cc
index 73181dc..15aa2d6 100644
--- a/src/resolver/atomics_validation_test.cc
+++ b/src/resolver/atomics_validation_test.cc
@@ -26,7 +26,23 @@
struct ResolverAtomicValidationTest : public resolver::TestHelper,
public testing::Test {};
-TEST_F(ResolverAtomicValidationTest, GlobalOfInvalidType) {
+TEST_F(ResolverAtomicValidationTest, StorageClass_WorkGroup) {
+ Global("a", ty.atomic(Source{{12, 34}}, ty.i32()),
+ ast::StorageClass::kWorkgroup);
+
+ EXPECT_TRUE(r()->Resolve());
+}
+
+TEST_F(ResolverAtomicValidationTest, StorageClass_Storage) {
+ auto* s = Structure("s", {Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))},
+ {StructBlock()});
+ Global("g", ty.Of(s), ast::StorageClass::kStorage, ast::Access::kReadWrite,
+ GroupAndBinding(0, 0));
+
+ EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidType) {
Global("a", ty.atomic(ty.f32(Source{{12, 34}})),
ast::StorageClass::kWorkgroup);
@@ -34,28 +50,274 @@
EXPECT_EQ(r()->error(), "12:34 error: atomic only supports i32 or u32 types");
}
-// TODO(crbug.com/tint/909): add validation and enable this test
-TEST_F(ResolverAtomicValidationTest, DISABLED_GlobalOfInvalidStorageClass) {
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_Simple) {
Global("a", ty.atomic(Source{{12, 34}}, ty.i32()),
ast::StorageClass::kPrivate);
EXPECT_FALSE(r()->Resolve());
- EXPECT_EQ(r()->error(), "12:34 error: atomic var requires workgroup storage");
+ EXPECT_EQ(r()->error(),
+ "12:34 error: atomic variables must have <storage> or <workgroup> "
+ "storage class");
}
-TEST_F(ResolverAtomicValidationTest, GlobalPrivateStruct) {
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_Array) {
+ Global("a", ty.atomic(Source{{12, 34}}, ty.i32()),
+ ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "12:34 error: atomic variables must have <storage> or <workgroup> "
+ "storage class");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_Struct) {
auto* s =
Structure("s", {Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))});
Global("g", ty.Of(s), ast::StorageClass::kPrivate);
EXPECT_FALSE(r()->Resolve());
EXPECT_EQ(r()->error(),
- "12:34 error: atomic types can only be used in storage classes "
- "workgroup or storage, but was used by storage class private");
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "note: atomic sub-type of 's' is declared here");
}
-// TODO(crbug.com/tint/901): Validate that storage buffer access mode is
-// read_write.
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_StructOfStruct) {
+ // struct Inner { m : atomic<i32>; };
+ // struct Outer { m : array<Inner, 4>; };
+ // var<private> g : Outer;
+
+ auto* Inner =
+ Structure("Inner", {Member("m", ty.atomic(Source{{12, 34}}, ty.i32()))});
+ auto* Outer = Structure("Outer", {Member("m", ty.Of(Inner))});
+ Global("g", ty.Of(Outer), ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "note: atomic sub-type of 'Outer' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest,
+ InvalidStorageClass_StructOfStructOfArray) {
+ // struct Inner { m : array<atomic<i32>, 4>; };
+ // struct Outer { m : array<Inner, 4>; };
+ // var<private> g : Outer;
+
+ auto* Inner =
+ Structure("Inner", {Member(Source{{12, 34}}, "m", ty.atomic(ty.i32()))});
+ auto* Outer = Structure("Outer", {Member("m", ty.Of(Inner))});
+ Global("g", ty.Of(Outer), ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "12:34 note: atomic sub-type of 'Outer' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfArray) {
+ // type AtomicArray = array<atomic<i32>, 5>;
+ // var<private> v: array<s, 5>;
+
+ auto* atomic_array = Alias(Source{{12, 34}}, "AtomicArray",
+ ty.atomic(Source{{12, 34}}, ty.i32()));
+ Global(Source{{56, 78}}, "v", ty.Of(atomic_array),
+ ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfStruct) {
+ // struct S{
+ // m: atomic<u32>;
+ // };
+ // var<private> v: array<S, 5>;
+
+ auto* s = Structure("S", {Member("m", ty.atomic<u32>())});
+ Global(Source{{56, 78}}, "v", ty.array(ty.Of(s), 5),
+ ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "note: atomic sub-type of 'array<S, 5>' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfStructOfArray) {
+ // type AtomicArray = array<atomic<i32>, 5>;
+ // struct S{
+ // m: AtomicArray;
+ // };
+ // var<private> v: array<S, 5>;
+
+ auto* atomic_array = Alias(Source{{12, 34}}, "AtomicArray",
+ ty.atomic(Source{{12, 34}}, ty.i32()));
+ auto* s = Structure("S", {Member("m", ty.Of(atomic_array))});
+ Global(Source{{56, 78}}, "v", ty.array(ty.Of(s), 5),
+ ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "note: atomic sub-type of 'array<S, 5>' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_Complex) {
+ // type AtomicArray = array<atomic<i32>, 5>;
+ // struct S6 { x: array<i32, 4>; };
+ // struct S5 { x: S6;
+ // y: AtomicArray;
+ // z: array<atomic<u32>, 8>; };
+ // struct S4 { x: S6;
+ // y: S5;
+ // z: array<atomic<i32>, 4>; };
+ // struct S3 { x: S4; };
+ // struct S2 { x: S3; };
+ // struct S1 { x: S2; };
+ // struct S0 { x: S1; };
+ // var<private> g : S0;
+
+ auto* atomic_array = Alias(Source{{12, 34}}, "AtomicArray",
+ ty.atomic(Source{{12, 34}}, ty.i32()));
+ auto* array_i32_4 = ty.array(ty.i32(), 4);
+ auto* array_atomic_u32_8 = ty.array(ty.atomic(ty.u32()), 8);
+ auto* array_atomic_i32_4 = ty.array(ty.atomic(ty.i32()), 4);
+
+ auto* s6 = Structure("S6", {Member("x", array_i32_4)});
+ auto* s5 = Structure("S5", {Member("x", ty.Of(s6)), //
+ Member("y", ty.Of(atomic_array)), //
+ Member("z", array_atomic_u32_8)}); //
+ auto* s4 = Structure("S4", {Member("x", ty.Of(s6)), //
+ Member("y", ty.Of(s5)), //
+ Member("z", array_atomic_i32_4)}); //
+ auto* s3 = Structure("S3", {Member("x", ty.Of(s4))});
+ auto* s2 = Structure("S2", {Member("x", ty.Of(s3))});
+ auto* s1 = Structure("S1", {Member("x", ty.Of(s2))});
+ auto* s0 = Structure("S0", {Member("x", ty.Of(s1))});
+ Global(Source{{56, 78}}, "g", ty.Of(s0), ast::StorageClass::kPrivate);
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables must have <storage> or <workgroup> "
+ "storage class\n"
+ "note: atomic sub-type of 'S0' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, Struct_AccessMode_Read) {
+ auto* s = Structure("s", {Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))},
+ {StructBlock()});
+ Global(Source{{56, 78}}, "g", ty.Of(s), ast::StorageClass::kStorage,
+ ast::Access::kRead, GroupAndBinding(0, 0));
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "error: atomic variables in <storage> storage class must have read_write "
+ "access mode\n"
+ "note: atomic sub-type of 's' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_Struct) {
+ auto* s = Structure("s", {Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))},
+ {StructBlock()});
+ Global(Source{{56, 78}}, "g", ty.Of(s), ast::StorageClass::kStorage,
+ ast::Access::kRead, GroupAndBinding(0, 0));
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "error: atomic variables in <storage> storage class must have read_write "
+ "access mode\n"
+ "note: atomic sub-type of 's' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStruct) {
+ // struct Inner { m : atomic<i32>; };
+ // struct Outer { m : array<Inner, 4>; };
+ // var<storage, read> g : Outer;
+
+ auto* Inner =
+ Structure("Inner", {Member("m", ty.atomic(Source{{12, 34}}, ty.i32()))});
+ auto* Outer =
+ Structure("Outer", {Member("m", ty.Of(Inner))}, {StructBlock()});
+ Global(Source{{56, 78}}, "g", ty.Of(Outer), ast::StorageClass::kStorage,
+ ast::Access::kRead, GroupAndBinding(0, 0));
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(
+ r()->error(),
+ "error: atomic variables in <storage> storage class must have read_write "
+ "access mode\n"
+ "note: atomic sub-type of 'Outer' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStructOfArray) {
+ // struct Inner { m : array<atomic<i32>, 4>; };
+ // struct Outer { m : array<Inner, 4>; };
+ // var<storage, read> g : Outer;
+
+ auto* Inner =
+ Structure("Inner", {Member(Source{{12, 34}}, "m", ty.atomic(ty.i32()))});
+ auto* Outer =
+ Structure("Outer", {Member("m", ty.Of(Inner))}, {StructBlock()});
+ Global(Source{{56, 78}}, "g", ty.Of(Outer), ast::StorageClass::kStorage,
+ ast::Access::kRead, GroupAndBinding(0, 0));
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables in <storage> storage class must have "
+ "read_write access mode\n"
+ "12:34 note: atomic sub-type of 'Outer' is declared here");
+}
+
+TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_Complex) {
+ // type AtomicArray = array<atomic<i32>, 5>;
+ // struct S6 { x: array<i32, 4>; };
+ // struct S5 { x: S6;
+ // y: AtomicArray;
+ // z: array<atomic<u32>, 8>; };
+ // struct S4 { x: S6;
+ // y: S5;
+ // z: array<atomic<i32>, 4>; };
+ // struct S3 { x: S4; };
+ // struct S2 { x: S3; };
+ // struct S1 { x: S2; };
+ // struct S0 { x: S1; };
+ // var<storage, read> g : S0;
+
+ auto* atomic_array = Alias(Source{{12, 34}}, "AtomicArray",
+ ty.atomic(Source{{12, 34}}, ty.i32()));
+ auto* array_i32_4 = ty.array(ty.i32(), 4);
+ auto* array_atomic_u32_8 = ty.array(ty.atomic(ty.u32()), 8);
+ auto* array_atomic_i32_4 = ty.array(ty.atomic(ty.i32()), 4);
+
+ auto* s6 = Structure("S6", {Member("x", array_i32_4)});
+ auto* s5 = Structure("S5", {Member("x", ty.Of(s6)), //
+ Member("y", ty.Of(atomic_array)), //
+ Member("z", array_atomic_u32_8)}); //
+ auto* s4 = Structure("S4", {Member("x", ty.Of(s6)), //
+ Member("y", ty.Of(s5)), //
+ Member("z", array_atomic_i32_4)}); //
+ auto* s3 = Structure("S3", {Member("x", ty.Of(s4))});
+ auto* s2 = Structure("S2", {Member("x", ty.Of(s3))});
+ auto* s1 = Structure("S1", {Member("x", ty.Of(s2))});
+ auto* s0 = Structure("S0", {Member("x", ty.Of(s1))}, {StructBlock()});
+ Global(Source{{56, 78}}, "g", ty.Of(s0), ast::StorageClass::kStorage,
+ ast::Access::kRead, GroupAndBinding(0, 0));
+
+ EXPECT_FALSE(r()->Resolve());
+ EXPECT_EQ(r()->error(),
+ "error: atomic variables in <storage> storage class must have "
+ "read_write access mode\n"
+ "note: atomic sub-type of 'S0' is declared here");
+}
TEST_F(ResolverAtomicValidationTest, Local) {
WrapInFunction(Var("a", ty.atomic(Source{{12, 34}}, ty.i32())));
diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc
index a1ad4ae..e746bc1 100644
--- a/src/resolver/resolver.cc
+++ b/src/resolver/resolver.cc
@@ -265,12 +265,8 @@
if (!ValidatePipelineStages()) {
return false;
}
- if (!ValidateAtomicUses()) {
- return false;
- }
bool result = true;
-
for (auto* node : builder_->ASTNodes().Objects()) {
if (marked_.count(node) == 0) {
TINT_ICE(Resolver, diagnostics_)
@@ -421,34 +417,6 @@
return true;
}
-bool Resolver::ValidateAtomicUses() {
- // https://gpuweb.github.io/gpuweb/wgsl/#atomic-types
- // Atomic types may only be instantiated by variables in the workgroup storage
- // class or by storage buffer variables with a read_write access mode.
- for (auto sm : atomic_members_) {
- auto* structure = sm.structure;
- for (auto usage : structure->StorageClassUsage()) {
- if (usage == ast::StorageClass::kWorkgroup) {
- continue;
- }
- if (usage != ast::StorageClass::kStorage) {
- // TODO(crbug.com/tint/901): Validate that the access mode is
- // read_write.
- auto* member = structure->Members()[sm.index];
- AddError(
- "atomic types can only be used in storage classes workgroup or "
- "storage, but was used by storage class " +
- std::string(ast::str(usage)),
- member->Declaration()->type()->source());
- // TODO(crbug.com/tint/901): Add note pointing at where the usage came
- // from.
- return false;
- }
- }
- }
- return true;
-}
-
bool Resolver::ValidateStorageTexture(const ast::StorageTexture* t) {
switch (t->access()) {
case ast::Access::kUndefined:
@@ -979,8 +947,7 @@
if (info->storage_class != ast::StorageClass::kStorage &&
info->declaration->declared_access() != ast::Access::kUndefined) {
AddError(
- "variables not in <storage> storage class must not declare an access "
- "mode",
+ "only variables in <storage> storage class may declare an access mode",
info->declaration->source());
return false;
}
@@ -1060,9 +1027,62 @@
break;
}
+ if (!info->declaration->is_const()) {
+ if (!ValidateAtomicVariable(info)) {
+ return false;
+ }
+ }
+
return ValidateVariable(info);
}
+// https://gpuweb.github.io/gpuweb/wgsl/#atomic-types
+// Atomic types may only be instantiated by variables in the workgroup storage
+// class or by storage buffer variables with a read_write access mode.
+bool Resolver::ValidateAtomicVariable(const VariableInfo* info) {
+ auto sc = info->storage_class;
+ auto access = info->access;
+ auto* type = info->type->UnwrapRef();
+ auto source = info->declaration->type()->source();
+
+ if (type->Is<sem::Atomic>()) {
+ if (sc != ast::StorageClass::kWorkgroup) {
+ AddError(
+ "atomic variables must have <storage> or <workgroup> storage class",
+ info->declaration->type()->source());
+ return false;
+ }
+ } else if (type->IsAnyOf<sem::Struct, sem::Array>()) {
+ auto found = atomic_composite_info_.find(type);
+ if (found != atomic_composite_info_.end()) {
+ if (sc != ast::StorageClass::kStorage &&
+ sc != ast::StorageClass::kWorkgroup) {
+ AddError(
+ "atomic variables must have <storage> or <workgroup> storage class",
+ source);
+ AddNote("atomic sub-type of '" +
+ type->FriendlyName(builder_->Symbols()) +
+ "' is declared here",
+ found->second);
+ return false;
+ } else if (sc == ast::StorageClass::kStorage &&
+ access != ast::Access::kReadWrite) {
+ AddError(
+ "atomic variables in <storage> storage class must have read_write "
+ "access mode",
+ source);
+ AddNote("atomic sub-type of '" +
+ type->FriendlyName(builder_->Symbols()) +
+ "' is declared here",
+ found->second);
+ return false;
+ }
+ }
+ }
+
+ return true;
+}
+
bool Resolver::ValidateVariable(const VariableInfo* info) {
auto* var = info->declaration;
auto* storage_type = info->type->UnwrapRef();
@@ -3738,20 +3758,20 @@
sem::Array* Resolver::Array(const ast::Array* arr) {
auto source = arr->source();
- auto* el_ty = Type(arr->type());
- if (!el_ty) {
+ auto* elem_type = Type(arr->type());
+ if (!elem_type) {
return nullptr;
}
- if (!IsPlain(el_ty)) { // Check must come before GetDefaultAlignAndSize()
- AddError(el_ty->FriendlyName(builder_->Symbols()) +
+ if (!IsPlain(elem_type)) { // Check must come before GetDefaultAlignAndSize()
+ AddError(elem_type->FriendlyName(builder_->Symbols()) +
" cannot be used as an element type of an array",
source);
return nullptr;
}
- uint32_t el_align = el_ty->Align();
- uint32_t el_size = el_ty->Size();
+ uint32_t el_align = elem_type->Align();
+ uint32_t el_size = elem_type->Size();
if (!ValidateNoDuplicateDecorations(arr->decorations())) {
return nullptr;
@@ -3781,14 +3801,23 @@
// WebGPU requires runtime arrays have at least one element, but the AST
// records an element count of 0 for it.
auto size = std::max<uint32_t>(arr->size(), 1) * stride;
- auto* sem = builder_->create<sem::Array>(el_ty, arr->size(), el_align, size,
- stride, implicit_stride);
+ auto* out = builder_->create<sem::Array>(elem_type, arr->size(), el_align,
+ size, stride, implicit_stride);
- if (!ValidateArray(sem, source)) {
+ if (!ValidateArray(out, source)) {
return nullptr;
}
- return sem;
+ if (elem_type->Is<sem::Atomic>()) {
+ atomic_composite_info_.emplace(out, arr->type()->source());
+ } else {
+ auto found = atomic_composite_info_.find(elem_type);
+ if (found != atomic_composite_info_.end()) {
+ atomic_composite_info_.emplace(out, found->second);
+ }
+ }
+
+ return out;
}
bool Resolver::ValidateArray(const sem::Array* arr, const Source& source) {
@@ -4090,11 +4119,18 @@
builder_->create<sem::Struct>(str, str->name(), sem_members, struct_align,
struct_size, size_no_padding);
- // Keep track of atomic members for validation after all usages have been
- // determined.
for (size_t i = 0; i < sem_members.size(); i++) {
- if (sem_members[i]->Type()->Is<sem::Atomic>()) {
- atomic_members_.emplace_back(StructMember{out, i});
+ auto* mem_type = sem_members[i]->Type();
+ if (mem_type->Is<sem::Atomic>()) {
+ atomic_composite_info_.emplace(out,
+ sem_members[i]->Declaration()->source());
+ break;
+ } else {
+ auto found = atomic_composite_info_.find(mem_type);
+ if (found != atomic_composite_info_.end()) {
+ atomic_composite_info_.emplace(out, found->second);
+ break;
+ }
}
}
diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h
index 86074f2..37de090 100644
--- a/src/resolver/resolver.h
+++ b/src/resolver/resolver.h
@@ -207,13 +207,6 @@
sem::Type* const sem;
};
- // Structure holding a pointer to the sem::Struct and an index to a member of
- // that structure.
- struct StructMember {
- sem::Struct* structure;
- size_t index;
- };
-
/// Resolves the program, without creating final the semantic nodes.
/// @returns true on success, false on error
bool ResolveInternal();
@@ -277,7 +270,7 @@
uint32_t el_align,
const Source& source);
bool ValidateAtomic(const ast::Atomic* a, const sem::Atomic* s);
- bool ValidateAtomicUses();
+ bool ValidateAtomicVariable(const VariableInfo* info);
bool ValidateAssignment(const ast::AssignmentStatement* a);
bool ValidateBuiltinDecoration(const ast::BuiltinDecoration* deco,
const sem::Type* storage_type,
@@ -470,7 +463,7 @@
ScopeStack<VariableInfo*> variable_stack_;
std::unordered_map<Symbol, FunctionInfo*> symbol_to_function_;
std::vector<FunctionInfo*> entry_points_;
- std::vector<StructMember> atomic_members_;
+ std::unordered_map<const sem::Type*, const Source&> atomic_composite_info_;
std::unordered_map<const ast::Function*, FunctionInfo*> function_to_info_;
std::unordered_map<const ast::Variable*, VariableInfo*> variable_to_info_;
std::unordered_map<const ast::CallExpression*, FunctionCallInfo>
diff --git a/src/resolver/storage_class_validation_test.cc b/src/resolver/storage_class_validation_test.cc
index 89fa428..9a60ac4 100644
--- a/src/resolver/storage_class_validation_test.cc
+++ b/src/resolver/storage_class_validation_test.cc
@@ -108,7 +108,7 @@
EXPECT_EQ(
r()->error(),
- R"(56:78 error: variables not in <storage> storage class must not declare an access mode)");
+ R"(56:78 error: only variables in <storage> storage class may declare an access mode)");
}
TEST_F(ResolverStorageClassValidationTest, StorageBufferNoBlockDecoration) {
