[mlir] Make TypedStrAttr actually enforce the string type.

[ingomueller-net]

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.

[llvmbot]

@llvm/pr-subscribers-mlir-core

@llvm/pr-subscribers-mlir

Author: Ingo Müller (ingomueller-net)

Changes

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.

---

Full diff: https://github.com/llvm/llvm-project/pull/124770.diff

3 Files Affected:

• (modified) mlir/include/mlir/IR/CommonAttrConstraints.td (+4-2)
• (modified) mlir/test/IR/attribute.mlir (+12-4)
• (modified) mlir/test/lib/Dialect/Test/TestOps.td (+1-1)

diff --git a/mlir/include/mlir/IR/CommonAttrConstraints.td b/mlir/include/mlir/IR/CommonAttrConstraints.td
index 17ca82c510f8a2..4cd6c84db61df8 100644
--- a/mlir/include/mlir/IR/CommonAttrConstraints.td
+++ b/mlir/include/mlir/IR/CommonAttrConstraints.td
@@ -343,8 +343,10 @@ def SymbolNameAttr : StringBasedAttr<CPred<"::llvm::isa<::mlir::StringAttr>($_se
 
 // String attribute that has a specific value type.
 class TypedStrAttr<Type ty>
-    : StringBasedAttr<CPred<"::llvm::isa<::mlir::StringAttr>($_self)">,
-                            "string attribute"> {
+    : StringBasedAttr<
+        SubstLeaves<"$_self", "::mlir::cast<StringAttr>($_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 0085d64ae82b6b..048dd06d71096f 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 f37573c1351cec..84b7f34d37d0f0 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<AnyType>:$attr);
+  let arguments = (ins TypedStrAttr<AnyInteger>:$attr);
   let assemblyFormat = "$attr attr-dict";
 }
 

[ingomueller-net]

Can we assume at this point that the cast will succeed or should I test that $_self is StringAttr as well?

[matthias-springer]

I suspect that the order of the checks here matters. I.e., it should be safe to assume that it is a string attr. But to be sure, you could try to add another test case for test.string_attr_with_type in generic op syntax, where the attr is not a string attr.

[ingomueller-net]

I had, since opening the PR, encountered a crash with the first version, which didn't test for StringAttr. The second commit fixed that but didn't add a test. A just pushed a third commit that adds a test that crashes without the fix from the second commit (but passes with the fix). Thanks for suggesting that!

[matthias-springer]

I suspect that the order of the checks here matters. I.e., it should be safe to assume that it is a string attr. But to be sure, you could try to add another test case for test.string_attr_with_type in generic op syntax, where the attr is not a string attr.