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>
This commit is contained in:
Sarah 2021-08-05 15:18:29 +00:00 committed by Tint LUCI CQ
parent b5025dbc82
commit 4038fa7f43
4 changed files with 358 additions and 67 deletions

View File

@ -26,7 +26,23 @@ namespace {
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 @@ TEST_F(ResolverAtomicValidationTest, GlobalOfInvalidType) {
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())));

View File

@ -265,12 +265,8 @@ bool Resolver::ResolveInternal() {
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 @@ bool Resolver::ValidateAtomic(const ast::Atomic* a, const sem::Atomic* s) {
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 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
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 @@ bool Resolver::ValidateGlobalVariable(const VariableInfo* info) {
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 @@ void Resolver::CreateSemanticNodes() const {
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 @@ sem::Array* Resolver::Array(const ast::Array* arr) {
// 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 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) {
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;
}
}
}

View File

@ -207,13 +207,6 @@ class Resolver {
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 @@ class Resolver {
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 @@ class Resolver {
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>

View File

@ -108,7 +108,7 @@ TEST_F(ResolverStorageClassValidationTest, NotStorage_AccessMode) {
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) {