[mlir] notify insertion of parent op first when cloning #73806
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
jeanPerier
requested review from
wsmoses,
joker-eph,
tblah,
River707 and
vzakhari
|
@llvm/pr-subscribers-mlir Author: None (jeanPerier) ChangesWhen cloning an operation with a region, the builder was currently notifying about the insertion of the cloned operations inside the region before the cloned operation itself. When using cloning inside rewrite pass, this could cause issues if a pattern is expected to be applied on a cloned parent operation before trying to apply patterns on the cloned operations it contains (the patterns are attempted in order of notifications for the cloned operations). Full diff: https://github.com/llvm/llvm-project/pull/73806.diff 3 Files Affected:
diff --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index ab20f4863e11c23..7bb7358426af5a4 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -527,6 +527,7 @@ LogicalResult OpBuilder::tryFold(Operation *op,
Operation *OpBuilder::clone(Operation &op, IRMapping &mapper) {
Operation *newOp = op.clone(mapper);
+ newOp = insert(newOp);
// The `insert` call below handles the notification for inserting `newOp`
// itself. But if `newOp` has any regions, we need to notify the listener
// about any ops that got inserted inside those regions as part of cloning.
@@ -537,7 +538,7 @@ Operation *OpBuilder::clone(Operation &op, IRMapping &mapper) {
for (Region ®ion : newOp->getRegions())
region.walk(walkFn);
}
- return insert(newOp);
+ return newOp;
}
Operation *OpBuilder::clone(Operation &op) {
diff --git a/mlir/test/IR/test-clone.mlir b/mlir/test/IR/test-clone.mlir
index 575098b642e8ea2..f7a29569d67d625 100644
--- a/mlir/test/IR/test-clone.mlir
+++ b/mlir/test/IR/test-clone.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline="builtin.module(func.func(test-clone))" -split-input-file
+// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline="builtin.module(func.func(test-clone))" | FileCheck %s
module {
func.func @fixpoint(%arg1 : i32) -> i32 {
@@ -9,6 +9,10 @@ module {
}
}
+// CHECK: notifyOperationInserted: test.use
+// CHECK-NEXT: notifyOperationInserted: test.yield
+// CHECK-NEXT: notifyOperationInserted: func.return
+
// CHECK: func @fixpoint(%[[arg0:.+]]: i32) -> i32 {
// CHECK-NEXT: %[[i0:.+]] = "test.use"(%[[arg0]]) ({
// CHECK-NEXT: "test.yield"(%arg0) : (i32) -> ()
diff --git a/mlir/test/lib/IR/TestClone.cpp b/mlir/test/lib/IR/TestClone.cpp
index 70238608a67c2b5..13a0cfeb402a9cd 100644
--- a/mlir/test/lib/IR/TestClone.cpp
+++ b/mlir/test/lib/IR/TestClone.cpp
@@ -14,6 +14,12 @@ using namespace mlir;
namespace {
+struct DumpNotifications : public OpBuilder::Listener {
+ void notifyOperationInserted(Operation *op) override {
+ llvm::outs() << "notifyOperationInserted: " << op->getName() << "\n";
+ }
+};
+
/// This is a test pass which clones the body of a function. Specifically
/// this pass replaces f(x) to instead return f(f(x)) in which the cloned body
/// takes the result of the first operation return as an input.
@@ -50,6 +56,8 @@ struct ClonePass
}
OpBuilder builder(op->getContext());
+ DumpNotifications dumpNotifications;
+ builder.setListener(&dumpNotifications);
builder.setInsertionPointToEnd(®ionEntry);
SmallVector<Operation *> toClone;
for (Operation &inst : regionEntry)
|
|
@llvm/pr-subscribers-mlir-core Author: None (jeanPerier) ChangesWhen cloning an operation with a region, the builder was currently notifying about the insertion of the cloned operations inside the region before the cloned operation itself. When using cloning inside rewrite pass, this could cause issues if a pattern is expected to be applied on a cloned parent operation before trying to apply patterns on the cloned operations it contains (the patterns are attempted in order of notifications for the cloned operations). Full diff: https://github.com/llvm/llvm-project/pull/73806.diff 3 Files Affected:
diff --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index ab20f4863e11c23..7bb7358426af5a4 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -527,6 +527,7 @@ LogicalResult OpBuilder::tryFold(Operation *op,
Operation *OpBuilder::clone(Operation &op, IRMapping &mapper) {
Operation *newOp = op.clone(mapper);
+ newOp = insert(newOp);
// The `insert` call below handles the notification for inserting `newOp`
// itself. But if `newOp` has any regions, we need to notify the listener
// about any ops that got inserted inside those regions as part of cloning.
@@ -537,7 +538,7 @@ Operation *OpBuilder::clone(Operation &op, IRMapping &mapper) {
for (Region ®ion : newOp->getRegions())
region.walk(walkFn);
}
- return insert(newOp);
+ return newOp;
}
Operation *OpBuilder::clone(Operation &op) {
diff --git a/mlir/test/IR/test-clone.mlir b/mlir/test/IR/test-clone.mlir
index 575098b642e8ea2..f7a29569d67d625 100644
--- a/mlir/test/IR/test-clone.mlir
+++ b/mlir/test/IR/test-clone.mlir
@@ -1,4 +1,4 @@
-// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline="builtin.module(func.func(test-clone))" -split-input-file
+// RUN: mlir-opt -allow-unregistered-dialect %s -pass-pipeline="builtin.module(func.func(test-clone))" | FileCheck %s
module {
func.func @fixpoint(%arg1 : i32) -> i32 {
@@ -9,6 +9,10 @@ module {
}
}
+// CHECK: notifyOperationInserted: test.use
+// CHECK-NEXT: notifyOperationInserted: test.yield
+// CHECK-NEXT: notifyOperationInserted: func.return
+
// CHECK: func @fixpoint(%[[arg0:.+]]: i32) -> i32 {
// CHECK-NEXT: %[[i0:.+]] = "test.use"(%[[arg0]]) ({
// CHECK-NEXT: "test.yield"(%arg0) : (i32) -> ()
diff --git a/mlir/test/lib/IR/TestClone.cpp b/mlir/test/lib/IR/TestClone.cpp
index 70238608a67c2b5..13a0cfeb402a9cd 100644
--- a/mlir/test/lib/IR/TestClone.cpp
+++ b/mlir/test/lib/IR/TestClone.cpp
@@ -14,6 +14,12 @@ using namespace mlir;
namespace {
+struct DumpNotifications : public OpBuilder::Listener {
+ void notifyOperationInserted(Operation *op) override {
+ llvm::outs() << "notifyOperationInserted: " << op->getName() << "\n";
+ }
+};
+
/// This is a test pass which clones the body of a function. Specifically
/// this pass replaces f(x) to instead return f(f(x)) in which the cloned body
/// takes the result of the first operation return as an input.
@@ -50,6 +56,8 @@ struct ClonePass
}
OpBuilder builder(op->getContext());
+ DumpNotifications dumpNotifications;
+ builder.setListener(&dumpNotifications);
builder.setInsertionPointToEnd(®ionEntry);
SmallVector<Operation *> toClone;
for (Operation &inst : regionEntry)
|
tblah
reviewed
Nov 29, 2023
joker-eph
reviewed
Nov 29, 2023
|
This change makes sense to me: when creating IR with the builder API, outer operations are typically created before inner operations. So it makes sense to notify in that order. (But patterns should not depend on a particular application order (apart from |
vzakhari
reviewed
Nov 29, 2023
When cloning and operation with a region, the builder was currently notifying about the insertion of the cloned operations inside the region before the operation itself. When using cloning inside rewrite pass, this could cause issues if a pattern is expected to be applied on a cloned parent operation before trying to apply patterns on the cloned operations it contains because the patterns are attempted in order of notifications for the cloned operations.
jeanPerier
force-pushed
the
clone-notification-order
branch
from
64dfc9b to
195d757
Compare
matthias-springer
approved these changes
Nov 30, 2023
Guzhu-AMD
pushed a commit
to GPUOpen-Drivers/llvm-project
that referenced
this pull request
Dec 7, 2023
Local branch amd-gfx 5377955 Merged main:9557fcca563d into amd-gfx:eb07c55c4b6a Remote branch main 5a4ca51 [mlir] notify insertion of parent op first when cloning (llvm#73806)
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.
When cloning an operation with a region, the builder was currently notifying about the insertion of the cloned operations inside the region before the cloned operation itself.
When using cloning inside rewrite pass, this could cause issues if a pattern is expected to be applied on a cloned parent operation before trying to apply patterns on the cloned operations it contains (the patterns are attempted in order of notifications for the cloned operations).