From 4038fa7f43266d77055dc0d2ddba5a0cb2c9b7f7 Mon Sep 17 00:00:00 2001 From: Sarah Date: Thu, 5 Aug 2021 15:18:29 +0000 Subject: [PATCH] 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 Kokoro: Kokoro Commit-Queue: Sarah Mashayekhi Reviewed-by: Ben Clayton --- src/resolver/atomics_validation_test.cc | 280 +++++++++++++++++- src/resolver/resolver.cc | 132 ++++++--- src/resolver/resolver.h | 11 +- src/resolver/storage_class_validation_test.cc | 2 +- 4 files changed, 358 insertions(+), 67 deletions(-) diff --git a/src/resolver/atomics_validation_test.cc b/src/resolver/atomics_validation_test.cc index 73181dc9b3..15aa2d6a8e 100644 --- a/src/resolver/atomics_validation_test.cc +++ b/src/resolver/atomics_validation_test.cc @@ -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 or " + "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 or " + "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 or " + "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; }; + // struct Outer { m : array; }; + // var 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 or " + "storage class\n" + "note: atomic sub-type of 'Outer' is declared here"); +} + +TEST_F(ResolverAtomicValidationTest, + InvalidStorageClass_StructOfStructOfArray) { + // struct Inner { m : array, 4>; }; + // struct Outer { m : array; }; + // var 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 or " + "storage class\n" + "12:34 note: atomic sub-type of 'Outer' is declared here"); +} + +TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfArray) { + // type AtomicArray = array, 5>; + // var v: array; + + 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 or " + "storage class"); +} + +TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfStruct) { + // struct S{ + // m: atomic; + // }; + // var v: array; + + auto* s = Structure("S", {Member("m", ty.atomic())}); + 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 or " + "storage class\n" + "note: atomic sub-type of 'array' is declared here"); +} + +TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_ArrayOfStructOfArray) { + // type AtomicArray = array, 5>; + // struct S{ + // m: AtomicArray; + // }; + // var v: array; + + 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 or " + "storage class\n" + "note: atomic sub-type of 'array' is declared here"); +} + +TEST_F(ResolverAtomicValidationTest, InvalidStorageClass_Complex) { + // type AtomicArray = array, 5>; + // struct S6 { x: array; }; + // struct S5 { x: S6; + // y: AtomicArray; + // z: array, 8>; }; + // struct S4 { x: S6; + // y: S5; + // z: array, 4>; }; + // struct S3 { x: S4; }; + // struct S2 { x: S3; }; + // struct S1 { x: S2; }; + // struct S0 { x: S1; }; + // var 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 or " + "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 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 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; }; + // struct Outer { m : array; }; + // var 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 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, 4>; }; + // struct Outer { m : array; }; + // var 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 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, 5>; + // struct S6 { x: array; }; + // struct S5 { x: S6; + // y: AtomicArray; + // z: array, 8>; }; + // struct S4 { x: S6; + // y: S5; + // z: array, 4>; }; + // struct S3 { x: S4; }; + // struct S2 { x: S3; }; + // struct S1 { x: S2; }; + // struct S0 { x: S1; }; + // var 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 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 a1ad4ae8bf..e746bc11f1 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -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 class must not declare an access " - "mode", + "only variables in 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()) { + if (sc != ast::StorageClass::kWorkgroup) { + AddError( + "atomic variables must have or storage class", + info->declaration->type()->source()); + return false; + } + } else if (type->IsAnyOf()) { + 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 or 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 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(arr->size(), 1) * stride; - auto* sem = builder_->create(el_ty, arr->size(), el_align, size, - stride, implicit_stride); + auto* out = builder_->create(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()) { + 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(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()) { - atomic_members_.emplace_back(StructMember{out, i}); + auto* mem_type = sem_members[i]->Type(); + if (mem_type->Is()) { + 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 86074f2101..37de090900 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -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 variable_stack_; std::unordered_map symbol_to_function_; std::vector entry_points_; - std::vector atomic_members_; + std::unordered_map atomic_composite_info_; std::unordered_map function_to_info_; std::unordered_map variable_to_info_; std::unordered_map diff --git a/src/resolver/storage_class_validation_test.cc b/src/resolver/storage_class_validation_test.cc index 89fa428cb8..9a60ac4be9 100644 --- a/src/resolver/storage_class_validation_test.cc +++ b/src/resolver/storage_class_validation_test.cc @@ -108,7 +108,7 @@ TEST_F(ResolverStorageClassValidationTest, NotStorage_AccessMode) { EXPECT_EQ( r()->error(), - R"(56:78 error: variables not in storage class must not declare an access mode)"); + R"(56:78 error: only variables in storage class may declare an access mode)"); } TEST_F(ResolverStorageClassValidationTest, StorageBufferNoBlockDecoration) {