From ac8975f2912608bfcdb87dd7a1f460d36d5cd093 Mon Sep 17 00:00:00 2001 From: James Price Date: Thu, 9 Dec 2021 15:45:03 +0000 Subject: [PATCH] validation: Remove requirement for block attribute Replace all validation rules that rely on the block attribute with the new rules based on fixed-footprint types. Bug: tint:1324 Change-Id: I02656537bee66e6e1af95875e503a37bf23d4a6b Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/72081 Kokoro: Kokoro Reviewed-by: Ben Clayton --- src/resolver/decoration_validation_test.cc | 35 ---------- src/resolver/resolver.cc | 29 +++++++- src/resolver/resolver.h | 4 ++ src/resolver/resolver_validation.cc | 67 +++---------------- src/resolver/storage_class_validation_test.cc | 37 ---------- src/resolver/type_validation_test.cc | 50 +++++++++++--- 6 files changed, 81 insertions(+), 141 deletions(-) diff --git a/src/resolver/decoration_validation_test.cc b/src/resolver/decoration_validation_test.cc index 1f41c7a19f..8f043d6abd 100644 --- a/src/resolver/decoration_validation_test.cc +++ b/src/resolver/decoration_validation_test.cc @@ -963,41 +963,6 @@ TEST_F(ArrayStrideTest, DuplicateDecoration) { } // namespace } // namespace ArrayStrideTests -namespace StructBlockTests { -namespace { - -using StructBlockTest = ResolverTest; -TEST_F(StructBlockTest, StructUsedAsArrayElement) { - auto* s = Structure("S", {Member("x", ty.i32())}, - {create()}); - auto* a = ty.array(ty.Of(s), 4); - Global("G", a, ast::StorageClass::kPrivate); - - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), - "error: A structure type with a [[block]] decoration cannot be " - "used as an element of an array"); -} - -TEST_F(StructBlockTest, StructWithNestedBlockMember_Invalid) { - auto* inner = - Structure("Inner", {Member("x", ty.i32())}, - {create(Source{{56, 78}})}); - - auto* outer = - Structure("Outer", {Member(Source{{12, 34}}, "y", ty.Of(inner))}); - - Global("G", ty.Of(outer), ast::StorageClass::kPrivate); - - EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ( - r()->error(), - R"(12:34 error: structs must not contain [[block]] decorated struct members -56:78 note: see member's struct decoration here)"); -} -} // namespace -} // namespace StructBlockTests - namespace ResourceTests { namespace { diff --git a/src/resolver/resolver.cc b/src/resolver/resolver.cc index d211dde089..65820f9953 100644 --- a/src/resolver/resolver.cc +++ b/src/resolver/resolver.cc @@ -43,7 +43,6 @@ #include "src/ast/sampled_texture.h" #include "src/ast/sampler.h" #include "src/ast/storage_texture.h" -#include "src/ast/struct_block_decoration.h" #include "src/ast/switch_statement.h" #include "src/ast/traverse_expressions.h" #include "src/ast/type_name.h" @@ -2707,6 +2706,34 @@ bool Resolver::IsPlain(const sem::Type* type) const { sem::Struct>(); } +// https://gpuweb.github.io/gpuweb/wgsl/#fixed-footprint-types +bool Resolver::IsFixedFootprint(const sem::Type* type) const { + if (type->is_scalar()) { + return true; + } + if (type->Is()) { + return true; + } + if (type->Is()) { + return true; + } + if (type->Is()) { + return true; + } + if (auto* arr = type->As()) { + return !arr->IsRuntimeSized() && IsFixedFootprint(arr->ElemType()); + } + if (auto* str = type->As()) { + for (auto* member : str->Members()) { + if (!IsFixedFootprint(member->Type())) { + return false; + } + } + return true; + } + return false; +} + // https://gpuweb.github.io/gpuweb/wgsl.html#storable-types bool Resolver::IsStorable(const sem::Type* type) const { return IsPlain(type) || type->IsAnyOf(); diff --git a/src/resolver/resolver.h b/src/resolver/resolver.h index ff4cf96da1..8aed02819f 100644 --- a/src/resolver/resolver.h +++ b/src/resolver/resolver.h @@ -92,6 +92,10 @@ class Resolver { /// @returns true if the given type is a plain type bool IsPlain(const sem::Type* type) const; + /// @param type the given type + /// @returns true if the given type is a fixed-footprint type + bool IsFixedFootprint(const sem::Type* type) const; + /// @param type the given type /// @returns true if the given type is storable bool IsStorable(const sem::Type* type) const; diff --git a/src/resolver/resolver_validation.cc b/src/resolver/resolver_validation.cc index 917c8db5b7..1d1199a7b0 100644 --- a/src/resolver/resolver_validation.cc +++ b/src/resolver/resolver_validation.cc @@ -43,7 +43,6 @@ #include "src/ast/sampled_texture.h" #include "src/ast/sampler.h" #include "src/ast/storage_texture.h" -#include "src/ast/struct_block_decoration.h" #include "src/ast/switch_statement.h" #include "src/ast/traverse_expressions.h" #include "src/ast/type_name.h" @@ -541,7 +540,6 @@ bool Resolver::ValidateGlobalVariable(const sem::Variable* var) { // attribute, satisfying the storage class constraints. auto* str = var->Type()->UnwrapRef()->As(); - if (!str) { AddError( "variables declared in the storage class must be of a " @@ -549,17 +547,6 @@ bool Resolver::ValidateGlobalVariable(const sem::Variable* var) { decl->source); return false; } - - if (!str->IsBlockDecorated()) { - AddError( - "structure used as a storage buffer must be declared with the " - "[[block]] decoration", - str->Declaration()->source); - if (decl->source.range.begin.line) { - AddNote("structure used as storage buffer here", decl->source); - } - return false; - } break; } case ast::StorageClass::kUniform: { @@ -575,18 +562,6 @@ bool Resolver::ValidateGlobalVariable(const sem::Variable* var) { decl->source); return false; } - - if (!str->IsBlockDecorated()) { - AddError( - "structure used as a uniform buffer must be declared with the " - "[[block]] decoration", - str->Declaration()->source); - if (decl->source.range.begin.line) { - AddNote("structure used as uniform buffer here", decl->source); - } - return false; - } - for (auto* member : str->Members()) { if (auto* arr = member->Type()->As()) { if (arr->IsRuntimeSized()) { @@ -2050,18 +2025,10 @@ bool Resolver::ValidatePipelineStages() { bool Resolver::ValidateArray(const sem::Array* arr, const Source& source) { auto* el_ty = arr->ElemType(); - if (auto* el_str = el_ty->As()) { - if (el_str->IsBlockDecorated()) { - // https://gpuweb.github.io/gpuweb/wgsl/#attributes - // A structure type with the block attribute must not be: - // * the element type of an array type - // * the member type in another structure - AddError( - "A structure type with a [[block]] decoration cannot be used as an " - "element of an array", - source); - return false; - } + if (!IsFixedFootprint(el_ty)) { + AddError("an array element type cannot contain a runtime-sized array", + source); + return false; } return true; } @@ -2123,15 +2090,13 @@ bool Resolver::ValidateStructure(const sem::Struct* str) { member->Declaration()->source); return false; } - if (!str->IsBlockDecorated()) { - AddError( - "a struct containing a runtime-sized array " - "requires the [[block]] attribute: '" + - builder_->Symbols().NameFor(str->Declaration()->name) + "'", - member->Declaration()->source); - return false; - } } + } else if (!IsFixedFootprint(member->Type())) { + AddError( + "a struct that contains a runtime array cannot be nested inside " + "another struct", + member->Declaration()->source); + return false; } auto has_location = false; @@ -2192,18 +2157,6 @@ bool Resolver::ValidateStructure(const sem::Struct* str) { interpolate_attribute->source); return false; } - - if (auto* member_struct_type = member->Type()->As()) { - if (auto* member_struct_type_block_decoration = - ast::GetDecoration( - member_struct_type->Declaration()->decorations)) { - AddError("structs must not contain [[block]] decorated struct members", - member->Declaration()->source); - AddNote("see member's struct decoration here", - member_struct_type_block_decoration->source); - return false; - } - } } for (auto* deco : str->Declaration()->decorations) { diff --git a/src/resolver/storage_class_validation_test.cc b/src/resolver/storage_class_validation_test.cc index f902a9efb6..9391edcca9 100644 --- a/src/resolver/storage_class_validation_test.cc +++ b/src/resolver/storage_class_validation_test.cc @@ -112,25 +112,6 @@ TEST_F(ResolverStorageClassValidationTest, NotStorage_AccessMode) { R"(56:78 error: only variables in storage class may declare an access mode)"); } -TEST_F(ResolverStorageClassValidationTest, StorageBufferNoBlockDecoration) { - // struct S { x : i32 }; - // var g : S; - auto* s = Structure(Source{{12, 34}}, "S", {Member("x", ty.i32())}); - Global(Source{{56, 78}}, "g", ty.Of(s), ast::StorageClass::kStorage, - ast::Access::kRead, - ast::DecorationList{ - create(0), - create(0), - }); - - ASSERT_FALSE(r()->Resolve()); - - EXPECT_EQ( - r()->error(), - R"(12:34 error: structure used as a storage buffer must be declared with the [[block]] decoration -56:78 note: structure used as storage buffer here)"); -} - TEST_F(ResolverStorageClassValidationTest, StorageBufferNoError_Basic) { // [[block]] struct S { x : i32 }; // var g : S; @@ -249,24 +230,6 @@ TEST_F(ResolverStorageClassValidationTest, UniformBufferBoolAlias) { 56:78 note: while instantiating variable g)"); } -TEST_F(ResolverStorageClassValidationTest, UniformBufferNoBlockDecoration) { - // struct S { x : i32 }; - // var g : S; - auto* s = Structure(Source{{12, 34}}, "S", {Member("x", ty.i32())}); - Global(Source{{56, 78}}, "g", ty.Of(s), ast::StorageClass::kUniform, - ast::DecorationList{ - create(0), - create(0), - }); - - ASSERT_FALSE(r()->Resolve()); - - EXPECT_EQ( - r()->error(), - R"(12:34 error: structure used as a uniform buffer must be declared with the [[block]] decoration -56:78 note: structure used as uniform buffer here)"); -} - TEST_F(ResolverStorageClassValidationTest, UniformBufferNoError_Basic) { // [[block]] struct S { x : i32 }; // var g : S; diff --git a/src/resolver/type_validation_test.cc b/src/resolver/type_validation_test.cc index 82037b3c8b..aa9a7be072 100644 --- a/src/resolver/type_validation_test.cc +++ b/src/resolver/type_validation_test.cc @@ -499,23 +499,51 @@ TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLast_Pass) { EXPECT_TRUE(r()->Resolve()) << r()->error(); } -TEST_F(ResolverTypeValidationTest, RuntimeArrayIsLastNoBlock_Fail) { +TEST_F(ResolverTypeValidationTest, RuntimeArrayInArray) { // struct Foo { - // vf: f32; - // rt: array; + // rt : array, 4>; // }; - Structure("Foo", { - Member("vf", ty.f32()), - Member(Source{{12, 34}}, "rt", ty.array()), - }); - - WrapInFunction(); + Structure("Foo", + {Member("rt", ty.array(Source{{12, 34}}, ty.array(), 4))}); EXPECT_FALSE(r()->Resolve()) << r()->error(); EXPECT_EQ(r()->error(), - "12:34 error: a struct containing a runtime-sized array requires " - "the [[block]] attribute: 'Foo'"); + "12:34 error: an array element type cannot contain a runtime-sized " + "array"); +} + +TEST_F(ResolverTypeValidationTest, RuntimeArrayInStructInArray) { + // struct Foo { + // rt : array; + // }; + // var a : array; + + auto* foo = Structure("Foo", {Member("rt", ty.array())}); + Global("v", ty.array(Source{{12, 34}}, ty.Of(foo), 4), + ast::StorageClass::kPrivate); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: an array element type cannot contain a runtime-sized " + "array"); +} + +TEST_F(ResolverTypeValidationTest, RuntimeArrayInStructInStruct) { + // struct Foo { + // rt : array; + // }; + // struct Outer { + // inner : Foo; + // }; + + auto* foo = Structure("Foo", {Member("rt", ty.array())}); + Structure("Outer", {Member(Source{{12, 34}}, "inner", ty.Of(foo))}); + + EXPECT_FALSE(r()->Resolve()) << r()->error(); + EXPECT_EQ(r()->error(), + "12:34 error: a struct that contains a runtime array cannot be " + "nested inside another struct"); } TEST_F(ResolverTypeValidationTest, RuntimeArrayIsNotLast_Fail) {