[Mlir-commits] [mlir] [MLIR][TableGen] Warn on APInt parameter without custom comparator (PR #135970)

Robert Konicar llvmlistbot at llvm.org
Tue Apr 22 01:00:13 PDT 2025 

https://github.com/Jezurko updated https://github.com/llvm/llvm-project/pull/135970

>From b0d36fb79aa9663567415d7e5f4c287689be76a6 Mon Sep 17 00:00:00 2001
From: Robert Konicar <rkonicar at mail.muni.cz>
Date: Wed, 16 Apr 2025 15:20:55 +0200
Subject: [PATCH 1/4] [MLIR][TableGen] Warn on APInt parameter without custom
 comparator

---
 .../mlir/Dialect/SMT/IR/SMTAttributes.td      | 12 +------
 mlir/include/mlir/IR/BuiltinAttributes.td     |  2 +-
 mlir/include/mlir/TableGen/AttrOrTypeDef.h    |  6 +++-
 mlir/lib/Dialect/SMT/IR/SMTAttributes.cpp     | 36 -------------------
 mlir/lib/TableGen/AttrOrTypeDef.cpp           |  4 +++
 mlir/test/mlir-tblgen/apint-param-warn.td     | 17 +++++++++
 mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp   | 13 ++++++-
 7 files changed, 40 insertions(+), 50 deletions(-)
 create mode 100644 mlir/test/mlir-tblgen/apint-param-warn.td

diff --git a/mlir/include/mlir/Dialect/SMT/IR/SMTAttributes.td b/mlir/include/mlir/Dialect/SMT/IR/SMTAttributes.td
index 0909c9abad951..c1c5bfc76e055 100644
--- a/mlir/include/mlir/Dialect/SMT/IR/SMTAttributes.td
+++ b/mlir/include/mlir/Dialect/SMT/IR/SMTAttributes.td
@@ -45,21 +45,11 @@ def BitVectorAttr : AttrDef<SMTDialect, "BitVector", [
     present).
   }];
 
-  let parameters = (ins "llvm::APInt":$value);
+  let parameters = (ins APIntParameter<"">:$value);
 
   let hasCustomAssemblyFormat = true;
   let genVerifyDecl = true;
 
