From 308c55d9e0cbad1cf2a1f11ac8acb3face71ceee Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Fri, 14 Oct 2022 18:27:35 +0000 Subject: [PATCH] Convert `size` attribute to expressions. This CL updates the size attribute to parse expressions. Bug: tint:1633 Change-Id: Ia12650848e7041faa53013d195f4313b8d3e9969 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/103320 Commit-Queue: Dan Sinclair Kokoro: Kokoro Reviewed-by: Ben Clayton Auto-Submit: Dan Sinclair --- src/tint/reader/wgsl/parser_impl.cc | 9 ++- .../reader/wgsl/parser_impl_error_msg_test.cc | 14 +--- .../wgsl/parser_impl_struct_body_decl_test.cc | 4 +- ..._impl_struct_member_attribute_decl_test.cc | 4 +- ...arser_impl_struct_member_attribute_test.cc | 36 ++++++++-- .../wgsl/parser_impl_struct_member_test.cc | 4 +- .../resolver/attribute_validation_test.cc | 72 +++++++++++++++++++ src/tint/resolver/dependency_graph.cc | 7 +- src/tint/resolver/dependency_graph_test.cc | 8 ++- src/tint/resolver/resolver.cc | 15 +++- src/tint/resolver/validation_test.cc | 4 +- 11 files changed, 143 insertions(+), 34 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 3dad333bd6..841649d9f4 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -3551,13 +3551,16 @@ Maybe ParserImpl::attribute() { if (t == "size") { const char* use = "size attribute"; return expect_paren_block(use, [&]() -> Result { - auto val = expect_positive_sint(use); - if (val.errored) { + auto expr = expression(); + if (expr.errored) { return Failure::kErrored; } + if (!expr.matched) { + return add_error(peek(), "expected size expression"); + } match(Token::Type::kComma); - return builder_.MemberSize(t.source(), AInt(val.value)); + return builder_.MemberSize(t.source(), expr.value); }); } diff --git a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc index af9734c0fe..19434e64ca 100644 --- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc @@ -862,17 +862,9 @@ struct S { @align(fn) i : i32, }; } TEST_F(ParserImplErrorTest, GlobalDeclStructMemberSizeInvaldValue) { - EXPECT("struct S { @size(x) i : i32, };", - R"(test.wgsl:1:18 error: expected signed integer literal for size attribute -struct S { @size(x) i : i32, }; - ^ -)"); -} - -TEST_F(ParserImplErrorTest, GlobalDeclStructMemberSizeNegativeValue) { - EXPECT("struct S { @size(-2) i : i32, };", - R"(test.wgsl:1:18 error: size attribute must be positive -struct S { @size(-2) i : i32, }; + EXPECT("struct S { @size(if) i : i32, };", + R"(test.wgsl:1:18 error: expected size expression +struct S { @size(if) i : i32, }; ^^ )"); } diff --git a/src/tint/reader/wgsl/parser_impl_struct_body_decl_test.cc b/src/tint/reader/wgsl/parser_impl_struct_body_decl_test.cc index ce56a676c9..e994230804 100644 --- a/src/tint/reader/wgsl/parser_impl_struct_body_decl_test.cc +++ b/src/tint/reader/wgsl/parser_impl_struct_body_decl_test.cc @@ -71,12 +71,12 @@ TEST_F(ParserImplTest, StructBodyDecl_InvalidAlign) { TEST_F(ParserImplTest, StructBodyDecl_InvalidSize) { auto p = parser(R"( { - @size(nan) a : i32, + @size(if) a : i32, })"); auto m = p->expect_struct_body_decl(); ASSERT_TRUE(p->has_error()); ASSERT_TRUE(m.errored); - EXPECT_EQ(p->error(), "3:9: expected signed integer literal for size attribute"); + EXPECT_EQ(p->error(), "3:9: expected size expression"); } TEST_F(ParserImplTest, StructBodyDecl_MissingClosingBracket) { diff --git a/src/tint/reader/wgsl/parser_impl_struct_member_attribute_decl_test.cc b/src/tint/reader/wgsl/parser_impl_struct_member_attribute_decl_test.cc index 09d5274fe1..0417f7c133 100644 --- a/src/tint/reader/wgsl/parser_impl_struct_member_attribute_decl_test.cc +++ b/src/tint/reader/wgsl/parser_impl_struct_member_attribute_decl_test.cc @@ -39,12 +39,12 @@ TEST_F(ParserImplTest, AttributeDecl_Single) { } TEST_F(ParserImplTest, AttributeDecl_InvalidAttribute) { - auto p = parser("@size(nan)"); + auto p = parser("@size(if)"); auto attrs = p->attribute_list(); EXPECT_TRUE(p->has_error()) << p->error(); EXPECT_TRUE(attrs.errored); EXPECT_FALSE(attrs.matched); - EXPECT_EQ(p->error(), "1:7: expected signed integer literal for size attribute"); + EXPECT_EQ(p->error(), "1:7: expected size expression"); } } // namespace diff --git a/src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc b/src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc index f6957e4c66..80376d83f9 100644 --- a/src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc +++ b/src/tint/reader/wgsl/parser_impl_struct_member_attribute_test.cc @@ -23,7 +23,7 @@ TEST_F(ParserImplTest, Attribute_Size) { EXPECT_TRUE(attr.matched); EXPECT_FALSE(attr.errored); ASSERT_NE(attr.value, nullptr); - ASSERT_FALSE(p->has_error()); + ASSERT_FALSE(p->has_error()) << p->error(); auto* member_attr = attr.value->As(); ASSERT_NE(member_attr, nullptr); @@ -34,13 +34,39 @@ TEST_F(ParserImplTest, Attribute_Size) { EXPECT_EQ(o->expr->As()->value, 4u); } +TEST_F(ParserImplTest, Attribute_Size_Expression) { + auto p = parser("size(4 + 5)"); + auto attr = p->attribute(); + EXPECT_TRUE(attr.matched); + EXPECT_FALSE(attr.errored); + ASSERT_NE(attr.value, nullptr); + ASSERT_FALSE(p->has_error()) << p->error(); + + auto* member_attr = attr.value->As(); + ASSERT_NE(member_attr, nullptr); + ASSERT_TRUE(member_attr->Is()); + + auto* o = member_attr->As(); + ASSERT_TRUE(o->expr->Is()); + auto* expr = o->expr->As(); + + EXPECT_EQ(ast::BinaryOp::kAdd, expr->op); + auto* v = expr->lhs->As(); + ASSERT_NE(nullptr, v); + EXPECT_EQ(v->value, 4u); + + v = expr->rhs->As(); + ASSERT_NE(nullptr, v); + EXPECT_EQ(v->value, 5u); +} + TEST_F(ParserImplTest, Attribute_Size_TrailingComma) { auto p = parser("size(4,)"); auto attr = p->attribute(); EXPECT_TRUE(attr.matched); EXPECT_FALSE(attr.errored); ASSERT_NE(attr.value, nullptr); - ASSERT_FALSE(p->has_error()); + ASSERT_FALSE(p->has_error()) << p->error(); auto* member_attr = attr.value->As(); ASSERT_NE(member_attr, nullptr); @@ -78,17 +104,17 @@ TEST_F(ParserImplTest, Attribute_Size_MissingValue) { EXPECT_TRUE(attr.errored); EXPECT_EQ(attr.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:6: expected signed integer literal for size attribute"); + EXPECT_EQ(p->error(), "1:6: expected size expression"); } TEST_F(ParserImplTest, Attribute_Size_MissingInvalid) { - auto p = parser("size(nan)"); + auto p = parser("size(if)"); auto attr = p->attribute(); EXPECT_FALSE(attr.matched); EXPECT_TRUE(attr.errored); EXPECT_EQ(attr.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:6: expected signed integer literal for size attribute"); + EXPECT_EQ(p->error(), "1:6: expected size expression"); } TEST_F(ParserImplTest, Attribute_Align) { diff --git a/src/tint/reader/wgsl/parser_impl_struct_member_test.cc b/src/tint/reader/wgsl/parser_impl_struct_member_test.cc index 6267406571..53174f6adb 100644 --- a/src/tint/reader/wgsl/parser_impl_struct_member_test.cc +++ b/src/tint/reader/wgsl/parser_impl_struct_member_test.cc @@ -115,14 +115,14 @@ TEST_F(ParserImplTest, StructMember_ParsesWithMultipleattributes) { } TEST_F(ParserImplTest, StructMember_InvalidAttribute) { - auto p = parser("@size(nan) a : i32,"); + auto p = parser("@size(if) a : i32,"); auto m = p->expect_struct_member(); ASSERT_TRUE(m.errored); ASSERT_EQ(m.value, nullptr); ASSERT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:7: expected signed integer literal for size attribute"); + EXPECT_EQ(p->error(), "1:7: expected size expression"); } } // namespace diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc index 30615bb7d9..317fab041e 100644 --- a/src/tint/resolver/attribute_validation_test.cc +++ b/src/tint/resolver/attribute_validation_test.cc @@ -743,6 +743,78 @@ TEST_F(StructMemberAttributeTest, Align_Attribute_Override) { R"(error: @align requires a const-expression, but expression is an override-expression)"); } +TEST_F(StructMemberAttributeTest, Size_Attribute_Const) { + GlobalConst("val", ty.i32(), Expr(4_i)); + + Structure("mystruct", utils::Vector{Member("a", ty.f32(), utils::Vector{MemberSize("val")})}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_ConstNegative) { + GlobalConst("val", ty.i32(), Expr(-2_i)); + + Structure("mystruct", utils::Vector{Member( + "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'size' attribute must be positive)"); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_ConstF32) { + GlobalConst("val", ty.f32(), Expr(1.23_f)); + + Structure("mystruct", utils::Vector{Member( + "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'size' must be an i32 or u32 value)"); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_ConstU32) { + GlobalConst("val", ty.u32(), Expr(4_u)); + + Structure("mystruct", utils::Vector{Member( + "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_ConstAInt) { + GlobalConst("val", Expr(4_a)); + + Structure("mystruct", utils::Vector{Member( + "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_ConstAFloat) { + GlobalConst("val", Expr(2.0_a)); + + Structure("mystruct", utils::Vector{Member( + "a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, "val")})}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'size' must be an i32 or u32 value)"); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_Var) { + GlobalVar(Source{{1, 2}}, "val", ty.f32(), ast::AddressSpace::kPrivate, ast::Access::kUndefined, + Expr(1.23_f)); + + Structure(Source{{6, 4}}, "mystruct", + utils::Vector{Member(Source{{12, 5}}, "a", ty.f32(), + utils::Vector{MemberSize(Expr(Source{{12, 35}}, "val"))})}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:35 error: var 'val' cannot be referenced at module-scope +1:2 note: var 'val' declared here)"); +} + +TEST_F(StructMemberAttributeTest, Size_Attribute_Override) { + Override("val", ty.f32(), Expr(1.23_f)); + + Structure("mystruct", utils::Vector{Member("a", ty.f32(), utils::Vector{MemberSize("val")})}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ( + r()->error(), + R"(error: @size requires a const-expression, but expression is an override-expression)"); +} + } // namespace StructAndStructMemberTests using ArrayAttributeTest = TestWithParams; diff --git a/src/tint/resolver/dependency_graph.cc b/src/tint/resolver/dependency_graph.cc index 0b391c55f4..168734fa62 100644 --- a/src/tint/resolver/dependency_graph.cc +++ b/src/tint/resolver/dependency_graph.cc @@ -426,12 +426,15 @@ class DependencyScanner { TraverseExpression(align->expr); return; } + if (auto* size = attr->As()) { + TraverseExpression(size->expr); + return; + } if (attr->IsAnyOf()) { + ast::StrideAttribute, ast::StructMemberOffsetAttribute>()) { return; } diff --git a/src/tint/resolver/dependency_graph_test.cc b/src/tint/resolver/dependency_graph_test.cc index 614873e51a..53bf3eedea 100644 --- a/src/tint/resolver/dependency_graph_test.cc +++ b/src/tint/resolver/dependency_graph_test.cc @@ -1230,9 +1230,11 @@ TEST_F(ResolverDependencyGraphTraversalTest, SymbolsReached) { Alias(Sym(), T); Structure(Sym(), // - utils::Vector{ - Member(Sym(), T, utils::Vector{MemberAlign(V)}) // - }); + utils::Vector{Member(Sym(), T, + utils::Vector{ + // + MemberAlign(V), MemberSize(V) // + })}); GlobalVar(Sym(), T, V); GlobalConst(Sym(), T, V); Func(Sym(), // diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 1a69de1429..0ae9cd9030 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -2885,15 +2885,26 @@ sem::Struct* Resolver::Structure(const ast::Struct* str) { if (!materialized) { return nullptr; } + if (!materialized->Type()->IsAnyOf()) { + AddError("'size' must be an i32 or u32 value", s->source); + return nullptr; + } + auto const_value = materialized->ConstantValue(); if (!const_value) { AddError("'size' must be constant expression", s->expr->source); return nullptr; } + { + auto value = const_value->As(); + if (value <= 0) { + AddError("'size' attribute must be positive", s->source); + return nullptr; + } + } auto value = const_value->As(); - if (value < size) { - AddError("size must be at least as big as the type's size (" + + AddError("'size' must be at least as big as the type's size (" + std::to_string(size) + ")", s->source); return nullptr; diff --git a/src/tint/resolver/validation_test.cc b/src/tint/resolver/validation_test.cc index f4fa3ccfaa..edce7ebd81 100644 --- a/src/tint/resolver/validation_test.cc +++ b/src/tint/resolver/validation_test.cc @@ -1266,11 +1266,11 @@ TEST_F(ResolverValidationTest, ZeroStructMemberAlignAttribute) { TEST_F(ResolverValidationTest, ZeroStructMemberSizeAttribute) { Structure("S", utils::Vector{ - Member("a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, 0_a)}), + Member("a", ty.f32(), utils::Vector{MemberSize(Source{{12, 34}}, 1_a)}), }); EXPECT_FALSE(r()->Resolve()); - EXPECT_EQ(r()->error(), "12:34 error: size must be at least as big as the type's size (4)"); + EXPECT_EQ(r()->error(), "12:34 error: 'size' must be at least as big as the type's size (4)"); } TEST_F(ResolverValidationTest, OffsetAndSizeAttribute) {