From 155165cd52df5692d7af7f6158fb21d7490a4c88 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Mon, 17 Oct 2022 14:35:43 +0000 Subject: [PATCH] Convert the id attribute to expressions. This CL updates the @id attribute to use expressions instead of integers. Bug: tint:1633 Change-Id: I3db9ab39f10a7f50f8d1e418ec508d4e709a24ff Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/106120 Auto-Submit: Dan Sinclair Commit-Queue: Ben Clayton Reviewed-by: Ben Clayton Kokoro: Kokoro Commit-Queue: Dan Sinclair --- src/tint/reader/wgsl/parser_impl.cc | 11 ++- .../reader/wgsl/parser_impl_error_msg_test.cc | 26 ++++- .../parser_impl_global_constant_decl_test.cc | 32 ------ .../parser_impl_variable_attribute_test.cc | 99 +++++++++++++++++++ .../resolver/attribute_validation_test.cc | 35 +++++++ src/tint/resolver/dependency_graph.cc | 11 ++- src/tint/resolver/dependency_graph_test.cc | 1 + src/tint/resolver/resolver.cc | 20 ++-- 8 files changed, 186 insertions(+), 49 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index 7c06020aa2..f15fcce38d 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -3495,15 +3495,16 @@ Maybe ParserImpl::attribute() { if (t == "id") { const char* use = "id 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 id expression"); + } match(Token::Type::kComma); - return create( - t.source(), create( - val.value, ast::IntLiteralExpression::Suffix::kNone)); + return create(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 0fc27dd541..a29c29c09b 100644 --- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc @@ -930,7 +930,7 @@ var i : array; )"); } -TEST_F(ParserImplErrorTest, GlobalDeclVarAttrListMissingComma) { +TEST_F(ParserImplErrorTest, GlobalDeclVarAttrListMissingAt) { EXPECT("@location(1) group(2) var i : i32;", R"(test.wgsl:1:14 error: expected declaration after attributes @location(1) group(2) var i : i32; @@ -966,6 +966,30 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarAttrLocationInvalidValue) { )"); } +TEST_F(ParserImplErrorTest, GlobalDeclVarAttrIdMissingLParen) { + EXPECT("@id 1) var i : i32;", + R"(test.wgsl:1:5 error: expected '(' for id attribute +@id 1) var i : i32; + ^ +)"); +} + +TEST_F(ParserImplErrorTest, GlobalDeclVarAttrIdMissingRParen) { + EXPECT("@id (1 var i : i32;", + R"(test.wgsl:1:8 error: expected ')' for id attribute +@id (1 var i : i32; + ^^^ +)"); +} + +TEST_F(ParserImplErrorTest, GlobalDeclVarAttrIdInvalidValue) { + EXPECT("@id(if) var i : i32;", + R"(test.wgsl:1:5 error: expected id expression +@id(if) var i : i32; + ^^ +)"); +} + TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBuiltinMissingLParen) { EXPECT("@builtin position) var i : i32;", R"(test.wgsl:1:10 error: expected '(' for builtin attribute diff --git a/src/tint/reader/wgsl/parser_impl_global_constant_decl_test.cc b/src/tint/reader/wgsl/parser_impl_global_constant_decl_test.cc index 01ed4d99cf..05310662f3 100644 --- a/src/tint/reader/wgsl/parser_impl_global_constant_decl_test.cc +++ b/src/tint/reader/wgsl/parser_impl_global_constant_decl_test.cc @@ -263,37 +263,5 @@ TEST_F(ParserImplTest, GlobalOverrideDecl_WithoutId) { ASSERT_EQ(id_attr, nullptr); } -TEST_F(ParserImplTest, GlobalOverrideDecl_MissingId) { - auto p = parser("@id() override a : f32 = 1."); - auto attrs = p->attribute_list(); - EXPECT_TRUE(attrs.errored); - EXPECT_FALSE(attrs.matched); - - auto e = p->global_constant_decl(attrs.value); - EXPECT_TRUE(e.matched); - EXPECT_FALSE(e.errored); - auto* override = e.value->As(); - ASSERT_NE(override, nullptr); - - EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:5: expected signed integer literal for id attribute"); -} - -TEST_F(ParserImplTest, GlobalOverrideDecl_InvalidId) { - auto p = parser("@id(-7) override a : f32 = 1."); - auto attrs = p->attribute_list(); - EXPECT_TRUE(attrs.errored); - EXPECT_FALSE(attrs.matched); - - auto e = p->global_constant_decl(attrs.value); - EXPECT_TRUE(e.matched); - EXPECT_FALSE(e.errored); - auto* override = e.value->As(); - ASSERT_NE(override, nullptr); - - EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:5: id attribute must be positive"); -} - } // namespace } // namespace tint::reader::wgsl diff --git a/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc b/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc index 86e5487449..4f3f1b96b8 100644 --- a/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc +++ b/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc @@ -17,6 +17,105 @@ namespace tint::reader::wgsl { namespace { +TEST_F(ParserImplTest, Attribute_Id) { + auto p = parser("id(4)"); + auto attr = p->attribute(); + EXPECT_TRUE(attr.matched); + EXPECT_FALSE(attr.errored); + ASSERT_NE(attr.value, nullptr); + auto* var_attr = attr.value->As(); + ASSERT_NE(var_attr, nullptr); + ASSERT_FALSE(p->has_error()); + ASSERT_TRUE(var_attr->Is()); + + auto* loc = var_attr->As(); + ASSERT_TRUE(loc->expr->Is()); + auto* exp = loc->expr->As(); + EXPECT_EQ(exp->value, 4u); +} + +TEST_F(ParserImplTest, Attribute_Id_Expression) { + auto p = parser("id(4 + 5)"); + auto attr = p->attribute(); + EXPECT_TRUE(attr.matched); + EXPECT_FALSE(attr.errored); + ASSERT_NE(attr.value, nullptr); + auto* var_attr = attr.value->As(); + ASSERT_NE(var_attr, nullptr); + ASSERT_FALSE(p->has_error()); + ASSERT_TRUE(var_attr->Is()); + + auto* loc = var_attr->As(); + ASSERT_TRUE(loc->expr->Is()); + auto* expr = loc->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_Id_TrailingComma) { + auto p = parser("id(4,)"); + auto attr = p->attribute(); + EXPECT_TRUE(attr.matched); + EXPECT_FALSE(attr.errored); + ASSERT_NE(attr.value, nullptr); + auto* var_attr = attr.value->As(); + ASSERT_NE(var_attr, nullptr); + ASSERT_FALSE(p->has_error()); + ASSERT_TRUE(var_attr->Is()); + + auto* loc = var_attr->As(); + ASSERT_TRUE(loc->expr->Is()); + auto* exp = loc->expr->As(); + EXPECT_EQ(exp->value, 4u); +} + +TEST_F(ParserImplTest, Attribute_Id_MissingLeftParen) { + auto p = parser("id 4)"); + 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:4: expected '(' for id attribute"); +} + +TEST_F(ParserImplTest, Attribute_Id_MissingRightParen) { + auto p = parser("id(4"); + 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:5: expected ')' for id attribute"); +} + +TEST_F(ParserImplTest, Attribute_Id_MissingValue) { + auto p = parser("id()"); + 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:4: expected id expression"); +} + +TEST_F(ParserImplTest, Attribute_Id_MissingInvalid) { + auto p = parser("id(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:4: expected id expression"); +} + TEST_F(ParserImplTest, Attribute_Location) { auto p = parser("location(4)"); auto attr = p->attribute(); diff --git a/src/tint/resolver/attribute_validation_test.cc b/src/tint/resolver/attribute_validation_test.cc index 3e092edf6f..212f5946cd 100644 --- a/src/tint/resolver/attribute_validation_test.cc +++ b/src/tint/resolver/attribute_validation_test.cc @@ -1640,6 +1640,41 @@ TEST_F(GroupAndBindingTest, Group_AFloat) { EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)"); } +using IdTest = ResolverTest; + +TEST_F(IdTest, Const_I32) { + Override("val", ty.f32(), utils::Vector{Id(1_i)}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(IdTest, Const_U32) { + Override("val", ty.f32(), utils::Vector{Id(1_u)}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(IdTest, Const_AInt) { + Override("val", ty.f32(), utils::Vector{Id(1_a)}); + EXPECT_TRUE(r()->Resolve()) << r()->error(); +} + +TEST_F(IdTest, Negative) { + Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, -1_i)}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'id' value must be non-negative)"); +} + +TEST_F(IdTest, F32) { + Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1_f)}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)"); +} + +TEST_F(IdTest, AFloat) { + Override("val", ty.f32(), utils::Vector{Id(Source{{12, 34}}, 1.0_a)}); + EXPECT_FALSE(r()->Resolve()); + EXPECT_EQ(r()->error(), R"(12:34 error: 'id' must be an i32 or u32 value)"); +} + } // namespace } // namespace InterpolateTests diff --git a/src/tint/resolver/dependency_graph.cc b/src/tint/resolver/dependency_graph.cc index 514e0cb84a..4d67023d07 100644 --- a/src/tint/resolver/dependency_graph.cc +++ b/src/tint/resolver/dependency_graph.cc @@ -427,6 +427,10 @@ class DependencyScanner { TraverseExpression(group->expr); return true; }, + [&](const ast::IdAttribute* id) { + TraverseExpression(id->expr); + return true; + }, [&](const ast::StructMemberAlignAttribute* align) { TraverseExpression(align->expr); return true; @@ -445,10 +449,9 @@ class DependencyScanner { return; } - if (attr->IsAnyOf()) { + if (attr->IsAnyOf()) { return; } diff --git a/src/tint/resolver/dependency_graph_test.cc b/src/tint/resolver/dependency_graph_test.cc index 150d65ae76..138507ac9b 100644 --- a/src/tint/resolver/dependency_graph_test.cc +++ b/src/tint/resolver/dependency_graph_test.cc @@ -1291,6 +1291,7 @@ TEST_F(ResolverDependencyGraphTraversalTest, SymbolsReached) { GlobalVar(Sym(), ty.sampler(ast::SamplerKind::kSampler)); GlobalVar(Sym(), ty.i32(), utils::Vector{Binding(V), Group(V)}); + Override(Sym(), ty.i32(), utils::Vector{Id(V)}); Func(Sym(), utils::Empty, ty.void_(), utils::Empty); #undef V diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 9bbe4399d4..56f9246a65 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -449,18 +449,24 @@ sem::Variable* Resolver::Override(const ast::Override* v) { sem->SetConstructor(rhs); if (auto* id_attr = ast::GetAttribute(v->attributes)) { - auto* materialize = Materialize(Expression(id_attr->expr)); - if (!materialize) { + ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@id"}; + TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint); + + auto* materialized = Materialize(Expression(id_attr->expr)); + if (!materialized) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Handle invalid materialization when expressions are - // supported. + if (!materialized->Type()->IsAnyOf()) { + AddError("'id' must be an i32 or u32 value", id_attr->source); return nullptr; } - auto value = c->As(); + auto const_value = materialized->ConstantValue(); + auto value = const_value->As(); + if (value < 0) { + AddError("'id' value must be non-negative", id_attr->source); + return nullptr; + } if (value > std::numeric_limits::max()) { AddError("override IDs must be between 0 and " + std::to_string(std::numeric_limits::max()),