Convert binding and group attributes to expressions.

This CL updates the @group and @binding attributes to use
expressions instead of integers.

Bug: tint:1633
Change-Id: I91068874c104d5b84390f1617cb96265dda6e1e0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/105801
Auto-Submit: Dan Sinclair <dsinclair@chromium.org>
Commit-Queue: Dan Sinclair <dsinclair@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>

src/tint/reader/wgsl/parser_impl.cc

@@ -3441,15 +3441,16 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
     if (t == "binding") {
         const char* use = "binding 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 binding expression");
+            }
             match(Token::Type::kComma);
 
-            return create<ast::BindingAttribute>(
-                t.source(), create<ast::IntLiteralExpression>(
-                                val.value, ast::IntLiteralExpression::Suffix::kNone));
+            return create<ast::BindingAttribute>(t.source(), expr.value);
         });
     }
 
@@ -3478,15 +3479,16 @@ Maybe<const ast::Attribute*> ParserImpl::attribute() {
     if (t == "group") {
         const char* use = "group 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 group expression");
+            }
             match(Token::Type::kComma);
 
-            return create<ast::GroupAttribute>(
-                t.source(), create<ast::IntLiteralExpression>(
-                                val.value, ast::IntLiteralExpression::Suffix::kNone));
+            return create<ast::GroupAttribute>(t.source(), expr.value);
         });
     }
 

src/tint/reader/wgsl/parser_impl_error_msg_test.cc

