[mlir][tblgen] Fix bug when mixing props and InferTypes #157367
Merged
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
|
|
fabianmcg
force-pushed
the
pr-fix-tblgen
branch
from
c7654ab to
7dfa2d4
Compare
|
@llvm/pr-subscribers-mlir Author: Fabian Mora (fabianmcg) ChangesThis patch fixes a bug occurring when properties are mixed with any of the InferType traits, causing tblgen to crash. A simple reproducer is: The issue occurs because of the call: To understand better the issue, consider: In this case, The fix is to change Full diff: https://github.com/llvm/llvm-project/pull/157367.diff 4 Files Affected:
diff --git a/mlir/include/mlir/TableGen/Operator.h b/mlir/include/mlir/TableGen/Operator.h
index 9e570373d9cd3..f0514d8e61748 100644
--- a/mlir/include/mlir/TableGen/Operator.h
+++ b/mlir/include/mlir/TableGen/Operator.h
@@ -323,21 +323,22 @@ class Operator {
/// Requires: all result types are known.
const InferredResultType &getInferredResultType(int index) const;
- /// Pair consisting kind of argument and index into operands or attributes.
- struct OperandOrAttribute {
- enum class Kind { Operand, Attribute };
- OperandOrAttribute(Kind kind, int index) {
- packed = (index << 1) | (kind == Kind::Attribute);
+ /// Pair consisting kind of argument and index into operands, attributes, or
+ /// properties.
+ struct OperandAttrOrProp {
+ enum class Kind { Operand = 0x0, Attribute = 0x1, Property = 0x2 };
+ OperandAttrOrProp(Kind kind, int index) {
+ packed = (index << 2) | static_cast<int>(kind);
}
- int operandOrAttributeIndex() const { return (packed >> 1); }
- Kind kind() { return (packed & 0x1) ? Kind::Attribute : Kind::Operand; }
+ int operandOrAttributeIndex() const { return (packed >> 2); }
+ Kind kind() const { return static_cast<Kind>(packed & 0x3); }
private:
int packed;
};
- /// Returns the OperandOrAttribute corresponding to the index.
- OperandOrAttribute getArgToOperandOrAttribute(int index) const;
+ /// Returns the OperandAttrOrProp corresponding to the index.
+ OperandAttrOrProp getArgToOperandAttrOrProp(int index) const;
/// Returns the builders of this operation.
ArrayRef<Builder> getBuilders() const { return builders; }
@@ -405,8 +406,8 @@ class Operator {
/// The argument with the same type as the result.
SmallVector<InferredResultType> resultTypeMapping;
- /// Map from argument to attribute or operand number.
- SmallVector<OperandOrAttribute, 4> attrOrOperandMapping;
+ /// Map from argument to attribute, property, or operand number.
+ SmallVector<OperandAttrOrProp, 4> attrPropOrOperandMapping;
/// The builders of this operator.
SmallVector<Builder> builders;
diff --git a/mlir/lib/TableGen/Operator.cpp b/mlir/lib/TableGen/Operator.cpp
index da86b00ebc300..926ffd0e363a0 100644
--- a/mlir/lib/TableGen/Operator.cpp
+++ b/mlir/lib/TableGen/Operator.cpp
@@ -385,7 +385,8 @@ void Operator::populateTypeInferenceInfo(
if (getTrait("::mlir::OpTrait::SameOperandsAndResultType")) {
// Check for a non-variable length operand to use as the type anchor.
auto *operandI = llvm::find_if(arguments, [](const Argument &arg) {
- NamedTypeConstraint *operand = llvm::dyn_cast_if_present<NamedTypeConstraint *>(arg);
+ NamedTypeConstraint *operand =
+ llvm::dyn_cast_if_present<NamedTypeConstraint *>(arg);
return operand && !operand->isVariableLength();
});
if (operandI == arguments.end())
@@ -663,15 +664,17 @@ void Operator::populateOpStructure() {
argDef = argDef->getValueAsDef("constraint");
if (argDef->isSubClassOf(typeConstraintClass)) {
- attrOrOperandMapping.push_back(
- {OperandOrAttribute::Kind::Operand, operandIndex});
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Operand, operandIndex});
arguments.emplace_back(&operands[operandIndex++]);
} else if (argDef->isSubClassOf(attrClass)) {
- attrOrOperandMapping.push_back(
- {OperandOrAttribute::Kind::Attribute, attrIndex});
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Attribute, attrIndex});
arguments.emplace_back(&attributes[attrIndex++]);
} else {
assert(argDef->isSubClassOf(propertyClass));
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Property, propIndex});
arguments.emplace_back(&properties[propIndex++]);
}
}
@@ -867,9 +870,8 @@ auto Operator::VariableDecoratorIterator::unwrap(const Init *init)
return VariableDecorator(cast<DefInit>(init)->getDef());
}
-auto Operator::getArgToOperandOrAttribute(int index) const
- -> OperandOrAttribute {
- return attrOrOperandMapping[index];
+auto Operator::getArgToOperandAttrOrProp(int index) const -> OperandAttrOrProp {
+ return attrPropOrOperandMapping[index];
}
std::string Operator::getGetterName(StringRef name) const {
diff --git a/mlir/test/mlir-tblgen/op-decl-and-defs.td b/mlir/test/mlir-tblgen/op-decl-and-defs.td
index f213f50ae2f39..87b41f9dea995 100644
--- a/mlir/test/mlir-tblgen/op-decl-and-defs.td
+++ b/mlir/test/mlir-tblgen/op-decl-and-defs.td
@@ -543,3 +543,12 @@ def _BOp : NS_Op<"_op_with_leading_underscore_and_no_namespace", []>;
// REDUCE_EXC-NOT: NS::AOp declarations
// REDUCE_EXC-LABEL: NS::BOp declarations
+
+// CHECK-LABEL: _TypeInferredPropOp declarations
+def _TypeInferredPropOp : NS_Op<"type_inferred_prop_op_with_properties", [
+ AllTypesMatch<["value", "result"]>
+ ]> {
+ let arguments = (ins Property<"unsigned">:$prop, AnyType:$value);
+ let results = (outs AnyType:$result);
+ let hasCustomAssemblyFormat = 1;
+}
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 8ea4eb7b3eeca..270522380eb59 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3849,9 +3849,9 @@ void OpEmitter::genTypeInterfaceMethods() {
const InferredResultType &infer = op.getInferredResultType(i);
if (!infer.isArg())
continue;
- Operator::OperandOrAttribute arg =
- op.getArgToOperandOrAttribute(infer.getIndex());
- if (arg.kind() == Operator::OperandOrAttribute::Kind::Operand) {
+ Operator::OperandAttrOrProp arg =
+ op.getArgToOperandAttrOrProp(infer.getIndex());
+ if (arg.kind() == Operator::OperandAttrOrProp::Kind::Operand) {
maxAccessedIndex =
std::max(maxAccessedIndex, arg.operandOrAttributeIndex());
}
@@ -3877,17 +3877,16 @@ void OpEmitter::genTypeInterfaceMethods() {
if (infer.isArg()) {
// If this is an operand, just index into operand list to access the
// type.
- Operator::OperandOrAttribute arg =
- op.getArgToOperandOrAttribute(infer.getIndex());
- if (arg.kind() == Operator::OperandOrAttribute::Kind::Operand) {
+ Operator::OperandAttrOrProp arg =
+ op.getArgToOperandAttrOrProp(infer.getIndex());
+ if (arg.kind() == Operator::OperandAttrOrProp::Kind::Operand) {
typeStr = ("operands[" + Twine(arg.operandOrAttributeIndex()) +
"].getType()")
.str();
// If this is an attribute, index into the attribute dictionary.
- } else {
- auto *attr =
- cast<NamedAttribute *>(op.getArg(arg.operandOrAttributeIndex()));
+ } else if (auto *attr = dyn_cast<NamedAttribute *>(
+ op.getArg(arg.operandOrAttributeIndex()))) {
body << " ::mlir::TypedAttr odsInferredTypeAttr" << inferredTypeIdx
<< " = ";
if (op.getDialect().usePropertiesForAttributes()) {
@@ -3907,6 +3906,8 @@ void OpEmitter::genTypeInterfaceMethods() {
typeStr =
("odsInferredTypeAttr" + Twine(inferredTypeIdx) + ".getType()")
.str();
+ } else {
+ llvm_unreachable("Properties cannot be used for type inference");
}
} else if (std::optional<StringRef> builder =
op.getResult(infer.getResultIndex())
|
|
@llvm/pr-subscribers-mlir-core Author: Fabian Mora (fabianmcg) ChangesThis patch fixes a bug occurring when properties are mixed with any of the InferType traits, causing tblgen to crash. A simple reproducer is: The issue occurs because of the call: To understand better the issue, consider: In this case, The fix is to change Full diff: https://github.com/llvm/llvm-project/pull/157367.diff 4 Files Affected:
diff --git a/mlir/include/mlir/TableGen/Operator.h b/mlir/include/mlir/TableGen/Operator.h
index 9e570373d9cd3..f0514d8e61748 100644
--- a/mlir/include/mlir/TableGen/Operator.h
+++ b/mlir/include/mlir/TableGen/Operator.h
@@ -323,21 +323,22 @@ class Operator {
/// Requires: all result types are known.
const InferredResultType &getInferredResultType(int index) const;
- /// Pair consisting kind of argument and index into operands or attributes.
- struct OperandOrAttribute {
- enum class Kind { Operand, Attribute };
- OperandOrAttribute(Kind kind, int index) {
- packed = (index << 1) | (kind == Kind::Attribute);
+ /// Pair consisting kind of argument and index into operands, attributes, or
+ /// properties.
+ struct OperandAttrOrProp {
+ enum class Kind { Operand = 0x0, Attribute = 0x1, Property = 0x2 };
+ OperandAttrOrProp(Kind kind, int index) {
+ packed = (index << 2) | static_cast<int>(kind);
}
- int operandOrAttributeIndex() const { return (packed >> 1); }
- Kind kind() { return (packed & 0x1) ? Kind::Attribute : Kind::Operand; }
+ int operandOrAttributeIndex() const { return (packed >> 2); }
+ Kind kind() const { return static_cast<Kind>(packed & 0x3); }
private:
int packed;
};
- /// Returns the OperandOrAttribute corresponding to the index.
- OperandOrAttribute getArgToOperandOrAttribute(int index) const;
+ /// Returns the OperandAttrOrProp corresponding to the index.
+ OperandAttrOrProp getArgToOperandAttrOrProp(int index) const;
/// Returns the builders of this operation.
ArrayRef<Builder> getBuilders() const { return builders; }
@@ -405,8 +406,8 @@ class Operator {
/// The argument with the same type as the result.
SmallVector<InferredResultType> resultTypeMapping;
- /// Map from argument to attribute or operand number.
- SmallVector<OperandOrAttribute, 4> attrOrOperandMapping;
+ /// Map from argument to attribute, property, or operand number.
+ SmallVector<OperandAttrOrProp, 4> attrPropOrOperandMapping;
/// The builders of this operator.
SmallVector<Builder> builders;
diff --git a/mlir/lib/TableGen/Operator.cpp b/mlir/lib/TableGen/Operator.cpp
index da86b00ebc300..926ffd0e363a0 100644
--- a/mlir/lib/TableGen/Operator.cpp
+++ b/mlir/lib/TableGen/Operator.cpp
@@ -385,7 +385,8 @@ void Operator::populateTypeInferenceInfo(
if (getTrait("::mlir::OpTrait::SameOperandsAndResultType")) {
// Check for a non-variable length operand to use as the type anchor.
auto *operandI = llvm::find_if(arguments, [](const Argument &arg) {
- NamedTypeConstraint *operand = llvm::dyn_cast_if_present<NamedTypeConstraint *>(arg);
+ NamedTypeConstraint *operand =
+ llvm::dyn_cast_if_present<NamedTypeConstraint *>(arg);
return operand && !operand->isVariableLength();
});
if (operandI == arguments.end())
@@ -663,15 +664,17 @@ void Operator::populateOpStructure() {
argDef = argDef->getValueAsDef("constraint");
if (argDef->isSubClassOf(typeConstraintClass)) {
- attrOrOperandMapping.push_back(
- {OperandOrAttribute::Kind::Operand, operandIndex});
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Operand, operandIndex});
arguments.emplace_back(&operands[operandIndex++]);
} else if (argDef->isSubClassOf(attrClass)) {
- attrOrOperandMapping.push_back(
- {OperandOrAttribute::Kind::Attribute, attrIndex});
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Attribute, attrIndex});
arguments.emplace_back(&attributes[attrIndex++]);
} else {
assert(argDef->isSubClassOf(propertyClass));
+ attrPropOrOperandMapping.push_back(
+ {OperandAttrOrProp::Kind::Property, propIndex});
arguments.emplace_back(&properties[propIndex++]);
}
}
@@ -867,9 +870,8 @@ auto Operator::VariableDecoratorIterator::unwrap(const Init *init)
return VariableDecorator(cast<DefInit>(init)->getDef());
}
-auto Operator::getArgToOperandOrAttribute(int index) const
- -> OperandOrAttribute {
- return attrOrOperandMapping[index];
+auto Operator::getArgToOperandAttrOrProp(int index) const -> OperandAttrOrProp {
+ return attrPropOrOperandMapping[index];
}
std::string Operator::getGetterName(StringRef name) const {
diff --git a/mlir/test/mlir-tblgen/op-decl-and-defs.td b/mlir/test/mlir-tblgen/op-decl-and-defs.td
index f213f50ae2f39..87b41f9dea995 100644
--- a/mlir/test/mlir-tblgen/op-decl-and-defs.td
+++ b/mlir/test/mlir-tblgen/op-decl-and-defs.td
@@ -543,3 +543,12 @@ def _BOp : NS_Op<"_op_with_leading_underscore_and_no_namespace", []>;
// REDUCE_EXC-NOT: NS::AOp declarations
// REDUCE_EXC-LABEL: NS::BOp declarations
+
+// CHECK-LABEL: _TypeInferredPropOp declarations
+def _TypeInferredPropOp : NS_Op<"type_inferred_prop_op_with_properties", [
+ AllTypesMatch<["value", "result"]>
+ ]> {
+ let arguments = (ins Property<"unsigned">:$prop, AnyType:$value);
+ let results = (outs AnyType:$result);
+ let hasCustomAssemblyFormat = 1;
+}
diff --git a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
index 8ea4eb7b3eeca..270522380eb59 100644
--- a/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
+++ b/mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp
@@ -3849,9 +3849,9 @@ void OpEmitter::genTypeInterfaceMethods() {
const InferredResultType &infer = op.getInferredResultType(i);
if (!infer.isArg())
continue;
- Operator::OperandOrAttribute arg =
- op.getArgToOperandOrAttribute(infer.getIndex());
- if (arg.kind() == Operator::OperandOrAttribute::Kind::Operand) {
+ Operator::OperandAttrOrProp arg =
+ op.getArgToOperandAttrOrProp(infer.getIndex());
+ if (arg.kind() == Operator::OperandAttrOrProp::Kind::Operand) {
maxAccessedIndex =
std::max(maxAccessedIndex, arg.operandOrAttributeIndex());
}
@@ -3877,17 +3877,16 @@ void OpEmitter::genTypeInterfaceMethods() {
if (infer.isArg()) {
// If this is an operand, just index into operand list to access the
// type.
- Operator::OperandOrAttribute arg =
- op.getArgToOperandOrAttribute(infer.getIndex());
- if (arg.kind() == Operator::OperandOrAttribute::Kind::Operand) {
+ Operator::OperandAttrOrProp arg =
+ op.getArgToOperandAttrOrProp(infer.getIndex());
+ if (arg.kind() == Operator::OperandAttrOrProp::Kind::Operand) {
typeStr = ("operands[" + Twine(arg.operandOrAttributeIndex()) +
"].getType()")
.str();
// If this is an attribute, index into the attribute dictionary.
- } else {
- auto *attr =
- cast<NamedAttribute *>(op.getArg(arg.operandOrAttributeIndex()));
+ } else if (auto *attr = dyn_cast<NamedAttribute *>(
+ op.getArg(arg.operandOrAttributeIndex()))) {
body << " ::mlir::TypedAttr odsInferredTypeAttr" << inferredTypeIdx
<< " = ";
if (op.getDialect().usePropertiesForAttributes()) {
@@ -3907,6 +3906,8 @@ void OpEmitter::genTypeInterfaceMethods() {
typeStr =
("odsInferredTypeAttr" + Twine(inferredTypeIdx) + ".getType()")
.str();
+ } else {
+ llvm_unreachable("Properties cannot be used for type inference");
}
} else if (std::optional<StringRef> builder =
op.getResult(infer.getResultIndex())
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a crash in mlir-tblgen when mixing properties with InferType traits. The issue occurred because the argument-to-operand/attribute mapping didn't account for properties, causing index mismatches during type inference.
Key changes:
- Expand the argument mapping to include properties alongside operands and attributes
- Update the enum and data structures to handle three kinds of arguments instead of two
- Add proper error handling for unsupported property-based type inference
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mlir/include/mlir/TableGen/Operator.h | Updates data structures and enums to support properties in argument mapping |
| mlir/lib/TableGen/Operator.cpp | Implements the expanded mapping logic and updates method signatures |
| mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | Updates type inference code to use new mapping and adds error handling |
| mlir/test/mlir-tblgen/op-decl-and-defs.td | Adds test case for the previously crashing scenario |
zero9178
reviewed
Sep 8, 2025
zero9178
approved these changes
Sep 8, 2025
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.
This patch fixes a bug occurring when properties are mixed with any of the InferType traits, causing tblgen to crash. A simple reproducer is:
The issue occurs because of the call:
To understand better the issue, consider:
In this case,
infer.getIndex()will return1forOperand0, butgetArgToOperandOrAttributeexpects0, causing the discrepancy that causes the crash.The fix is to change
attrOrOperandMappingto also include props.