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 <dsinclair@chromium.org>
Commit-Queue: Ben Clayton <bclayton@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
diff --git a/src/tint/reader/wgsl/parser_impl.cc b/src/tint/reader/wgsl/parser_impl.cc
index 7c06020..f15fcce 100644
--- a/src/tint/reader/wgsl/parser_impl.cc
+++ b/src/tint/reader/wgsl/parser_impl.cc
@@ -3495,15 +3495,16 @@
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<ast::IdAttribute>(
- t.source(), create<ast::IntLiteralExpression>(
- val.value, ast::IntLiteralExpression::Suffix::kNone));
+ return create<ast::IdAttribute>(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 0fc27dd..a29c29c 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 @@
)");
}
-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, 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 01ed4d9..0531066 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 @@
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<ast::Override>();
- 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<ast::Override>();
- 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 86e5487..4f3f1b9 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<ast::Attribute>();
+ ASSERT_NE(var_attr, nullptr);
+ ASSERT_FALSE(p->has_error());
+ ASSERT_TRUE(var_attr->Is<ast::IdAttribute>());
+
+ auto* loc = var_attr->As<ast::IdAttribute>();
+ ASSERT_TRUE(loc->expr->Is<ast::IntLiteralExpression>());
+ auto* exp = loc->expr->As<ast::IntLiteralExpression>();
+ 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<ast::Attribute>();
+ ASSERT_NE(var_attr, nullptr);
+ ASSERT_FALSE(p->has_error());
+ ASSERT_TRUE(var_attr->Is<ast::IdAttribute>());
+
+ auto* loc = var_attr->As<ast::IdAttribute>();
+ ASSERT_TRUE(loc->expr->Is<ast::BinaryExpression>());
+ auto* expr = loc->expr->As<ast::BinaryExpression>();
+
+ EXPECT_EQ(ast::BinaryOp::kAdd, expr->op);
+ auto* v = expr->lhs->As<ast::IntLiteralExpression>();
+ ASSERT_NE(nullptr, v);
+ EXPECT_EQ(v->value, 4u);
+
+ v = expr->rhs->As<ast::IntLiteralExpression>();
+ 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<ast::Attribute>();
+ ASSERT_NE(var_attr, nullptr);
+ ASSERT_FALSE(p->has_error());
+ ASSERT_TRUE(var_attr->Is<ast::IdAttribute>());
+
+ auto* loc = var_attr->As<ast::IdAttribute>();
+ ASSERT_TRUE(loc->expr->Is<ast::IntLiteralExpression>());
+ auto* exp = loc->expr->As<ast::IntLiteralExpression>();
+ 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 3e092ed..212f594 100644
--- a/src/tint/resolver/attribute_validation_test.cc
+++ b/src/tint/resolver/attribute_validation_test.cc
@@ -1640,6 +1640,41 @@
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 514e0cb..4d67023 100644
--- a/src/tint/resolver/dependency_graph.cc
+++ b/src/tint/resolver/dependency_graph.cc
@@ -427,6 +427,10 @@
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 @@
return;
}
- if (attr->IsAnyOf<ast::BuiltinAttribute, ast::IdAttribute, ast::InternalAttribute,
- ast::InterpolateAttribute, ast::InvariantAttribute,
- ast::LocationAttribute, ast::StageAttribute, ast::StrideAttribute,
- ast::StructMemberOffsetAttribute>()) {
+ if (attr->IsAnyOf<ast::BuiltinAttribute, ast::InternalAttribute, ast::InterpolateAttribute,
+ ast::InvariantAttribute, ast::LocationAttribute, 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 150d65a..138507a 100644
--- a/src/tint/resolver/dependency_graph_test.cc
+++ b/src/tint/resolver/dependency_graph_test.cc
@@ -1291,6 +1291,7 @@
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 9bbe439..56f9246 100644
--- a/src/tint/resolver/resolver.cc
+++ b/src/tint/resolver/resolver.cc
@@ -449,18 +449,24 @@
sem->SetConstructor(rhs);
if (auto* id_attr = ast::GetAttribute<ast::IdAttribute>(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<sem::I32, sem::U32>()) {
+ AddError("'id' must be an i32 or u32 value", id_attr->source);
return nullptr;
}
- auto value = c->As<uint32_t>();
+ auto const_value = materialized->ConstantValue();
+ auto value = const_value->As<AInt>();
+ if (value < 0) {
+ AddError("'id' value must be non-negative", id_attr->source);
+ return nullptr;
+ }
if (value > std::numeric_limits<decltype(OverrideId::value)>::max()) {
AddError("override IDs must be between 0 and " +
std::to_string(std::numeric_limits<decltype(OverrideId::value)>::max()),