@@ -1017,10 +1017,10 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBindingMissingRParen) {
 }
 
 TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBindingInvalidValue) {
-    EXPECT("@binding(x) var i : i32;",
-           R"(test.wgsl:1:10 error: expected signed integer literal for binding attribute
-@binding(x) var i : i32;
-         ^
+    EXPECT("@binding(if) var i : i32;",
+           R"(test.wgsl:1:10 error: expected binding expression
+@binding(if) var i : i32;
+         ^^
 )");
 }
 
@@ -1041,10 +1041,10 @@ TEST_F(ParserImplErrorTest, GlobalDeclVarAttrGroupMissingRParen) {
 }
 
 TEST_F(ParserImplErrorTest, GlobalDeclVarAttrBindingGroupValue) {
-    EXPECT("@group(x) var i : i32;",
-           R"(test.wgsl:1:8 error: expected signed integer literal for group attribute
-@group(x) var i : i32;
-       ^
+    EXPECT("@group(if) var i : i32;",
+           R"(test.wgsl:1:8 error: expected group expression
+@group(if) var i : i32;
+       ^^
 )");
 }
 

src/tint/reader/wgsl/parser_impl_global_variable_decl_test.cc

@@ -139,7 +139,7 @@ TEST_F(ParserImplTest, GlobalVariableDecl_InvalidAttribute) {
     EXPECT_NE(e.value, nullptr);
 
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:10: expected signed integer literal for binding attribute");
+    EXPECT_EQ(p->error(), "1:10: expected binding expression");
 }
 
 TEST_F(ParserImplTest, GlobalVariableDecl_InvalidConstExpr) {

src/tint/reader/wgsl/parser_impl_variable_attribute_test.cc

@@ -392,6 +392,31 @@ TEST_F(ParserImplTest, Attribute_Binding) {
     EXPECT_EQ(expr->suffix, ast::IntLiteralExpression::Suffix::kNone);
 }
 
+TEST_F(ParserImplTest, Attribute_Binding_Expression) {
+    auto p = parser("binding(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::BindingAttribute>());
+
+    auto* binding = var_attr->As<ast::BindingAttribute>();
+    ASSERT_TRUE(binding->expr->Is<ast::BinaryExpression>());
+    auto* expr = binding->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_Binding_TrailingComma) {
     auto p = parser("binding(4,)");
     auto attr = p->attribute();
@@ -437,17 +462,17 @@ TEST_F(ParserImplTest, Attribute_Binding_MissingValue) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:9: expected signed integer literal for binding attribute");
+    EXPECT_EQ(p->error(), "1:9: expected binding expression");
 }
 
 TEST_F(ParserImplTest, Attribute_Binding_MissingInvalid) {
-    auto p = parser("binding(nan)");
+    auto p = parser("binding(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:9: expected signed integer literal for binding attribute");
+    EXPECT_EQ(p->error(), "1:9: expected binding expression");
 }
 
 TEST_F(ParserImplTest, Attribute_group) {
@@ -468,6 +493,31 @@ TEST_F(ParserImplTest, Attribute_group) {
     EXPECT_EQ(expr->suffix, ast::IntLiteralExpression::Suffix::kNone);
 }
 
+TEST_F(ParserImplTest, Attribute_group_expression) {
+    auto p = parser("group(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_FALSE(p->has_error());
+    ASSERT_NE(var_attr, nullptr);
+    ASSERT_TRUE(var_attr->Is<ast::GroupAttribute>());
+
+    auto* group = var_attr->As<ast::GroupAttribute>();
+    ASSERT_TRUE(group->expr->Is<ast::BinaryExpression>());
+    auto* expr = group->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_group_TrailingComma) {
     auto p = parser("group(4,)");
     auto attr = p->attribute();
@@ -513,17 +563,17 @@ TEST_F(ParserImplTest, Attribute_Group_MissingValue) {
     EXPECT_TRUE(attr.errored);
     EXPECT_EQ(attr.value, nullptr);
     EXPECT_TRUE(p->has_error());
-    EXPECT_EQ(p->error(), "1:7: expected signed integer literal for group attribute");
+    EXPECT_EQ(p->error(), "1:7: expected group expression");
 }
 
 TEST_F(ParserImplTest, Attribute_Group_MissingInvalid) {
-    auto p = parser("group(nan)");
+    auto p = parser("group(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:7: expected signed integer literal for group attribute");
+    EXPECT_EQ(p->error(), "1:7: expected group expression");
 }
 
 }  // namespace

src/tint/resolver/attribute_validation_test.cc

@@ -1562,6 +1562,83 @@ TEST_F(InterpolateTest, MissingLocationAttribute_Struct) {
               R"(12:34 error: interpolate attribute must only be used with @location)");
 }
 
+using GroupAndBindingTest = ResolverTest;
+
+TEST_F(GroupAndBindingTest, Const_I32) {
+    GlobalConst("b", Expr(4_i));
+    GlobalConst("g", Expr(2_i));
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding("b"),
+              Group("g"));
+
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(GroupAndBindingTest, Const_U32) {
+    GlobalConst("b", Expr(4_u));
+    GlobalConst("g", Expr(2_u));
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding("b"),
+              Group("g"));
+
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(GroupAndBindingTest, Const_AInt) {
+    GlobalConst("b", Expr(4_a));
+    GlobalConst("g", Expr(2_a));
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding("b"),
+              Group("g"));
+
+    EXPECT_TRUE(r()->Resolve()) << r()->error();
+}
+
+TEST_F(GroupAndBindingTest, Binding_Negative) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()),
+              Binding(Source{{12, 34}}, -2_i), Group(1_i));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' value must be non-negative)");
+}
+
+TEST_F(GroupAndBindingTest, Binding_F32) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()),
+              Binding(Source{{12, 34}}, 2.0_f), Group(1_u));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)");
+}
+
+TEST_F(GroupAndBindingTest, Binding_AFloat) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()),
+              Binding(Source{{12, 34}}, 2.0_a), Group(1_u));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'binding' must be an i32 or u32 value)");
+}
+
+TEST_F(GroupAndBindingTest, Group_Negative) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(2_u),
+              Group(Source{{12, 34}}, -1_i));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' value must be non-negative)");
+}
+
+TEST_F(GroupAndBindingTest, Group_F32) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(2_u),
+              Group(Source{{12, 34}}, 1.0_f));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)");
+}
+
+TEST_F(GroupAndBindingTest, Group_AFloat) {
+    GlobalVar("val", ty.sampled_texture(ast::TextureDimension::k2d, ty.f32()), Binding(2_u),
+              Group(Source{{12, 34}}, 1.0_a));
+
+    EXPECT_FALSE(r()->Resolve());
+    EXPECT_EQ(r()->error(), R"(12:34 error: 'group' must be an i32 or u32 value)");
+}
+
 }  // namespace
 }  // namespace InterpolateTests
 

src/tint/resolver/dependency_graph.cc