-  // We need to manually define the storage class because the generated one is
-  // buggy (because the APInt asserts matching bitwidth in the `==` operator and
-  // the generated storage uses that directly.
-  // Alternatively: add a type parameter to redundantly store the bitwidth of
-  // of the attribute type, it it's in the order before the 'value' it will be
-  // checked before the APInt equality (this is the reason it works for the
-  // builtin integer attribute), but would be more fragile (and we'd store
-  // duplicate data).
-  let genStorageClass = false;
-
   let builders = [
     AttrBuilder<(ins "llvm::StringRef":$value)>,
     AttrBuilder<(ins "uint64_t":$value, "unsigned":$width)>,
diff --git a/mlir/include/mlir/IR/BuiltinAttributes.td b/mlir/include/mlir/IR/BuiltinAttributes.td
index 6826d1a437775..50dcb8de1f7e7 100644
--- a/mlir/include/mlir/IR/BuiltinAttributes.td
+++ b/mlir/include/mlir/IR/BuiltinAttributes.td
@@ -700,7 +700,7 @@ def Builtin_IntegerAttr : Builtin_Attr<"Integer", "integer",
     false // A bool, i.e. i1, value.
     ```
   }];
-  let parameters = (ins AttributeSelfTypeParameter<"">:$type, "APInt":$value);
+  let parameters = (ins AttributeSelfTypeParameter<"">:$type, APIntParameter<"">:$value);
   let builders = [
     AttrBuilderWithInferredContext<(ins "Type":$type,
                                         "const APInt &":$value), [{
diff --git a/mlir/include/mlir/TableGen/AttrOrTypeDef.h b/mlir/include/mlir/TableGen/AttrOrTypeDef.h
index c3d730e42ef70..8c1d399b39e0b 100644
--- a/mlir/include/mlir/TableGen/AttrOrTypeDef.h
+++ b/mlir/include/mlir/TableGen/AttrOrTypeDef.h
@@ -68,7 +68,11 @@ class AttrOrTypeParameter {
   /// If specified, get the custom allocator code for this parameter.
   std::optional<StringRef> getAllocator() const;
 
-  /// If specified, get the custom comparator code for this parameter.
+  /// Return true if user defined comparator is specified.
+  bool hasCustomComparator() const;
+
+  /// Get the custom comparator code for this parameter or fallback to the
+  /// default.
   StringRef getComparator() const;
 
   /// Get the C++ type of this parameter.
diff --git a/mlir/lib/Dialect/SMT/IR/SMTAttributes.cpp b/mlir/lib/Dialect/SMT/IR/SMTAttributes.cpp
index c28f3558a02d2..3f40d6a42eafd 100644
--- a/mlir/lib/Dialect/SMT/IR/SMTAttributes.cpp
+++ b/mlir/lib/Dialect/SMT/IR/SMTAttributes.cpp
@@ -21,42 +21,6 @@ using namespace mlir::smt;
 // BitVectorAttr
 //===----------------------------------------------------------------------===//
 
-namespace mlir {
-namespace smt {
-namespace detail {
-struct BitVectorAttrStorage : public mlir::AttributeStorage {
-  using KeyTy = APInt;
-  BitVectorAttrStorage(APInt value) : value(std::move(value)) {}
-
-  KeyTy getAsKey() const { return value; }
-
-  // NOTE: the implementation of this operator is the reason we need to define
-  // the storage manually. The auto-generated version would just do the direct
-  // equality check of the APInt, but that asserts the bitwidth of both to be
-  // the same, leading to a crash. This implementation, therefore, checks for
-  // matching bit-width beforehand.
-  bool operator==(const KeyTy &key) const {
-    return (value.getBitWidth() == key.getBitWidth() && value == key);
-  }
-
-  static llvm::hash_code hashKey(const KeyTy &key) {
-    return llvm::hash_value(key);
-  }
-
-  static BitVectorAttrStorage *
-  construct(mlir::AttributeStorageAllocator &allocator, KeyTy &&key) {
-    return new (allocator.allocate<BitVectorAttrStorage>())
-        BitVectorAttrStorage(std::move(key));
-  }
-
-  APInt value;
-};
-} // namespace detail
-} // namespace smt
-} // namespace mlir
-
-APInt BitVectorAttr::getValue() const { return getImpl()->value; }
-
 LogicalResult BitVectorAttr::verify(
     function_ref<InFlightDiagnostic()> emitError,
     APInt value) { // NOLINT(performance-unnecessary-value-param)
diff --git a/mlir/lib/TableGen/AttrOrTypeDef.cpp b/mlir/lib/TableGen/AttrOrTypeDef.cpp
index 9e8f789d71b5e..ccb0a6c6c261e 100644
--- a/mlir/lib/TableGen/AttrOrTypeDef.cpp
+++ b/mlir/lib/TableGen/AttrOrTypeDef.cpp
@@ -278,6 +278,10 @@ std::optional<StringRef> AttrOrTypeParameter::getAllocator() const {
   return getDefValue<StringInit>("allocator");
 }
 
+bool AttrOrTypeParameter::hasCustomComparator() const {
+  return getDefValue<StringInit>("comparator").has_value();
+}
+
 StringRef AttrOrTypeParameter::getComparator() const {
   return getDefValue<StringInit>("comparator").value_or("$_lhs == $_rhs");
 }
diff --git a/mlir/test/mlir-tblgen/apint-param-warn.td b/mlir/test/mlir-tblgen/apint-param-warn.td
new file mode 100644
index 0000000000000..ecfa97e5b5355
--- /dev/null
+++ b/mlir/test/mlir-tblgen/apint-param-warn.td
@@ -0,0 +1,17 @@
+// RUN: mlir-tblgen -gen-attrdef-decls -I %S/../../include %s 2>&1 | FileCheck %s
+
+include "mlir/IR/AttrTypeBase.td"
+include "mlir/IR/OpBase.td"
+
+def Test_Dialect: Dialect {
+  let name = "TestDialect";
+  let cppNamespace = "::test";
+}
+
+def RawAPIntAttr : AttrDef<Test_Dialect, "RawAPInt"> {
+  let mnemonic = "raw_ap_int";
+  let parameters = (ins "APInt":$value);
+  let hasCustomAssemblyFormat = 1;
+}
+
+// CHECK: apint-param-warn.td:11:5: warning: Using a raw APInt parameter
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index cf0d827942949..4b0a72ec60a0b 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -678,8 +678,19 @@ void DefGen::emitStorageClass() {
   emitConstruct();
   // Emit the storage class members as public, at the very end of the struct.
   storageCls->finalize();
-  for (auto &param : params)
+  for (auto &param : params) {
+     if (param.getCppType().contains("APInt") &&
+         !param.hasCustomComparator()) {
+       PrintWarning(
+           def.getLoc(),
+           "Using a raw APInt parameter without a custom comparator is "
+           "discouraged because an assert in the equality operator is "
+           "triggered when the two APInts have different bit widths. This can "
+           "lead to unexpected crashes. Consider using an `APIntParameter` or "
+           "providing a custom comparator.");
+     }
     storageCls->declare<Field>(param.getCppType(), param.getName());
+  }
 }
 
 //===----------------------------------------------------------------------===//

>From 5992ca8420bdd135de158247bd4cfa0ab24f52df Mon Sep 17 00:00:00 2001
From: Robert Konicar <rkonicar at mail.muni.cz>
Date: Wed, 16 Apr 2025 16:37:11 +0200
Subject: [PATCH 2/4] fixup! [MLIR][TableGen] Warn on APInt parameter without
 custom comparator

---
 mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index 4b0a72ec60a0b..c34cbfe47dc35 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -679,16 +679,15 @@ void DefGen::emitStorageClass() {
   // Emit the storage class members as public, at the very end of the struct.
   storageCls->finalize();
   for (auto &param : params) {
-     if (param.getCppType().contains("APInt") &&
-         !param.hasCustomComparator()) {
-       PrintWarning(
-           def.getLoc(),
-           "Using a raw APInt parameter without a custom comparator is "
-           "discouraged because an assert in the equality operator is "
-           "triggered when the two APInts have different bit widths. This can "
-           "lead to unexpected crashes. Consider using an `APIntParameter` or "
-           "providing a custom comparator.");
-     }
+    if (param.getCppType().contains("APInt") && !param.hasCustomComparator()) {
+      PrintWarning(
+          def.getLoc(),
+          "Using a raw APInt parameter without a custom comparator is "
+          "discouraged because an assert in the equality operator is "
+          "triggered when the two APInts have different bit widths. This can "
+          "lead to unexpected crashes. Consider using an `APIntParameter` or "
+          "providing a custom comparator.");
+    }
     storageCls->declare<Field>(param.getCppType(), param.getName());
   }
 }

>From 38b3e8bf140c2998a711eaa794eed46f5d78a07b Mon Sep 17 00:00:00 2001
From: Robert Konicar <robert.konicar at trailofbits.com>
Date: Thu, 17 Apr 2025 10:23:53 +0200
Subject: [PATCH 3/4] fixup!  [MLIR][TableGen] Warn on APInt parameter without
 custom comparator

Co-authored-by: Tobias Gysi <tobias.gysi at nextsilicon.com>
---
 mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index c34cbfe47dc35..1b314a149c418 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -683,10 +683,10 @@ void DefGen::emitStorageClass() {
       PrintWarning(
           def.getLoc(),
           "Using a raw APInt parameter without a custom comparator is "
-          "discouraged because an assert in the equality operator is "
+          "not supported because an assert in the equality operator is "
           "triggered when the two APInts have different bit widths. This can "
-          "lead to unexpected crashes. Consider using an `APIntParameter` or "
-          "providing a custom comparator.");
+          "lead to unexpected crashes. Use an `APIntParameter` or "
+          "provide a custom comparator.");
     }
     storageCls->declare<Field>(param.getCppType(), param.getName());
   }

>From a5cb62bc0a50b8f01d94b6b36215b36865731af4 Mon Sep 17 00:00:00 2001
From: Robert Konicar <rkonicar at mail.muni.cz>
Date: Tue, 22 Apr 2025 09:59:49 +0200
Subject: [PATCH 4/4] fixup! fixup!  [MLIR][TableGen] Warn on APInt parameter
 without custom comparator

---
 .../mlir-tblgen/{apint-param-warn.td => apint-param-error.td} | 4 ++--
 mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp                   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename mlir/test/mlir-tblgen/{apint-param-warn.td => apint-param-error.td} (66%)

diff --git a/mlir/test/mlir-tblgen/apint-param-warn.td b/mlir/test/mlir-tblgen/apint-param-error.td
similarity index 66%
rename from mlir/test/mlir-tblgen/apint-param-warn.td
rename to mlir/test/mlir-tblgen/apint-param-error.td
index ecfa97e5b5355..602180790bbff 100644
--- a/mlir/test/mlir-tblgen/apint-param-warn.td
+++ b/mlir/test/mlir-tblgen/apint-param-error.td
@@ -1,4 +1,4 @@
-// RUN: mlir-tblgen -gen-attrdef-decls -I %S/../../include %s 2>&1 | FileCheck %s
+// RUN: not mlir-tblgen -gen-attrdef-decls -I %S/../../include %s 2>&1 | FileCheck %s
 
 include "mlir/IR/AttrTypeBase.td"
 include "mlir/IR/OpBase.td"
@@ -14,4 +14,4 @@ def RawAPIntAttr : AttrDef<Test_Dialect, "RawAPInt"> {
   let hasCustomAssemblyFormat = 1;
 }
 
-// CHECK: apint-param-warn.td:11:5: warning: Using a raw APInt parameter
+// CHECK: apint-param-error.td:11:5: error: Using a raw APInt parameter
diff --git a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
index 1b314a149c418..d45792fc7764a 100644
--- a/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
+++ b/mlir/tools/mlir-tblgen/AttrOrTypeDefGen.cpp
@@ -680,7 +680,7 @@ void DefGen::emitStorageClass() {
   storageCls->finalize();
   for (auto &param : params) {
     if (param.getCppType().contains("APInt") && !param.hasCustomComparator()) {
-      PrintWarning(
+      PrintError(
           def.getLoc(),
           "Using a raw APInt parameter without a custom comparator is "
           "not supported because an assert in the equality operator is "

More information about the Mlir-commits mailing list