[MLIR][TableGen] Error on APInt parameter without custom comparator #135970
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This PR is a fllow-up on the discussion from #135772 |
|
@llvm/pr-subscribers-mlir-ods @llvm/pr-subscribers-mlir Author: Robert Konicar (Jezurko) ChangesThis warning informs the user about the danger of using an APInt typed parameter with the generated equality operator. If the compared APInts have different bit widths the equality operator triggers an assert. This is dangerous, since Full diff: https://github.com/llvm/llvm-project/pull/135970.diff 7 Files Affected:
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 ¶m : params)
+ for (auto ¶m : 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());
+ }
}
//===----------------------------------------------------------------------===//
|
|
@llvm/pr-subscribers-mlir-core Author: Robert Konicar (Jezurko) ChangesThis warning informs the user about the danger of using an APInt typed parameter with the generated equality operator. If the compared APInts have different bit widths the equality operator triggers an assert. This is dangerous, since Full diff: https://github.com/llvm/llvm-project/pull/135970.diff 7 Files Affected:
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 ¶m : params)
+ for (auto ¶m : 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());
+ }
}
//===----------------------------------------------------------------------===//
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
gysit
approved these changes
Apr 16, 2025
…ator Co-authored-by: Tobias Gysi <tobias.gysi@nextsilicon.com>
|
The change looks reasonable to me, though I would potentially ask what's the cost of just making this an error? Fine with landing this as-is though. |
River707
approved these changes
Apr 17, 2025
|
It could technically break downstream builds that can not experience the issue. For example, even the old version of the Builtin_IntegerAttribute could not trigger the assert, as the type parameter of the attribute contains the bit width information and short-circuited the equality operator. With that being said, I'm not opposed to making it an error. Fixing the issue is trivial. |
True, it sounds like a very doable integration task for downstream projects though? I would thus tend to make it an error since warnings can go unnoticed quite easily. No strong opinion from my side. |
xlauko
approved these changes
Apr 22, 2025
|
LGTM |
|
PR title and commit message probably should be updated before landing. Otherwise, still LGTM. |
Jezurko
changed the title
|
I've changed the error emitting function to I've updated the PR title and message to match the change. If it's okay like this, can someone merge it for me? Thanks! |
raikonenfnu
added a commit
to iree-org/llvm-project
that referenced
this pull request
Apr 23, 2025
…arator (llvm#135970)" This reverts commit 4bcc414.
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 23, 2025
This patch carries revert of llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
raikonenfnu
added a commit
to iree-org/llvm-project
that referenced
this pull request
Apr 24, 2025
…arator (llvm#135970)" This reverts commit 4bcc414.
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 24, 2025
This patch carries revert of llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 24, 2025
LIT change because of llvm/llvm-project#136640 which folds linalg.index who are unit dims. This patch carries revert of llvm/llvm-project#137122. StableHLO and Torch-mlir needs to update their usage of GreedyRewriteConfig to use fluent API. i.e enableRegionSimplification = VS setRegionSimplificationLevel llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 24, 2025
LIT change because of llvm/llvm-project#136640 which folds linalg.index who are unit dims. This patch carries revert of: llvm/llvm-project#137122. StableHLO and Torch-mlir needs to update their usage of GreedyRewriteConfig to use fluent API. i.e enableRegionSimplification = VS setRegionSimplificationLevel llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 25, 2025
This patch carries revert of llvm/llvm-project#136640 because linalg.index folder is segfaulting on inceptionv2 model llvm/llvm-project#133231. This PR breaks fp_to_subbytes and emulation_subbyte_types on llvm-cpu tests. iree-test-deps. llvm/llvm-project#137122. StableHLO and Torch-mlir needs to update their usage of GreedyRewriteConfig to use fluent API. i.e enableRegionSimplification = VS setRegionSimplificationLevel llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
raikonenfnu
added a commit
to iree-org/llvm-project
that referenced
this pull request
Apr 25, 2025
…arator (llvm#135970)" This reverts commit 4bcc414.
raikonenfnu
added a commit
to raikonenfnu/iree
that referenced
this pull request
Apr 25, 2025
This patch carries revert of: llvm/llvm-project#136640 because linalg.index folder is segfaulting on inceptionv2 model llvm/llvm-project#133231. This PR breaks fp_to_subbytes and emulation_subbyte_types on llvm-cpu tests. iree-test-deps. llvm/llvm-project#137122. StableHLO and Torch-mlir needs to update their usage of GreedyRewriteConfig to use fluent API. i.e enableRegionSimplification = VS setRegionSimplificationLevel llvm/llvm-project#135970. StableHLO has issue with VHLO_IntegerAttr and APInt not being updated. StableHLO needs to be updated with that PR's change for us to be able to integrate. llvm/llvm-project#121389. Torch-MLIR needs to be updated with that PR's change for us to be able to integrate. Signed-off-by: Stanley Winata <stanley.winata@amd.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The error is triggered when an attribute or type uses an APInt typed parameter with the generated equality operator. If the compared APInts have different bit widths the equality operator triggers an assert. This is dangerous, since
StorageUniquerfor types and attributes uses the equality operator when a hash collision appears. As such, it is necessary to use custom provided comarator orAPIntParameterthat already has it.This commit also replaces uses of the raw
APIntparameter with theAPIntParameterand removes the no longer necessary custom StorageClass for theBitVectorAttrfrom the SMT dialect that was a workaround for the described issue.