From 34afd9b6c6016df4287dc5fe5277000ce56c16f2 Mon Sep 17 00:00:00 2001 From: Ben Clayton Date: Tue, 29 Nov 2022 22:34:35 +0000 Subject: [PATCH] tint/resolver: Clean up variable validation Move the validation of usage and address space to helper. Improve diagnostics. Fix / clean up tests. This is in preparation for tint:1553 Change-Id: I2cbc8b851ecf02f214341f8cba6bd52413c42911 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/111880 Commit-Queue: Ben Clayton Auto-Submit: Ben Clayton Reviewed-by: David Neto Kokoro: Kokoro --- .../address_space_layout_validation_test.cc | 22 +- .../resolver/address_space_validation_test.cc | 3 +- src/tint/resolver/atomics_validation_test.cc | 134 ++++----- src/tint/resolver/resolver.cc | 16 +- src/tint/resolver/resolver.h | 3 +- src/tint/resolver/validator.cc | 264 +++++++++--------- src/tint/resolver/validator.h | 93 +++--- 7 files changed, 267 insertions(+), 268 deletions(-) diff --git a/src/tint/resolver/address_space_layout_validation_test.cc b/src/tint/resolver/address_space_layout_validation_test.cc index b34b88928e..638df2130f 100644 --- a/src/tint/resolver/address_space_layout_validation_test.cc +++ b/src/tint/resolver/address_space_layout_validation_test.cc @@ -52,7 +52,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, StorageBuffer_UnalignedMember) /* offset(5) align(1) size( 4) */ b : f32; /* offset(9) align(1) size( 3) */ // -- implicit struct size padding --; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'S' used in address space 'storage' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, StorageBuffer_UnalignedMember_SuggestedFix) { @@ -116,7 +116,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_UnalignedMember_S /* align(4) size(4) */ struct Inner { /* offset(0) align(4) size(4) */ scalar : i32; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, @@ -182,7 +182,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_UnalignedMember_A /* offset( 0) align(4) size( 4) */ scalar : f32; /* offset( 4) align(4) size(160) */ inner : @stride(16) array; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_UnalignedMember_Array_SuggestedFix) { @@ -253,7 +253,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_MembersOffsetNotM /* align(1) size(5) */ struct Inner { /* offset(0) align(1) size(5) */ scalar : i32; /* */ }; -22:24 note: see declaration of variable)"); +22:24 note: 'Outer' used in address space 'uniform' here)"); } // See https://crbug.com/tint/1344 @@ -308,7 +308,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, /* offset(12) align(1) size( 5) */ scalar : i32; /* offset(17) align(1) size( 3) */ // -- implicit struct size padding --; /* */ }; -22:24 note: see declaration of variable)"); +22:24 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, @@ -418,7 +418,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStrid /* offset( 0) align(4) size(40) */ inner : array; /* offset(40) align(4) size( 4) */ scalar : i32; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStride_Vector) { @@ -453,7 +453,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStrid /* offset(80) align(4) size( 4) */ scalar : i32; /* offset(84) align(1) size( 4) */ // -- implicit struct size padding --; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStride_Struct) { @@ -495,7 +495,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStrid /* offset( 0) align(4) size(80) */ inner : array; /* offset(80) align(4) size( 4) */ scalar : i32; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStride_TopLevelArray) { @@ -507,7 +507,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStrid ASSERT_FALSE(r()->Resolve()); EXPECT_EQ( r()->error(), - R"(34:56 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.)"); + R"(78:90 error: uniform storage requires that array elements be aligned to 16 bytes, but array element alignment is currently 4. Consider using a vector or struct as the element type instead.)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStride_NestedArray) { @@ -534,7 +534,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStrid /* align(4) size(64) */ struct Outer { /* offset( 0) align(4) size(64) */ inner : array, 4>; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'Outer' used in address space 'uniform' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, UniformBuffer_InvalidArrayStride_SuggestedFix) { @@ -587,7 +587,7 @@ TEST_F(ResolverAddressSpaceLayoutValidationTest, PushConstant_UnalignedMember) { /* offset(5) align(1) size( 4) */ b : f32; /* offset(9) align(1) size( 3) */ // -- implicit struct size padding --; /* */ }; -78:90 note: see declaration of variable)"); +78:90 note: 'S' used in address space 'push_constant' here)"); } TEST_F(ResolverAddressSpaceLayoutValidationTest, PushConstant_Aligned) { diff --git a/src/tint/resolver/address_space_validation_test.cc b/src/tint/resolver/address_space_validation_test.cc index 60e54df5d8..90467b474f 100644 --- a/src/tint/resolver/address_space_validation_test.cc +++ b/src/tint/resolver/address_space_validation_test.cc @@ -472,8 +472,7 @@ TEST_F(ResolverAddressSpaceValidationTest, PushConstantF16) { ASSERT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "56:78 error: using f16 types in 'push_constant' address space is not " - "implemented yet"); + "error: using f16 types in 'push_constant' address space is not implemented yet"); } TEST_F(ResolverAddressSpaceValidationTest, PushConstantPointer) { diff --git a/src/tint/resolver/atomics_validation_test.cc b/src/tint/resolver/atomics_validation_test.cc index 08bdf13472..8b799416fd 100644 --- a/src/tint/resolver/atomics_validation_test.cc +++ b/src/tint/resolver/atomics_validation_test.cc @@ -40,7 +40,7 @@ TEST_F(ResolverAtomicValidationTest, AddressSpace_Storage) { } TEST_F(ResolverAtomicValidationTest, AddressSpace_Storage_Struct) { - auto* s = Structure("s", utils::Vector{Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))}); + auto* s = Structure("s", utils::Vector{Member(Source{{12, 34}}, "a", ty.atomic(ty.i32()))}); GlobalVar("g", ty.Of(s), ast::AddressSpace::kStorage, ast::Access::kReadWrite, Group(0_a), Binding(0_a)); @@ -55,31 +55,28 @@ TEST_F(ResolverAtomicValidationTest, InvalidType) { } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Simple) { - GlobalVar("a", ty.atomic(Source{{12, 34}}, ty.i32()), ast::AddressSpace::kPrivate); + GlobalVar(Source{{12, 34}}, "a", ty.atomic(ty.i32()), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: atomic variables must have or " - "address space"); + "12:34 error: atomic variables must have or address space"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Array) { - GlobalVar("a", ty.atomic(Source{{12, 34}}, ty.i32()), ast::AddressSpace::kPrivate); + GlobalVar(Source{{12, 34}}, "a", ty.atomic(ty.i32()), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "12:34 error: atomic variables must have or " - "address space"); + "12:34 error: atomic variables must have or address space"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Struct) { - auto* s = Structure("s", utils::Vector{Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))}); - GlobalVar("g", ty.Of(s), ast::AddressSpace::kPrivate); + auto* s = Structure("s", utils::Vector{Member("a", ty.atomic(ty.i32()))}); + GlobalVar(Source{{56, 78}}, "g", ty.Of(s), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" + "56:78 error: atomic variables must have or address space\n" "note: atomic sub-type of 's' is declared here"); } @@ -91,12 +88,11 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_StructOfStruct) { auto* Inner = Structure("Inner", utils::Vector{Member("m", ty.atomic(Source{{12, 34}}, ty.i32()))}); auto* Outer = Structure("Outer", utils::Vector{Member("m", ty.Of(Inner))}); - GlobalVar("g", ty.Of(Outer), ast::AddressSpace::kPrivate); + GlobalVar(Source{{56, 78}}, "g", ty.Of(Outer), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" + "56:78 error: atomic variables must have or address space\n" "note: atomic sub-type of 'Outer' is declared here"); } @@ -108,13 +104,12 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_StructOfStructOfArray) auto* Inner = Structure("Inner", utils::Vector{Member(Source{{12, 34}}, "m", ty.atomic(ty.i32()))}); auto* Outer = Structure("Outer", utils::Vector{Member("m", ty.Of(Inner))}); - GlobalVar("g", ty.Of(Outer), ast::AddressSpace::kPrivate); + GlobalVar(Source{{56, 78}}, "g", ty.Of(Outer), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" - "12:34 note: atomic sub-type of 'Outer' is declared here"); + R"(56:78 error: atomic variables must have or address space +12:34 note: atomic sub-type of 'Outer' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfArray) { @@ -127,8 +122,7 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfArray) { EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space"); + "56:78 error: atomic variables must have or address space"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfStruct) { @@ -137,14 +131,13 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfStruct) { // }; // var v: array; - auto* s = Structure("S", utils::Vector{Member("m", ty.atomic())}); + auto* s = Structure("S", utils::Vector{Member(Source{{12, 34}}, "m", ty.atomic())}); GlobalVar(Source{{56, 78}}, "v", ty.array(ty.Of(s), 5_u), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" - "note: atomic sub-type of 'array' is declared here"); + R"(56:78 error: atomic variables must have or address space +12:34 note: atomic sub-type of 'array' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfStructOfArray) { @@ -154,16 +147,14 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_ArrayOfStructOfArray) { // }; // var v: array; - auto* atomic_array = - Alias(Source{{12, 34}}, "AtomicArray", ty.atomic(Source{{12, 34}}, ty.i32())); - auto* s = Structure("S", utils::Vector{Member("m", ty.Of(atomic_array))}); + auto* atomic_array = Alias("AtomicArray", ty.atomic(ty.i32())); + auto* s = Structure("S", utils::Vector{Member(Source{{12, 34}}, "m", ty.Of(atomic_array))}); GlobalVar(Source{{56, 78}}, "v", ty.array(ty.Of(s), 5_u), ast::AddressSpace::kPrivate); EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" - "note: atomic sub-type of 'array' is declared here"); + R"(56:78 error: atomic variables must have or address space +12:34 note: atomic sub-type of 'array' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Complex) { @@ -181,19 +172,18 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Complex) { // struct S0 { x: S1; }; // var g : S0; - auto* atomic_array = - Alias(Source{{12, 34}}, "AtomicArray", ty.atomic(Source{{12, 34}}, ty.i32())); + auto* atomic_array = Alias("AtomicArray", ty.atomic(ty.i32())); auto* array_i32_4 = ty.array(ty.i32(), 4_u); auto* array_atomic_u32_8 = ty.array(ty.atomic(ty.u32()), 8_u); auto* array_atomic_i32_4 = ty.array(ty.atomic(ty.i32()), 4_u); auto* s6 = Structure("S6", utils::Vector{Member("x", array_i32_4)}); - auto* s5 = Structure("S5", utils::Vector{Member("x", ty.Of(s6)), // - Member("y", ty.Of(atomic_array)), // - Member("z", array_atomic_u32_8)}); // - auto* s4 = Structure("S4", utils::Vector{Member("x", ty.Of(s6)), // - Member("y", ty.Of(s5)), // - Member("z", array_atomic_i32_4)}); // + auto* s5 = Structure("S5", utils::Vector{Member("x", ty.Of(s6)), // + Member(Source{{12, 34}}, "y", ty.Of(atomic_array)), // + Member("z", array_atomic_u32_8)}); // + auto* s4 = Structure("S4", utils::Vector{Member("x", ty.Of(s6)), // + Member("y", ty.Of(s5)), // + Member("z", array_atomic_i32_4)}); // auto* s3 = Structure("S3", utils::Vector{Member("x", ty.Of(s4))}); auto* s2 = Structure("S2", utils::Vector{Member("x", ty.Of(s3))}); auto* s1 = Structure("S1", utils::Vector{Member("x", ty.Of(s2))}); @@ -202,33 +192,32 @@ TEST_F(ResolverAtomicValidationTest, InvalidAddressSpace_Complex) { EXPECT_FALSE(r()->Resolve()); EXPECT_EQ(r()->error(), - "error: atomic variables must have or " - "address space\n" - "note: atomic sub-type of 'S0' is declared here"); + R"(56:78 error: atomic variables must have or address space +12:34 note: atomic sub-type of 'S0' is declared here)"); } TEST_F(ResolverAtomicValidationTest, Struct_AccessMode_Read) { - auto* s = Structure("s", utils::Vector{Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))}); + auto* s = Structure("s", utils::Vector{Member(Source{{12, 34}}, "a", ty.atomic(ty.i32()))}); GlobalVar(Source{{56, 78}}, "g", ty.Of(s), ast::AddressSpace::kStorage, ast::Access::kRead, Group(0_a), Binding(0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: atomic variables in address space must have read_write " - "access mode\n" - "note: atomic sub-type of 's' is declared here"); + EXPECT_EQ( + r()->error(), + R"(56:78 error: atomic variables in address space must have read_write access mode +12:34 note: atomic sub-type of 's' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_Struct) { - auto* s = Structure("s", utils::Vector{Member("a", ty.atomic(Source{{12, 34}}, ty.i32()))}); + auto* s = Structure("s", utils::Vector{Member(Source{{12, 34}}, "a", ty.atomic(ty.i32()))}); GlobalVar(Source{{56, 78}}, "g", ty.Of(s), ast::AddressSpace::kStorage, ast::Access::kRead, Group(0_a), Binding(0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: atomic variables in address space must have read_write " - "access mode\n" - "note: atomic sub-type of 's' is declared here"); + EXPECT_EQ( + r()->error(), + R"(56:78 error: atomic variables in address space must have read_write access mode +12:34 note: atomic sub-type of 's' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStruct) { @@ -237,16 +226,16 @@ TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStruct) { // var g : Outer; auto* Inner = - Structure("Inner", utils::Vector{Member("m", ty.atomic(Source{{12, 34}}, ty.i32()))}); + Structure("Inner", utils::Vector{Member(Source{{12, 34}}, "m", ty.atomic(ty.i32()))}); auto* Outer = Structure("Outer", utils::Vector{Member("m", ty.Of(Inner))}); GlobalVar(Source{{56, 78}}, "g", ty.Of(Outer), ast::AddressSpace::kStorage, ast::Access::kRead, Group(0_a), Binding(0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: atomic variables in address space must have read_write " - "access mode\n" - "note: atomic sub-type of 'Outer' is declared here"); + EXPECT_EQ( + r()->error(), + R"(56:78 error: atomic variables in address space must have read_write access mode +12:34 note: atomic sub-type of 'Outer' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStructOfArray) { @@ -261,10 +250,10 @@ TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_StructOfStructOfArray) { Group(0_a), Binding(0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: atomic variables in address space must have " - "read_write access mode\n" - "12:34 note: atomic sub-type of 'Outer' is declared here"); + EXPECT_EQ( + r()->error(), + R"(56:78 error: atomic variables in address space must have read_write access mode +12:34 note: atomic sub-type of 'Outer' is declared here)"); } TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_Complex) { @@ -282,31 +271,30 @@ TEST_F(ResolverAtomicValidationTest, InvalidAccessMode_Complex) { // struct S0 { x: S1; }; // var g : S0; - auto* atomic_array = - Alias(Source{{12, 34}}, "AtomicArray", ty.atomic(Source{{12, 34}}, ty.i32())); + auto* atomic_array = Alias("AtomicArray", ty.atomic(ty.i32())); auto* array_i32_4 = ty.array(ty.i32(), 4_u); auto* array_atomic_u32_8 = ty.array(ty.atomic(ty.u32()), 8_u); auto* array_atomic_i32_4 = ty.array(ty.atomic(ty.i32()), 4_u); auto* s6 = Structure("S6", utils::Vector{Member("x", array_i32_4)}); - auto* s5 = Structure("S5", utils::Vector{Member("x", ty.Of(s6)), // - Member("y", ty.Of(atomic_array)), // - Member("z", array_atomic_u32_8)}); // - auto* s4 = Structure("S4", utils::Vector{Member("x", ty.Of(s6)), // - Member("y", ty.Of(s5)), // - Member("z", array_atomic_i32_4)}); // + auto* s5 = Structure("S5", utils::Vector{Member("x", ty.Of(s6)), // + Member(Source{{56, 78}}, "y", ty.Of(atomic_array)), // + Member("z", array_atomic_u32_8)}); // + auto* s4 = Structure("S4", utils::Vector{Member("x", ty.Of(s6)), // + Member("y", ty.Of(s5)), // + Member("z", array_atomic_i32_4)}); // auto* s3 = Structure("S3", utils::Vector{Member("x", ty.Of(s4))}); auto* s2 = Structure("S2", utils::Vector{Member("x", ty.Of(s3))}); auto* s1 = Structure("S1", utils::Vector{Member("x", ty.Of(s2))}); auto* s0 = Structure("S0", utils::Vector{Member("x", ty.Of(s1))}); - GlobalVar(Source{{56, 78}}, "g", ty.Of(s0), ast::AddressSpace::kStorage, ast::Access::kRead, + GlobalVar(Source{{12, 34}}, "g", ty.Of(s0), ast::AddressSpace::kStorage, ast::Access::kRead, Group(0_a), Binding(0_a)); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: atomic variables in address space must have " - "read_write access mode\n" - "note: atomic sub-type of 'S0' is declared here"); + EXPECT_EQ( + r()->error(), + R"(12:34 error: atomic variables in address space must have read_write access mode +56:78 note: atomic sub-type of 'S0' is declared here)"); } TEST_F(ResolverAtomicValidationTest, Local) { diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 0fdd78c3e0..ccabe39834 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -103,7 +103,11 @@ Resolver::Resolver(ProgramBuilder* builder) const_eval_(*builder), intrinsic_table_(IntrinsicTable::Create(*builder)), sem_(builder, dependencies_), - validator_(builder, sem_) {} + validator_(builder, + sem_, + enabled_extensions_, + atomic_composite_info_, + valid_type_storage_layouts_) {} Resolver::~Resolver() = default; @@ -891,13 +895,7 @@ sem::GlobalVariable* Resolver::GlobalVariable(const ast::Variable* v) { return nullptr; } - if (!validator_.GlobalVariable(sem, override_ids_, atomic_composite_info_)) { - return nullptr; - } - - // TODO(bclayton): Call this at the end of resolve on all uniform and storage - // referenced structs - if (!validator_.AddressSpaceLayout(sem, enabled_extensions_, valid_type_storage_layouts_)) { + if (!validator_.GlobalVariable(sem, override_ids_)) { return nullptr; } @@ -2300,7 +2298,7 @@ sem::Call* Resolver::BuiltinCall(const ast::CallExpression* expr, current_function_->AddDirectCall(call); } - if (!validator_.RequiredExtensionForBuiltinFunction(call, enabled_extensions_)) { + if (!validator_.RequiredExtensionForBuiltinFunction(call)) { return nullptr; } diff --git a/src/tint/resolver/resolver.h b/src/tint/resolver/resolver.h index 0b31fe1131..abc76330cc 100644 --- a/src/tint/resolver/resolver.h +++ b/src/tint/resolver/resolver.h @@ -111,8 +111,6 @@ class Resolver { const Validator* GetValidatorForTesting() const { return &validator_; } private: - Validator::ValidTypeStorageLayouts valid_type_storage_layouts_; - /// Resolves the program, without creating final the semantic nodes. /// @returns true on success, false on error bool ResolveInternal(); @@ -471,6 +469,7 @@ class Resolver { sem::CompoundStatement* current_compound_statement_ = nullptr; uint32_t current_scoping_depth_ = 0; utils::UniqueVector* resolved_overrides_ = nullptr; + utils::Hashset valid_type_storage_layouts_; }; } // namespace tint::resolver diff --git a/src/tint/resolver/validator.cc b/src/tint/resolver/validator.cc index 70d9f8e997..6a64d5a539 100644 --- a/src/tint/resolver/validator.cc +++ b/src/tint/resolver/validator.cc @@ -153,8 +153,18 @@ void TraverseCallChain(diag::List& diagnostics, } // namespace -Validator::Validator(ProgramBuilder* builder, SemHelper& sem) - : symbols_(builder->Symbols()), diagnostics_(builder->Diagnostics()), sem_(sem) {} +Validator::Validator( + ProgramBuilder* builder, + SemHelper& sem, + const ast::Extensions& enabled_extensions, + const utils::Hashmap& atomic_composite_info, + utils::Hashset& valid_type_storage_layouts) + : symbols_(builder->Symbols()), + diagnostics_(builder->Diagnostics()), + sem_(sem), + enabled_extensions_(enabled_extensions), + atomic_composite_info_(atomic_composite_info), + valid_type_storage_layouts_(valid_type_storage_layouts) {} Validator::~Validator() = default; @@ -360,8 +370,7 @@ bool Validator::VariableInitializer(const ast::Variable* v, bool Validator::AddressSpaceLayout(const sem::Type* store_ty, ast::AddressSpace address_space, - Source source, - ValidTypeStorageLayouts& layouts) const { + Source source) const { // https://gpuweb.github.io/gpuweb/wgsl/#storage-class-layout-constraints auto is_uniform_struct_or_array = [address_space](const sem::Type* ty) { @@ -386,8 +395,8 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, return symbols_.NameFor(sm->Declaration()->symbol); }; - // Cache result of type + address space pair. - if (!layouts.emplace(store_ty, address_space).second) { + // Only validate the [type + address space] once + if (!valid_type_storage_layouts_.Add(TypeAndAddressSpace{store_ty, address_space})) { return true; } @@ -395,6 +404,12 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, return true; } + auto note_usage = [&] { + AddNote("'" + store_ty->FriendlyName(symbols_) + "' used in address space '" + + utils::ToString(address_space) + "' here", + source); + }; + // Among three host-shareable address spaces, f16 is supported in "uniform" and // "storage" address space, but not "push_constant" address space yet. if (Is(sem::Type::DeepestElementOf(store_ty)) && @@ -409,10 +424,10 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, uint32_t required_align = required_alignment_of(m->Type()); // Recurse into the member type. - if (!AddressSpaceLayout(m->Type(), address_space, m->Declaration()->type->source, - layouts)) { + if (!AddressSpaceLayout(m->Type(), address_space, m->Declaration()->type->source)) { AddNote("see layout of struct:\n" + str->Layout(symbols_), str->Declaration()->source); + note_usage(); return false; } @@ -435,6 +450,7 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, member_str->Declaration()->source); } + note_usage(); return false; } @@ -460,6 +476,7 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, AddNote("and layout of previous member struct:\n" + prev_member_str->Layout(symbols_), prev_member_str->Declaration()->source); + note_usage(); return false; } } @@ -472,7 +489,7 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, // TODO(crbug.com/tint/1388): Ideally we'd pass the source for nested element type here, but // we can't easily get that from the semantic node. We should consider recursing through the // AST type nodes instead. - if (!AddressSpaceLayout(arr->ElemType(), address_space, source, layouts)) { + if (!AddressSpaceLayout(arr->ElemType(), address_space, source)) { return false; } @@ -484,21 +501,16 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, // shader author can resolve the issue. std::string hint; if (arr->ElemType()->is_scalar()) { - hint = - "Consider using a vector or struct as the element type " - "instead."; + hint = "Consider using a vector or struct as the element type instead."; } else if (auto* vec = arr->ElemType()->As(); vec && vec->type()->Size() == 4) { hint = "Consider using a vec4 instead."; } else if (arr->ElemType()->Is()) { - hint = - "Consider using the @size attribute on the last struct " - "member."; + hint = "Consider using the @size attribute on the last struct member."; } else { hint = - "Consider wrapping the element type in a struct and using " - "the " - "@size attribute."; + "Consider wrapping the element type in a struct and using the @size " + "attribute."; } AddError( "uniform storage requires that array elements be aligned to 16 " @@ -513,42 +525,6 @@ bool Validator::AddressSpaceLayout(const sem::Type* store_ty, return true; } -bool Validator::AddressSpaceLayout(const sem::Variable* var, - const ast::Extensions& enabled_extensions, - ValidTypeStorageLayouts& layouts) const { - if (var->AddressSpace() == ast::AddressSpace::kPushConstant && - !enabled_extensions.Contains(ast::Extension::kChromiumExperimentalPushConstant) && - IsValidationEnabled(var->Declaration()->attributes, - ast::DisabledValidation::kIgnoreAddressSpace)) { - AddError( - "use of variable address space 'push_constant' requires enabling extension " - "'chromium_experimental_push_constant'", - var->Declaration()->source); - return false; - } - - if (auto* str = var->Type()->UnwrapRef()->As()) { - // Check the structure has a declaration. Builtins like modf() and frexp() return untypeable - // structures, and so they have no declaration. Just skip validation for these. - if (auto* str_decl = str->Declaration()) { - if (!AddressSpaceLayout(str, var->AddressSpace(), str_decl->source, layouts)) { - AddNote("see declaration of variable", var->Declaration()->source); - return false; - } - } - } else { - Source source = var->Declaration()->source; - if (var->Declaration()->type) { - source = var->Declaration()->type->source; - } - if (!AddressSpaceLayout(var->Type()->UnwrapRef(), var->AddressSpace(), source, layouts)) { - return false; - } - } - - return true; -} - bool Validator::LocalVariable(const sem::Variable* local) const { auto* decl = local->Declaration(); if (IsArrayWithOverrideCount(local->Type())) { @@ -581,8 +557,7 @@ bool Validator::LocalVariable(const sem::Variable* local) const { bool Validator::GlobalVariable( const sem::GlobalVariable* global, - const utils::Hashmap& override_ids, - const utils::Hashmap& atomic_composite_info) const { + const utils::Hashmap& override_ids) const { auto* decl = global->Declaration(); if (global->AddressSpace() != ast::AddressSpace::kWorkgroup && IsArrayWithOverrideCount(global->Type())) { @@ -592,7 +567,7 @@ bool Validator::GlobalVariable( } bool ok = Switch( decl, // - [&](const ast::Var* var) { + [&](const ast::Var*) { if (auto* init = global->Initializer(); init && init->Stage() > sem::EvaluationStage::kOverride) { AddError("module-scope 'var' initializer must be a constant or override-expression", @@ -620,29 +595,6 @@ bool Validator::GlobalVariable( } } - // https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration - // The access mode always has a default, and except for variables in the storage address - // space, must not be written. - if (var->declared_access != ast::Access::kUndefined) { - if (global->AddressSpace() == ast::AddressSpace::kStorage) { - // The access mode for the storage address space can only be 'read' or - // 'read_write'. - if (var->declared_access == ast::Access::kWrite) { - AddError("access mode 'write' is not valid for the 'storage' address space", - decl->source); - return false; - } - } else { - AddError("only variables in address space may declare an access mode", - decl->source); - return false; - } - } - - if (!AtomicVariable(global, atomic_composite_info)) { - return false; - } - return Var(global); }, [&](const ast::Override*) { return Override(global, override_ids); }, @@ -698,65 +650,40 @@ bool Validator::GlobalVariable( return true; } -// 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 Validator::AtomicVariable( - const sem::Variable* var, - const utils::Hashmap& atomic_composite_info) const { - auto address_space = var->AddressSpace(); - auto* decl = var->Declaration(); - auto access = var->Access(); - auto* type = var->Type()->UnwrapRef(); - auto source = decl->type ? decl->type->source : decl->source; - - if (type->Is()) { - if (address_space != ast::AddressSpace::kWorkgroup && - address_space != ast::AddressSpace::kStorage) { - AddError("atomic variables must have or address space", source); - return false; - } - } else if (type->IsAnyOf()) { - if (auto found = atomic_composite_info.Find(type)) { - if (address_space != ast::AddressSpace::kStorage && - address_space != ast::AddressSpace::kWorkgroup) { - AddError("atomic variables must have or address space", - source); - AddNote("atomic sub-type of '" + sem_.TypeNameOf(type) + "' is declared here", - **found); - return false; - } else if (address_space == ast::AddressSpace::kStorage && - access != ast::Access::kReadWrite) { - AddError( - "atomic variables in address space must have read_write " - "access mode", - source); - AddNote("atomic sub-type of '" + sem_.TypeNameOf(type) + "' is declared here", - **found); - return false; - } - } - } - - return true; -} - bool Validator::Var(const sem::Variable* v) const { auto* var = v->Declaration()->As(); - auto* storage_ty = v->Type()->UnwrapRef(); + auto* store_ty = v->Type()->UnwrapRef(); - if (!IsStorable(storage_ty)) { - AddError(sem_.TypeNameOf(storage_ty) + " cannot be used as the type of a var", var->source); + if (!IsStorable(store_ty)) { + AddError(sem_.TypeNameOf(store_ty) + " cannot be used as the type of a var", var->source); return false; } - if (storage_ty->is_handle() && var->declared_address_space != ast::AddressSpace::kNone) { - // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables - // If the store type is a texture type or a sampler type, then the variable declaration must - // not have a address space attribute. The address space will always be handle. - AddError( - "variables of type '" + sem_.TypeNameOf(storage_ty) + "' must not have a address space", - var->source); + if (store_ty->is_handle()) { + if (var->declared_address_space != ast::AddressSpace::kNone) { + // https://gpuweb.github.io/gpuweb/wgsl/#module-scope-variables + // If the store type is a texture type or a sampler type, then the variable declaration + // must not have a address space attribute. The address space will always be handle. + AddError("variables of type '" + sem_.TypeNameOf(store_ty) + + "' must not have a address space", + var->source); + return false; + } + } + + if (var->declared_access != ast::Access::kUndefined) { + // https://gpuweb.github.io/gpuweb/wgsl/#variable-declaration + // The access mode always has a default, and except for variables in the storage address + // space, must not be written. + if (var->declared_address_space != ast::AddressSpace::kStorage) { + AddError("only variables in address space may declare an access mode", + var->source); + return false; + } + } + + if (!CheckTypeAccessAddressSpace(v->Type()->UnwrapRef(), v->Access(), v->AddressSpace(), + var->attributes, var->source)) { return false; } @@ -1642,9 +1569,7 @@ bool Validator::TextureBuiltinFunction(const sem::Call* call) const { check_arg_is_constexpr(sem::ParameterUsage::kComponent, 0, 3); } -bool Validator::RequiredExtensionForBuiltinFunction( - const sem::Call* call, - const ast::Extensions& enabled_extensions) const { +bool Validator::RequiredExtensionForBuiltinFunction(const sem::Call* call) const { const auto* builtin = call->Target()->As(); if (!builtin) { return true; @@ -1655,7 +1580,7 @@ bool Validator::RequiredExtensionForBuiltinFunction( return true; } - if (!enabled_extensions.Contains(extension)) { + if (!enabled_extensions_.Contains(extension)) { AddError("cannot call built-in function '" + std::string(builtin->str()) + "' without extension " + utils::ToString(extension), call->Declaration()->source); @@ -2451,4 +2376,69 @@ std::string Validator::VectorPretty(uint32_t size, const sem::Type* element_type return vec_type.FriendlyName(symbols_); } +bool Validator::CheckTypeAccessAddressSpace( + const sem::Type* store_ty, + ast::Access access, + ast::AddressSpace address_space, + const utils::VectorRef attributes, + const Source& source) const { + if (!AddressSpaceLayout(store_ty, address_space, source)) { + return false; + } + + if (address_space == ast::AddressSpace::kPushConstant && + !enabled_extensions_.Contains(ast::Extension::kChromiumExperimentalPushConstant) && + IsValidationEnabled(attributes, ast::DisabledValidation::kIgnoreAddressSpace)) { + AddError( + "use of variable address space 'push_constant' requires enabling extension " + "'chromium_experimental_push_constant'", + source); + return false; + } + + if (address_space == ast::AddressSpace::kStorage && access == ast::Access::kWrite) { + // The access mode for the storage address space can only be 'read' or + // 'read_write'. + AddError("access mode 'write' is not valid for the 'storage' address space", source); + return false; + } + + auto atomic_error = [&]() -> const char* { + if (address_space != ast::AddressSpace::kStorage && + address_space != ast::AddressSpace::kWorkgroup) { + return "atomic variables must have or address space"; + } + if (address_space == ast::AddressSpace::kStorage && access != ast::Access::kReadWrite) { + return "atomic variables in address space must have read_write access " + "mode"; + } + return nullptr; + }; + + auto check_sub_atomics = [&] { + if (auto atomic_use = atomic_composite_info_.Get(store_ty)) { + if (auto* err = atomic_error()) { + AddError(err, source); + AddNote("atomic sub-type of '" + sem_.TypeNameOf(store_ty) + "' is declared here", + **atomic_use); + return false; + } + } + return true; + }; + + return Switch( + store_ty, // + [&](const sem::Atomic*) { + if (auto* err = atomic_error()) { + AddError(err, source); + return false; + } + return true; + }, + [&](const sem::Struct*) { return check_sub_atomics(); }, // + [&](const sem::Array*) { return check_sub_atomics(); }, // + [&](Default) { return true; }); +} + } // namespace tint::resolver diff --git a/src/tint/resolver/validator.h b/src/tint/resolver/validator.h index 500d056e79..5c0188f3de 100644 --- a/src/tint/resolver/validator.h +++ b/src/tint/resolver/validator.h @@ -24,6 +24,7 @@ #include "src/tint/resolver/sem_helper.h" #include "src/tint/sem/evaluation_stage.h" #include "src/tint/source.h" +#include "src/tint/utils/hash.h" #include "src/tint/utils/hashmap.h" #include "src/tint/utils/vector.h" @@ -66,19 +67,38 @@ class WhileStatement; namespace tint::resolver { +/// TypeAndAddressSpace is a pair of type and address space +struct TypeAndAddressSpace { + /// The type + const sem::Type* type; + /// The address space + ast::AddressSpace address_space; + + /// Equality operator + /// @param other the other TypeAndAddressSpace to compare this TypeAndAddressSpace to + /// @returns true if the type and address space of this TypeAndAddressSpace is equal to @p other + bool operator==(const TypeAndAddressSpace& other) const { + return type == other.type && address_space == other.address_space; + } +}; + /// Validation logic for various ast nodes. The validations in general should /// be shallow and depend on the resolver to call on children. The validations /// also assume that sem changes have already been made. The validation checks /// should not alter the AST or SEM trees. class Validator { public: - /// The valid type storage layouts typedef - using ValidTypeStorageLayouts = std::set>; - /// Constructor /// @param builder the program builder /// @param helper the SEM helper to validate with - Validator(ProgramBuilder* builder, SemHelper& helper); + /// @param enabled_extensions all the extensions declared in current module + /// @param atomic_composite_info atomic composite info of the module + /// @param valid_type_storage_layouts a set of validated type layouts by address space + Validator(ProgramBuilder* builder, + SemHelper& helper, + const ast::Extensions& enabled_extensions, + const utils::Hashmap& atomic_composite_info, + utils::Hashset& valid_type_storage_layouts); ~Validator(); /// Adds the given error message to the diagnostics @@ -143,20 +163,12 @@ class Validator { uint32_t el_size, uint32_t el_align) const; - /// Validates an atomic - /// @param a the atomic ast node to validate + /// Validates an atomic type + /// @param a the atomic ast node /// @param s the atomic sem node /// @returns true on success, false otherwise. bool Atomic(const ast::Atomic* a, const sem::Atomic* s) const; - /// Validates an atoic variable - /// @param var the variable to validate - /// @param atomic_composite_info store atomic information - /// @returns true on success, false otherwise. - bool AtomicVariable( - const sem::Variable* var, - const utils::Hashmap& atomic_composite_info) const; - /// Validates an assignment /// @param a the assignment statement /// @param rhs_ty the type of the right hand side @@ -238,12 +250,10 @@ class Validator { /// Validates a global variable /// @param var the global variable to validate /// @param override_id the set of override ids in the module - /// @param atomic_composite_info atomic composite info in the module /// @returns true on success, false otherwise bool GlobalVariable( const sem::GlobalVariable* var, - const utils::Hashmap& override_id, - const utils::Hashmap& atomic_composite_info) const; + const utils::Hashmap& override_id) const; /// Validates a break-if statement /// @param stmt the statement to validate @@ -423,10 +433,8 @@ class Validator { /// Validates an optional builtin function and its required extension. /// @param call the builtin call to validate - /// @param enabled_extensions all the extensions declared in current module /// @returns true on success, false otherwise - bool RequiredExtensionForBuiltinFunction(const sem::Call* call, - const ast::Extensions& enabled_extensions) const; + bool RequiredExtensionForBuiltinFunction(const sem::Call* call) const; /// Validates there are no duplicate attributes /// @param attributes the list of attributes to validate @@ -437,21 +445,8 @@ class Validator { /// @param type the type to validate /// @param sc the address space /// @param source the source of the type - /// @param layouts previously validated storage layouts /// @returns true on success, false otherwise - bool AddressSpaceLayout(const sem::Type* type, - ast::AddressSpace sc, - Source source, - ValidTypeStorageLayouts& layouts) const; - - /// Validates a address space layout - /// @param var the variable to validate - /// @param layouts previously validated storage layouts - /// @param enabled_extensions all the extensions declared in current module - /// @returns true on success, false otherwise. - bool AddressSpaceLayout(const sem::Variable* var, - const ast::Extensions& enabled_extensions, - ValidTypeStorageLayouts& layouts) const; + bool AddressSpaceLayout(const sem::Type* type, ast::AddressSpace sc, Source source) const; /// @returns true if the attribute list contains a /// ast::DisableValidationAttribute with the validation mode equal to @@ -497,11 +492,41 @@ class Validator { /// @return pretty string representation std::string VectorPretty(uint32_t size, const sem::Type* element_type) const; + /// Raises an error if combination of @p store_ty, @p access and @p address_space are not valid + /// for a `var` or `ptr` declaration. + /// @param store_ty the store type of the var or pointer + /// @param access the var or pointer access + /// @param address_space the var or pointer address space + /// @param source the source for the error + /// @returns true on success, false if an error was raised. + bool CheckTypeAccessAddressSpace(const sem::Type* store_ty, + ast::Access access, + ast::AddressSpace address_space, + const utils::VectorRef attributes, + const Source& source) const; SymbolTable& symbols_; diag::List& diagnostics_; SemHelper& sem_; + const ast::Extensions& enabled_extensions_; + const utils::Hashmap& atomic_composite_info_; + utils::Hashset& valid_type_storage_layouts_; }; } // namespace tint::resolver +namespace std { + +/// Custom std::hash specialization for tint::resolver::TypeAndAddressSpace. +template <> +class hash { + public: + /// @param tas the TypeAndAddressSpace + /// @return the hash value + inline std::size_t operator()(const tint::resolver::TypeAndAddressSpace& tas) const { + return tint::utils::Hash(tas.type, tas.address_space); + } +}; + +} // namespace std + #endif // SRC_TINT_RESOLVER_VALIDATOR_H_