@@ -198,6 +198,7 @@ class DependencyScanner {
             [&](const ast::Variable* var) {
                 Declare(var->symbol, var);
                 TraverseType(var->type);
+                TraverseAttributes(var->attributes);
                 if (var->constructor) {
                     TraverseExpression(var->constructor);
                 }
@@ -416,25 +417,38 @@ class DependencyScanner {
     /// Traverses the attribute, performing symbol resolution and determining
     /// global dependencies.
     void TraverseAttribute(const ast::Attribute* attr) {
-        if (auto* wg = attr->As<ast::WorkgroupAttribute>()) {
-            TraverseExpression(wg->x);
-            TraverseExpression(wg->y);
-            TraverseExpression(wg->z);
-            return;
-        }
-        if (auto* align = attr->As<ast::StructMemberAlignAttribute>()) {
-            TraverseExpression(align->expr);
-            return;
-        }
-        if (auto* size = attr->As<ast::StructMemberSizeAttribute>()) {
-            TraverseExpression(size->expr);
+        bool handled = Switch(
+            attr,
+            [&](const ast::BindingAttribute* binding) {
+                TraverseExpression(binding->expr);
+                return true;
+            },
+            [&](const ast::GroupAttribute* group) {
+                TraverseExpression(group->expr);
+                return true;
+            },
+            [&](const ast::StructMemberAlignAttribute* align) {
+                TraverseExpression(align->expr);
+                return true;
+            },
+            [&](const ast::StructMemberSizeAttribute* size) {
+                TraverseExpression(size->expr);
+                return true;
+            },
+            [&](const ast::WorkgroupAttribute* wg) {
+                TraverseExpression(wg->x);
+                TraverseExpression(wg->y);
+                TraverseExpression(wg->z);
+                return true;
+            });
+        if (handled) {
             return;
         }
 
-        if (attr->IsAnyOf<ast::BindingAttribute, ast::BuiltinAttribute, ast::GroupAttribute,
-                          ast::IdAttribute, ast::InternalAttribute, ast::InterpolateAttribute,
-                          ast::InvariantAttribute, ast::LocationAttribute, ast::StageAttribute,
-                          ast::StrideAttribute, ast::StructMemberOffsetAttribute>()) {
+        if (attr->IsAnyOf<ast::BuiltinAttribute, ast::IdAttribute, ast::InternalAttribute,
+                          ast::InterpolateAttribute, ast::InvariantAttribute,
+                          ast::LocationAttribute, ast::StageAttribute, ast::StrideAttribute,
+                          ast::StructMemberOffsetAttribute>()) {
             return;
         }
 

src/tint/resolver/dependency_graph_test.cc

@@ -1289,6 +1289,9 @@ TEST_F(ResolverDependencyGraphTraversalTest, SymbolsReached) {
     GlobalVar(Sym(), ty.storage_texture(ast::TextureDimension::k2d, ast::TexelFormat::kR32Float,
                                         ast::Access::kRead));  //
     GlobalVar(Sym(), ty.sampler(ast::SamplerKind::kSampler));
+
+    GlobalVar(Sym(), ty.i32(), utils::Vector{Binding(V), Group(V)});
+
     Func(Sym(), utils::Empty, ty.void_(), utils::Empty);
 #undef V
 #undef T

src/tint/resolver/resolver.cc

@@ -619,34 +619,50 @@ sem::Variable* Resolver::Var(const ast::Var* var, bool is_global) {
         if (var->HasBindingPoint()) {
             uint32_t binding = 0;
             {
+                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@binding"};
+                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
                 auto* attr = ast::GetAttribute<ast::BindingAttribute>(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 binding can be an expression.
+                if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
+                    AddError("'binding' must be an i32 or u32 value", attr->source);
                     return nullptr;
                 }
-                binding = c->As<uint32_t>();
+
+                auto const_value = materialized->ConstantValue();
+                auto value = const_value->As<AInt>();
+                if (value < 0) {
+                    AddError("'binding' value must be non-negative", attr->source);
+                    return nullptr;
+                }
+                binding = u32(value);
             }
 
             uint32_t group = 0;
             {
+                ExprEvalStageConstraint constraint{sem::EvaluationStage::kConstant, "@group"};
+                TINT_SCOPED_ASSIGNMENT(expr_eval_stage_constraint_, constraint);
+
                 auto* attr = ast::GetAttribute<ast::GroupAttribute>(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 binding can be an expression.
+                if (!materialized->Type()->IsAnyOf<sem::I32, sem::U32>()) {
+                    AddError("'group' must be an i32 or u32 value", attr->source);
                     return nullptr;
                 }
-                group = c->As<uint32_t>();
+
+                auto const_value = materialized->ConstantValue();
+                auto value = const_value->As<AInt>();
+                if (value < 0) {
+                    AddError("'group' value must be non-negative", attr->source);
+                    return nullptr;
+                }
+                group = u32(value);
             }
             binding_point = {group, binding};
         }