[mlir] Print aliases for recursive types #110346
Merged
Conversation
This file contains 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
We're already keeping track of the alias depth to ensure that aliases are printed before they're referenced. For recursive types, we can additionally track whether an alias has been printed and only reference it if so, to lift the restrictions on not printing aliases inside mutable types.
|
@llvm/pr-subscribers-mlir Author: Shoaib Meenai (smeenai) ChangesWe're already keeping track of the alias depth to ensure that aliases Full diff: https://github.com/llvm/llvm-project/pull/110346.diff 2 Files Affected:
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index d0fd8e420d38ea..7f95f5ace8c00f 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -545,6 +545,10 @@ class SymbolAlias {
bool isType : 1;
/// A flag indicating whether this alias may be deferred or not.
bool isDeferrable : 1;
+
+public:
+ /// Used to avoid printing incomplete aliases for recursive types.
+ bool isPrinted = false;
};
/// This class represents a utility that initializes the set of attribute and
@@ -1222,6 +1226,8 @@ LogicalResult AliasState::getAlias(Type ty, raw_ostream &os) const {
const auto *it = attrTypeToAlias.find(ty.getAsOpaquePointer());
if (it == attrTypeToAlias.end())
return failure();
+ if (!it->second.isPrinted)
+ return failure();
it->second.print(os);
return success();
@@ -1238,12 +1244,9 @@ void AliasState::printAliases(AsmPrinter::Impl &p, NewLineCounter &newLine,
p.getStream() << " = ";
if (alias.isTypeAlias()) {
- // TODO: Support nested aliases in mutable types.
Type type = Type::getFromOpaquePointer(opaqueSymbol);
- if (type.hasTrait<TypeTrait::IsMutable>())
- p.getStream() << type;
- else
- p.printTypeImpl(type);
+ p.printTypeImpl(type);
+ alias.isPrinted = true;
} else {
// TODO: Support nested aliases in mutable attributes.
Attribute attr = Attribute::getFromOpaquePointer(opaqueSymbol);
diff --git a/mlir/test/IR/recursive-type.mlir b/mlir/test/IR/recursive-type.mlir
index 121ba095573baa..42aecb41d998d1 100644
--- a/mlir/test/IR/recursive-type.mlir
+++ b/mlir/test/IR/recursive-type.mlir
@@ -2,7 +2,10 @@
// CHECK: !testrec = !test.test_rec<type_to_alias, test_rec<type_to_alias>>
// CHECK: ![[$NAME:.*]] = !test.test_rec_alias<name, !test.test_rec_alias<name>>
+// CHECK: ![[$NAME5:.*]] = !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5>>>>
// CHECK: ![[$NAME2:.*]] = !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
+// CHECK: ![[$NAME4:.*]] = !test.test_rec_alias<name4, !name5_>
+// CHECK: ![[$NAME3:.*]] = !test.test_rec_alias<name3, !name4_>
// CHECK-LABEL: @roundtrip
func.func @roundtrip() {
@@ -24,6 +27,14 @@ func.func @roundtrip() {
// CHECK: () -> ![[$NAME2]]
"test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
"test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
+
+ // Mutual recursion.
+ // CHECK: () -> ![[$NAME3]]
+ // CHECK: () -> ![[$NAME4]]
+ // CHECK: () -> ![[$NAME5]]
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5, !test.test_rec_alias<name3>>>>
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name4, !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4>>>>
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5>>>>
return
}
|
|
@llvm/pr-subscribers-mlir-core Author: Shoaib Meenai (smeenai) ChangesWe're already keeping track of the alias depth to ensure that aliases Full diff: https://github.com/llvm/llvm-project/pull/110346.diff 2 Files Affected:
diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp
index d0fd8e420d38ea..7f95f5ace8c00f 100644
--- a/mlir/lib/IR/AsmPrinter.cpp
+++ b/mlir/lib/IR/AsmPrinter.cpp
@@ -545,6 +545,10 @@ class SymbolAlias {
bool isType : 1;
/// A flag indicating whether this alias may be deferred or not.
bool isDeferrable : 1;
+
+public:
+ /// Used to avoid printing incomplete aliases for recursive types.
+ bool isPrinted = false;
};
/// This class represents a utility that initializes the set of attribute and
@@ -1222,6 +1226,8 @@ LogicalResult AliasState::getAlias(Type ty, raw_ostream &os) const {
const auto *it = attrTypeToAlias.find(ty.getAsOpaquePointer());
if (it == attrTypeToAlias.end())
return failure();
+ if (!it->second.isPrinted)
+ return failure();
it->second.print(os);
return success();
@@ -1238,12 +1244,9 @@ void AliasState::printAliases(AsmPrinter::Impl &p, NewLineCounter &newLine,
p.getStream() << " = ";
if (alias.isTypeAlias()) {
- // TODO: Support nested aliases in mutable types.
Type type = Type::getFromOpaquePointer(opaqueSymbol);
- if (type.hasTrait<TypeTrait::IsMutable>())
- p.getStream() << type;
- else
- p.printTypeImpl(type);
+ p.printTypeImpl(type);
+ alias.isPrinted = true;
} else {
// TODO: Support nested aliases in mutable attributes.
Attribute attr = Attribute::getFromOpaquePointer(opaqueSymbol);
diff --git a/mlir/test/IR/recursive-type.mlir b/mlir/test/IR/recursive-type.mlir
index 121ba095573baa..42aecb41d998d1 100644
--- a/mlir/test/IR/recursive-type.mlir
+++ b/mlir/test/IR/recursive-type.mlir
@@ -2,7 +2,10 @@
// CHECK: !testrec = !test.test_rec<type_to_alias, test_rec<type_to_alias>>
// CHECK: ![[$NAME:.*]] = !test.test_rec_alias<name, !test.test_rec_alias<name>>
+// CHECK: ![[$NAME5:.*]] = !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5>>>>
// CHECK: ![[$NAME2:.*]] = !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
+// CHECK: ![[$NAME4:.*]] = !test.test_rec_alias<name4, !name5_>
+// CHECK: ![[$NAME3:.*]] = !test.test_rec_alias<name3, !name4_>
// CHECK-LABEL: @roundtrip
func.func @roundtrip() {
@@ -24,6 +27,14 @@ func.func @roundtrip() {
// CHECK: () -> ![[$NAME2]]
"test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
"test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name2, tuple<!test.test_rec_alias<name2>, i32>>
+
+ // Mutual recursion.
+ // CHECK: () -> ![[$NAME3]]
+ // CHECK: () -> ![[$NAME4]]
+ // CHECK: () -> ![[$NAME5]]
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5, !test.test_rec_alias<name3>>>>
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name4, !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4>>>>
+ "test.dummy_op_for_roundtrip"() : () -> !test.test_rec_alias<name5, !test.test_rec_alias<name3, !test.test_rec_alias<name4, !test.test_rec_alias<name5>>>>
return
}
|
smeenai
commented
Sep 28, 2024
|
|
||
| public: | ||
| /// Used to avoid printing incomplete aliases for recursive types. | ||
| bool isPrinted = false; |
There was a problem hiding this comment.
On 64-bit systems, this won't increase the size of SymbolAlias, since there was already padding to align to 8 bytes (because of the StringRef). I'm happy to reduce the size of suffixIndex by 1 bit (29 bits should still be more than enough for it, hopefully) and pack this field into the prior bitfield if it's preferred though.
There was a problem hiding this comment.
Additionally, the bitfield won't be packed on Windows anyway because the MSVC ABI only packs bitfields when all members have the same type, but I'm not sure how much MLIR cares about that.
There was a problem hiding this comment.
Textual parsing/printing isn't considered to be performance-sensitive components.
joker-eph
approved these changes
Sep 28, 2024
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.
We're already keeping track of the alias depth to ensure that aliases
are printed before they're referenced. For recursive types, we can
additionally track whether an alias has been printed and only reference
it if so, to lift the restrictions on not printing aliases inside
mutable types.