From 3fd42ae042e0a97de8af43061b739f3b492629a1 Mon Sep 17 00:00:00 2001 From: dan sinclair Date: Mon, 17 Oct 2022 15:21:48 +0000 Subject: [PATCH] Convert the location attribute to expressions. This CL updates the @location attribute to use expressions instead of integers. Bug: tint:1633 Change-Id: If4dfca6d39e5134bb173209414ad8d2528c8095d Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/106121 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 | 8 ++--- .../parser_impl_struct_attribute_decl_test.cc | 2 +- .../parser_impl_variable_attribute_test.cc | 31 +++++++++++++++++-- src/tint/resolver/dependency_graph.cc | 8 +++-- src/tint/resolver/dependency_graph_test.cc | 1 + src/tint/resolver/resolver.cc | 19 +++++++----- 7 files changed, 58 insertions(+), 22 deletions(-) diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc index f15fcce38d..8d6040d9d8 100644 --- a/src/tint/reader/wgsl/parser_impl.cc +++ b/src/tint/reader/wgsl/parser_impl.cc @@ -3539,15 +3539,16 @@ Maybe ParserImpl::attribute() { if (t == "location") { const char* use = "location 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 location expression"); + } match(Token::Type::kComma); - return builder_.Location(t.source(), - create( - val.value, ast::IntLiteralExpression::Suffix::kNone)); + return builder_.Location(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 a29c29c09b..9416a4d451 100644 --- a/src/tint/reader/wgsl/parser_impl_error_msg_test.cc +++ b/src/tint/reader/wgsl/parser_impl_error_msg_test.cc @@ -959,10 +959,10 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarAttrLocationMissingRParen) { } TEST_F(ParserImplErrorTest, GlobalDeclVarAttrLocationInvalidValue) { - EXPECT("@location(x) var i : i32;", - R"(test.wgsl:1:11 error: expected signed integer literal for location attribute -@location(x) var i : i32; - ^ + EXPECT("@location(if) var i : i32;", + R"(test.wgsl:1:11 error: expected location expression +@location(if) var i : i32; + ^^ )"); } diff --git a/src/tint/reader/wgsl/parser_impl_struct_attribute_decl_test.cc b/src/tint/reader/wgsl/parser_impl_struct_attribute_decl_test.cc index e015033154..051104b5e8 100644 --- a/src/tint/reader/wgsl/parser_impl_struct_attribute_decl_test.cc +++ b/src/tint/reader/wgsl/parser_impl_struct_attribute_decl_test.cc @@ -46,7 +46,7 @@ TEST_F(ParserImplTest, AttributeDecl_MissingValue) { EXPECT_TRUE(attrs.errored); EXPECT_FALSE(attrs.matched); EXPECT_TRUE(attrs.value.IsEmpty()); - EXPECT_EQ(p->error(), "1:11: expected signed integer literal for location attribute"); + EXPECT_EQ(p->error(), "1:11: expected location expression"); } TEST_F(ParserImplTest, AttributeDecl_MissingParenRight) { 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 4f3f1b96b8..1b538a27c9 100644 --- a/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc +++ b/src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc @@ -133,6 +133,31 @@ TEST_F(ParserImplTest, Attribute_Location) { EXPECT_EQ(exp->value, 4u); } +TEST_F(ParserImplTest, Attribute_Location_Expression) { + auto p = parser("location(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_Location_TrailingComma) { auto p = parser("location(4,)"); auto attr = p->attribute(); @@ -177,17 +202,17 @@ TEST_F(ParserImplTest, Attribute_Location_MissingValue) { EXPECT_TRUE(attr.errored); EXPECT_EQ(attr.value, nullptr); EXPECT_TRUE(p->has_error()); - EXPECT_EQ(p->error(), "1:10: expected signed integer literal for location attribute"); + EXPECT_EQ(p->error(), "1:10: expected location expression"); } TEST_F(ParserImplTest, Attribute_Location_MissingInvalid) { - auto p = parser("location(nan)"); + auto p = parser("location(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:10: expected signed integer literal for location attribute"); + EXPECT_EQ(p->error(), "1:10: expected location expression"); } struct BuiltinData { diff --git a/src/tint/resolver/dependency_graph.cc b/src/tint/resolver/dependency_graph.cc index 4d67023d07..e84eec5679 100644 --- a/src/tint/resolver/dependency_graph.cc +++ b/src/tint/resolver/dependency_graph.cc @@ -431,6 +431,10 @@ class DependencyScanner { TraverseExpression(id->expr); return true; }, + [&](const ast::LocationAttribute* loc) { + TraverseExpression(loc->expr); + return true; + }, [&](const ast::StructMemberAlignAttribute* align) { TraverseExpression(align->expr); return true; @@ -450,8 +454,8 @@ class DependencyScanner { } if (attr->IsAnyOf()) { + ast::InvariantAttribute, ast::StageAttribute, ast::StrideAttribute, + ast::StructMemberOffsetAttribute>()) { return; } diff --git a/src/tint/resolver/dependency_graph_test.cc b/src/tint/resolver/dependency_graph_test.cc index 138507ac9b..724a527457 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)}); + GlobalVar(Sym(), ty.i32(), utils::Vector{Location(V)}); Override(Sym(), ty.i32(), utils::Vector{Id(V)}); Func(Sym(), utils::Empty, ty.void_(), utils::Empty); diff --git a/src/tint/resolver/resolver.cc b/src/tint/resolver/resolver.cc index 56f9246a65..c2524732b4 100644 --- a/src/tint/resolver/resolver.cc +++ b/src/tint/resolver/resolver.cc @@ -675,17 +675,22 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) { std::optional location; if (auto* attr = ast::GetAttribute(var->attributes)) { - auto* materialize = Materialize(Expression(attr->expr)); - if (!materialize) { + auto* materialized = Materialize(Expression(attr->expr)); + if (!materialized) { return nullptr; } - auto* c = materialize->ConstantValue(); - if (!c) { - // TODO(crbug.com/tint/1633): Add error message about invalid materialization - // when location can be an expression. + if (!materialized->Type()->IsAnyOf()) { + AddError("'location' must be an i32 or u32 value", attr->source); return nullptr; } - location = c->As(); + + auto const_value = materialized->ConstantValue(); + auto value = const_value->As(); + if (value < 0) { + AddError("'location' value must be non-negative", attr->source); + return nullptr; + } + location = u32(value); } sem = builder_->create(