From 12b9d9bb56dd01464cbb8e58037429ae5f9f7162 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20M=C3=BCller?= Date: Tue, 28 Jan 2025 13:55:12 +0000 Subject: [PATCH 1/3] [mlir] Make `TypedStrAttr` actually enforce the string type. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tablgen definition `TypedStrAttr` is an attribute constraints that is meant to restrict the type of a `StringAttr` to the type given as parameter. However, the definition did not previously restrict the type; any `StringAttr` was accepted. This PR makes the definition actually enforce the type. To test the constraints, the PR also changes the test op that was previously used to test this constraint such that the enforced type is `AnyInteger` instead of `AnyType`. The latter allowed any type, so not enforcing that constraint had no observable effect. The PR then adds a test case with a wrong type and ensures that diagnostics are produced. Signed-off-by: Ingo Müller --- mlir/include/mlir/IR/CommonAttrConstraints.td | 6 ++++-- mlir/test/IR/attribute.mlir | 16 ++++++++++++---- mlir/test/lib/Dialect/Test/TestOps.td | 2 +- 3 files changed, 17 insertions(+), 7 deletions(-) diff --git a/mlir/include/mlir/IR/CommonAttrConstraints.td b/mlir/include/mlir/IR/CommonAttrConstraints.td index 17ca82c510f8a..4cd6c84db61df 100644 --- a/mlir/include/mlir/IR/CommonAttrConstraints.td +++ b/mlir/include/mlir/IR/CommonAttrConstraints.td @@ -343,8 +343,10 @@ def SymbolNameAttr : StringBasedAttr($_se // String attribute that has a specific value type. class TypedStrAttr - : StringBasedAttr($_self)">, - "string attribute"> { + : StringBasedAttr< + SubstLeaves<"$_self", "::mlir::cast($_self).getType()", + ty.predicate>, + "string attribute of " # ty.summary> { let valueType = ty; } diff --git a/mlir/test/IR/attribute.mlir b/mlir/test/IR/attribute.mlir index 0085d64ae82b6..048dd06d71096 100644 --- a/mlir/test/IR/attribute.mlir +++ b/mlir/test/IR/attribute.mlir @@ -416,10 +416,18 @@ func.func @non_type_in_type_array_attr_fail() { // Test StringAttr with custom type //===----------------------------------------------------------------------===// -// CHECK-LABEL: func @string_attr_custom_type -func.func @string_attr_custom_type() { - // CHECK: "string_data" : !foo.string - test.string_attr_with_type "string_data" : !foo.string +// CHECK-LABEL: func @string_attr_custom_type_valid +func.func @string_attr_custom_type_valid() { + // CHECK: "string_data" : i64 + test.string_attr_with_type "string_data" : i64 + return +} + +// ----- + +func.func @string_attr_custom_type_invalid() { + // expected-error @+1 {{'attr' failed to satisfy constraint: string attribute of integer}} + test.string_attr_with_type "string_data" : f32 return } diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td index f37573c1351ce..84b7f34d37d0f 100644 --- a/mlir/test/lib/Dialect/Test/TestOps.td +++ b/mlir/test/lib/Dialect/Test/TestOps.td @@ -194,7 +194,7 @@ def TypeArrayAttrWithDefaultOp : TEST_Op<"type_array_attr_with_default"> { } def TypeStringAttrWithTypeOp : TEST_Op<"string_attr_with_type"> { - let arguments = (ins TypedStrAttr:$attr); + let arguments = (ins TypedStrAttr:$attr); let assemblyFormat = "$attr attr-dict"; } From 902cc533aacac30fddd5264c29ff1a68b5736d89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20M=C3=BCller?= Date: Wed, 29 Jan 2025 10:49:18 +0000 Subject: [PATCH 2/3] Fix possible crash by testing for `isa --- mlir/include/mlir/IR/CommonAttrConstraints.td | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/mlir/include/mlir/IR/CommonAttrConstraints.td b/mlir/include/mlir/IR/CommonAttrConstraints.td index 4cd6c84db61df..599f5ecba5803 100644 --- a/mlir/include/mlir/IR/CommonAttrConstraints.td +++ b/mlir/include/mlir/IR/CommonAttrConstraints.td @@ -334,18 +334,18 @@ class StringBasedAttr : Attr { let valueType = NoneType; } -def StrAttr : StringBasedAttr($_self)">, - "string attribute">; +def StrAttrPred : CPred<"::llvm::isa<::mlir::StringAttr>($_self)">; + +def StrAttr : StringBasedAttr; // A string attribute that represents the name of a symbol. -def SymbolNameAttr : StringBasedAttr($_self)">, - "string attribute">; +def SymbolNameAttr : StringBasedAttr; // String attribute that has a specific value type. class TypedStrAttr - : StringBasedAttr< + : StringBasedAttr($_self).getType()", - ty.predicate>, + ty.predicate>]>, "string attribute of " # ty.summary> { let valueType = ty; } From d6648ffb958d402b43f98c4485f9db01a66c0751 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ingo=20M=C3=BCller?= Date: Wed, 29 Jan 2025 12:06:37 +0000 Subject: [PATCH 3/3] Extend tests that applies new constraint non-`StringAttr` attribute. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Ingo Müller --- mlir/test/IR/attribute.mlir | 11 +++++++++++ mlir/test/lib/Dialect/Test/TestOps.td | 9 ++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/mlir/test/IR/attribute.mlir b/mlir/test/IR/attribute.mlir index 048dd06d71096..5a005a393d8ac 100644 --- a/mlir/test/IR/attribute.mlir +++ b/mlir/test/IR/attribute.mlir @@ -433,6 +433,17 @@ func.func @string_attr_custom_type_invalid() { // ----- +// CHECK-LABEL: func @string_attr_custom_mixed_type +func.func @string_attr_custom_mixed_type() { + // CHECK: "string_data" : i64 + test.string_attr_with_mixed_type "string_data" : i64 + // CHECK: 42 : i64 + test.string_attr_with_mixed_type 42 : i64 + return +} + +// ----- + //===----------------------------------------------------------------------===// // Test I32EnumAttr //===----------------------------------------------------------------------===// diff --git a/mlir/test/lib/Dialect/Test/TestOps.td b/mlir/test/lib/Dialect/Test/TestOps.td index 84b7f34d37d0f..79840094686e1 100644 --- a/mlir/test/lib/Dialect/Test/TestOps.td +++ b/mlir/test/lib/Dialect/Test/TestOps.td @@ -193,11 +193,18 @@ def TypeArrayAttrWithDefaultOp : TEST_Op<"type_array_attr_with_default"> { let arguments = (ins DefaultValuedAttr:$attr); } -def TypeStringAttrWithTypeOp : TEST_Op<"string_attr_with_type"> { +def TypedStringAttrWithTypeOp : TEST_Op<"string_attr_with_type"> { let arguments = (ins TypedStrAttr:$attr); let assemblyFormat = "$attr attr-dict"; } +def TypedStringAttrWithMixedTypeOp : TEST_Op<"string_attr_with_mixed_type"> { + let arguments = (ins + AnyAttrOf<[TypedStrAttr, I64Attr]>:$attr + ); + let assemblyFormat = "$attr attr-dict"; +} + def FloatAttrOp : TEST_Op<"float_attrs"> { // TODO: Clean up the OpBase float type and attribute selectors so they // can express all of the types.