[mlir][IR] Send notifications for cloneRegionBefore
#66871
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
matthias-springer
requested review from
ftynse,
joker-eph,
nicolasvasilache and
Mogball
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Changes
Details: Ops/blocks are notified in such an order in which they could have been created one-by-one with an The implementation first clones an entire op (including nested ops) and then sends all notifications. Ideally, notifications should be interleaved with the cloning process, but that would require duplicating It is not possible to use the current implementation of There is a "fast path" in the Imported from Phabricator: https://reviews.llvm.org/D146943 Patch is 25.29 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/66871.diff 8 Files Affected:
diff --git a/mlir/include/mlir/IR/Builders.h b/mlir/include/mlir/IR/Builders.h
index 3eca8bd12f43364..4f5144d821b7e2f 100644
--- a/mlir/include/mlir/IR/Builders.h
+++ b/mlir/include/mlir/IR/Builders.h
@@ -451,6 +451,16 @@ class OpBuilder : public Builder {
Block *createBlock(Block *insertBefore, TypeRange argTypes = std::nullopt,
ArrayRef<Location> locs = std::nullopt);
+ /// Clone the blocks that belong to "region" before the given position in
+ /// another region "parent". The two regions must be different. The caller is
+ /// responsible for creating or updating the operation transferring flow of
+ /// control to the region and passing it the correct block arguments.
+ void cloneRegionBefore(Region ®ion, Region &parent,
+ Region::iterator before, IRMapping &mapping);
+ void cloneRegionBefore(Region ®ion, Region &parent,
+ Region::iterator before);
+ void cloneRegionBefore(Region ®ion, Block *before);
+
//===--------------------------------------------------------------------===//
// Operation Creation
//===--------------------------------------------------------------------===//
diff --git a/mlir/include/mlir/IR/PatternMatch.h b/mlir/include/mlir/IR/PatternMatch.h
index 6625ef553eba21f..276ef079ad7682f 100644
--- a/mlir/include/mlir/IR/PatternMatch.h
+++ b/mlir/include/mlir/IR/PatternMatch.h
@@ -489,16 +489,6 @@ class RewriterBase : public OpBuilder {
Region::iterator before);
void inlineRegionBefore(Region ®ion, Block *before);
- /// Clone the blocks that belong to "region" before the given position in
- /// another region "parent". The two regions must be different. The caller is
- /// responsible for creating or updating the operation transferring flow of
- /// control to the region and passing it the correct block arguments.
- virtual void cloneRegionBefore(Region ®ion, Region &parent,
- Region::iterator before, IRMapping &mapping);
- void cloneRegionBefore(Region ®ion, Region &parent,
- Region::iterator before);
- void cloneRegionBefore(Region ®ion, Block *before);
-
/// This method replaces the uses of the results of `op` with the values in
/// `newValues` when the provided `functor` returns true for a specific use.
/// The number of values in `newValues` is required to match the number of
diff --git a/mlir/include/mlir/Transforms/DialectConversion.h b/mlir/include/mlir/Transforms/DialectConversion.h
index 6de981d35c8c3ab..1763344425dbaa7 100644
--- a/mlir/include/mlir/Transforms/DialectConversion.h
+++ b/mlir/include/mlir/Transforms/DialectConversion.h
@@ -727,14 +727,6 @@ class ConversionPatternRewriter final : public PatternRewriter,
Region::iterator before) override;
using PatternRewriter::inlineRegionBefore;
- /// PatternRewriter hook for cloning blocks of one region into another. The
- /// given region to clone *must* not have been modified as part of conversion
- /// yet, i.e. it must be within an operation that is either in the process of
- /// conversion, or has not yet been converted.
- void cloneRegionBefore(Region ®ion, Region &parent,
- Region::iterator before, IRMapping &mapping) override;
- using PatternRewriter::cloneRegionBefore;
-
/// PatternRewriter hook for inserting a new operation.
void notifyOperationInserted(Operation *op) override;
diff --git a/mlir/lib/IR/Builders.cpp b/mlir/lib/IR/Builders.cpp
index ab20f4863e11c23..3a4a6b5b4eaa290 100644
--- a/mlir/lib/IR/Builders.cpp
+++ b/mlir/lib/IR/Builders.cpp
@@ -14,7 +14,11 @@
#include "mlir/IR/IRMapping.h"
#include "mlir/IR/IntegerSet.h"
#include "mlir/IR/Matchers.h"
+#include "mlir/IR/RegionGraphTraits.h"
#include "mlir/IR/SymbolTable.h"
+#include "llvm/ADT/PostOrderIterator.h"
+#include "llvm/ADT/SetVector.h"
+#include "llvm/ADT/SmallPtrSet.h"
#include "llvm/ADT/SmallVectorExtras.h"
#include "llvm/Support/raw_ostream.h"
@@ -525,18 +529,66 @@ LogicalResult OpBuilder::tryFold(Operation *op,
return success();
}
+static void notifyOperationCloned(Operation *op, OpBuilder::Listener *listener);
+
+/// Notify the listener that the given range of regions was cloned.
+///
+/// Blocks and operations are enumerated in an order in which they could have
+/// been created separately (without using the `clone` API): Within a region,
+/// blocks are notified according to their successor relationship. Within a
+/// block, operations are notified in forward mode. This ensures that defining
+/// ops are notified before ops that use their results (unless there are
+/// cycles/graph regions).
+static void notifyRegionsCloned(iterator_range<Region::iterator> range,
+ OpBuilder::Listener *listener) {
+ // Maintain a set of blocks that were not notified yet. This is needed because
+ // the inverse_post_order iterator does not enumerate dead blocks.
+ llvm::SetVector<Block *> remainingBlocks;
+ // The order in which the set is initialized does not matter for correctness.
+ // For better performance, "leaf" blocks with no successors should be starting
+ // point for the block traversal. (Then there are fewer iterations of the
+ // "while" loop.)
+ for (Block &b : llvm::reverse(range))
+ remainingBlocks.insert(&b);
+ // Set of visited blocks that is shared among all inverse_post_order
+ // iterations. This is to avoid that the same block is enumerated multiple
+ // times.
+ llvm::SmallPtrSet<Block *, 4> visited;
+ while (!remainingBlocks.empty()) {
+ // Enumerate predecessors before successors. I.e., reverse post-order.
+ for (Block *b :
+ llvm::inverse_post_order_ext(remainingBlocks.front(), visited)) {
+ auto it = llvm::find(remainingBlocks, b);
+ assert(it != remainingBlocks.end() &&
+ "expected that only remaining blocks are visited");
+ listener->notifyBlockCreated(b);
+ remainingBlocks.erase(it);
+ for (Operation &op : *b)
+ notifyOperationCloned(&op, listener);
+ }
+ }
+}
+
+/// Notify the listener that the given op was cloned.
+static void notifyOperationCloned(Operation *op,
+ OpBuilder::Listener *listener) {
+ listener->notifyOperationInserted(op);
+ for (Region &r : op->getRegions())
+ notifyRegionsCloned(r.getBlocks(), listener);
+}
+
Operation *OpBuilder::clone(Operation &op, IRMapping &mapper) {
+ // TODO: The listener notifications should be interleaved with `clone`.
Operation *newOp = op.clone(mapper);
- // 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.
- if (listener) {
- auto walkFn = [&](Operation *walkedOp) {
- listener->notifyOperationInserted(walkedOp);
- };
- for (Region ®ion : newOp->getRegions())
- region.walk(walkFn);
- }
+
+ // Fast path: If no listener is attached, the op can be inserted directly.
+ if (!listener)
+ return insert(newOp);
+
+ // The `insert` call below handles the notification for inserting `newOp`.
+ // Just notify about nested op/block insertion.
+ for (Region &r : newOp->getRegions())
+ notifyRegionsCloned(r.getBlocks(), listener);
return insert(newOp);
}
@@ -544,3 +596,28 @@ Operation *OpBuilder::clone(Operation &op) {
IRMapping mapper;
return clone(op, mapper);
}
+
+void OpBuilder::cloneRegionBefore(Region ®ion, Region &parent,
+ Region::iterator before, IRMapping &mapping) {
+ // TODO: The listener notifications should be interleaved with `clone`.
+ region.cloneInto(&parent, before, mapping);
+
+ // Fast path: If no listener is attached, there is no more work to do.
+ if (!listener)
+ return;
+
+ // Notify about op/block insertion.
+ Region::iterator clonedBeginIt =
+ mapping.lookup(®ion.front())->getIterator();
+ notifyRegionsCloned(llvm::make_range(clonedBeginIt, before), listener);
+}
+
+void OpBuilder::cloneRegionBefore(Region ®ion, Region &parent,
+ Region::iterator before) {
+ IRMapping mapping;
+ cloneRegionBefore(region, parent, before, mapping);
+}
+
+void OpBuilder::cloneRegionBefore(Region ®ion, Block *before) {
+ cloneRegionBefore(region, *before->getParent(), before->getIterator());
+}
diff --git a/mlir/lib/IR/PatternMatch.cpp b/mlir/lib/IR/PatternMatch.cpp
index 5e9b9b2a810a4c5..a6f09f1609b3bae 100644
--- a/mlir/lib/IR/PatternMatch.cpp
+++ b/mlir/lib/IR/PatternMatch.cpp
@@ -458,21 +458,3 @@ void RewriterBase::inlineRegionBefore(Region ®ion, Region &parent,
void RewriterBase::inlineRegionBefore(Region ®ion, Block *before) {
inlineRegionBefore(region, *before->getParent(), before->getIterator());
}
-
-/// Clone the blocks that belong to "region" before the given position in
-/// another region "parent". The two regions must be different. The caller is
-/// responsible for creating or updating the operation transferring flow of
-/// control to the region and passing it the correct block arguments.
-void RewriterBase::cloneRegionBefore(Region ®ion, Region &parent,
- Region::iterator before,
- IRMapping &mapping) {
- region.cloneInto(&parent, before, mapping);
-}
-void RewriterBase::cloneRegionBefore(Region ®ion, Region &parent,
- Region::iterator before) {
- IRMapping mapping;
- cloneRegionBefore(region, parent, before, mapping);
-}
-void RewriterBase::cloneRegionBefore(Region ®ion, Block *before) {
- cloneRegionBefore(region, *before->getParent(), before->getIterator());
-}
diff --git a/mlir/lib/Transforms/Utils/DialectConversion.cpp b/mlir/lib/Transforms/Utils/DialectConversion.cpp
index 4d2afe462b9281d..e0b8e0373605462 100644
--- a/mlir/lib/Transforms/Utils/DialectConversion.cpp
+++ b/mlir/lib/Transforms/Utils/DialectConversion.cpp
@@ -1588,23 +1588,6 @@ void ConversionPatternRewriter::inlineRegionBefore(Region ®ion,
PatternRewriter::inlineRegionBefore(region, parent, before);
}
-void ConversionPatternRewriter::cloneRegionBefore(Region ®ion,
- Region &parent,
- Region::iterator before,
- IRMapping &mapping) {
- if (region.empty())
- return;
-
- PatternRewriter::cloneRegionBefore(region, parent, before, mapping);
-
- for (Block &b : ForwardDominanceIterator<>::makeIterable(region)) {
- Block *cloned = mapping.lookup(&b);
- impl->notifyCreatedBlock(cloned);
- cloned->walk<WalkOrder::PreOrder, ForwardDominanceIterator<>>(
- [&](Operation *op) { notifyOperationInserted(op); });
- }
-}
-
void ConversionPatternRewriter::notifyOperationInserted(Operation *op) {
LLVM_DEBUG({
impl->logger.startLine()
diff --git a/mlir/test/Transforms/test-strict-pattern-driver.mlir b/mlir/test/Transforms/test-strict-pattern-driver.mlir
index a5ab8f97c74ce33..f7c1200ca8ad2a4 100644
--- a/mlir/test/Transforms/test-strict-pattern-driver.mlir
+++ b/mlir/test/Transforms/test-strict-pattern-driver.mlir
@@ -18,7 +18,7 @@
func.func @test_erase() {
%0 = "test.arg0"() : () -> (i32)
%1 = "test.arg1"() : () -> (i32)
- %erase = "test.erase_op"(%0, %1) : (i32, i32) -> (i32)
+ %erase = "test.erase_op"(%0, %1) {worklist} : (i32, i32) -> (i32)
return
}
@@ -29,7 +29,7 @@ func.func @test_erase() {
// CHECK-EN: "test.insert_same_op"() {skip = true}
// CHECK-EN: "test.insert_same_op"() {skip = true}
func.func @test_insert_same_op() {
- %0 = "test.insert_same_op"() : () -> (i32)
+ %0 = "test.insert_same_op"() {worklist} : () -> (i32)
return
}
@@ -41,7 +41,7 @@ func.func @test_insert_same_op() {
// CHECK-EN: "test.dummy_user"(%[[n]])
// CHECK-EN: "test.dummy_user"(%[[n]])
func.func @test_replace_with_new_op() {
- %0 = "test.replace_with_new_op"() : () -> (i32)
+ %0 = "test.replace_with_new_op"() {worklist} : () -> (i32)
%1 = "test.dummy_user"(%0) : (i32) -> (i32)
%2 = "test.dummy_user"(%0) : (i32) -> (i32)
return
@@ -59,7 +59,7 @@ func.func @test_replace_with_new_op() {
// CHECK-EX-NOT: "test.replace_with_new_op"
// CHECK-EX: "test.erase_op"
func.func @test_replace_with_erase_op() {
- "test.replace_with_new_op"() {create_erase_op} : () -> ()
+ "test.replace_with_new_op"() {create_erase_op, worklist} : () -> ()
return
}
@@ -74,14 +74,14 @@ func.func @test_trigger_rewrite_through_block() {
return
^bb1:
// Uses bb1. ChangeBlockOp replaces that and all other usages of bb1 with bb2.
- "test.change_block_op"() [^bb1, ^bb2] : () -> ()
+ "test.change_block_op"() [^bb1, ^bb2] {worklist} : () -> ()
^bb2:
return
^bb3:
// Also uses bb1. ChangeBlockOp replaces that usage with bb2. This triggers
// this op being put on the worklist, which triggers ImplicitChangeOp, which,
// in turn, replaces the successor with bb3.
- "test.implicit_change_op"() [^bb1] : () -> ()
+ "test.implicit_change_op"() [^bb1] {worklist} : () -> ()
}
// -----
@@ -98,7 +98,7 @@ func.func @test_remove_graph_region() {
%0 = "test.foo_a"(%1) : (i1) -> (i1)
%1 = "test.foo_b"(%0) : (i1) -> (i1)
}
- }) : () -> ()
+ }) {worklist} : () -> ()
return
}
@@ -123,7 +123,7 @@ func.func @test_remove_cyclic_blocks() {
^bb2(%arg1: i1):
"test.bar"(%x) : (i1) -> ()
cf.br ^bb1(%arg1: i1)
- }) : () -> ()
+ }) {worklist} : () -> ()
return
}
@@ -155,7 +155,7 @@ func.func @test_remove_dead_blocks() {
"test.qux_unreachable"() : () -> ()
}) : () -> ()
"test.bar"() : () -> ()
- }) : () -> ()
+ }) {worklist} : () -> ()
return
}
@@ -201,7 +201,7 @@ func.func @test_remove_nested_ops() {
^bb2(%arg1: i1):
"test.bar"(%x) : (i1) -> ()
cf.br ^bb1(%arg1: i1)
- }) : () -> ()
+ }) {worklist} : () -> ()
return
}
@@ -226,6 +226,224 @@ func.func @test_remove_diamond(%c: i1) {
cf.br ^bb3
^bb3:
"test.qux"() : () -> ()
- }) : () -> ()
+ }) {worklist} : () -> ()
return
}
+
+// -----
+
+// Make sure that test.erase_op is deleted.
+
+// CHECK-AN: notifyBlockCreated: block ^bb1 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.inner_op
+// CHECK-AN: notifyOperationInserted: test.erase_op
+// CHECK-AN: notifyOperationRemoved: test.erase_op
+// CHECK-AN: notifyOperationRemoved: test.inner_op
+// CHECK-AN: notifyOperationRemoved: test.clone_region_in_parent
+// CHECK-AN: notifyOperationRemoved: test.erase_op
+// CHECK-AN-LABEL: func @test_add_cloned_ops_to_worklist
+
+// CHECK-EN-LABEL: func @test_add_cloned_ops_to_worklist
+// CHECK-EN-NEXT: "test.dummy_op"() ({
+// CHECK-EN-NEXT: "test.another_op"() : () -> ()
+// CHECK-EN-NEXT: ^bb1: // no predecessors
+// CHECK-EN-NEXT: "test.inner_op"() : () -> ()
+// CHECK-EN-NEXT: }) : () -> ()
+// CHECK-EN-NEXT: }
+
+// When strictness=ExistingOps, test.erase_op is not deleted because it was not
+// on the initial worklist.
+
+// CHECK-EX-LABEL: func @test_add_cloned_ops_to_worklist
+// CHECK-EX-NEXT: "test.dummy_op"() ({
+// CHECK-EX-NEXT: "test.another_op"() : () -> ()
+// CHECK-EX-NEXT: ^bb1: // no predecessors
+// CHECK-EX-NEXT: "test.inner_op"() : () -> ()
+// CHECK-EX-NEXT: "test.erase_op"() : () -> ()
+// CHECK-EX-NEXT: }) : () -> ()
+// CHECK-EX-NEXT: }
+func.func @test_add_cloned_ops_to_worklist() {
+ "test.dummy_op"() ({
+ ^bb0():
+ "test.clone_region_in_parent"() ({
+ ^bb1():
+ "test.inner_op"() : () -> ()
+ "test.erase_op"() {worklist} : () -> ()
+ }) {worklist} : () -> ()
+ "test.another_op"() : () -> ()
+ }) {worklist} : () -> ()
+}
+
+// -----
+
+// CHECK-AN: notifyBlockCreated: block ^bb1 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.inner_op
+// CHECK-AN: notifyBlockCreated: block ^bb0 from region 0 from operation 'test.inner_op'
+// CHECK-AN: notifyOperationInserted: test.graph_region
+// CHECK-AN: notifyBlockCreated: block ^bb0 from region 0 from operation 'test.graph_region'
+// CHECK-AN: notifyOperationInserted: test.foo_a
+// CHECK-AN: notifyOperationInserted: test.foo_b
+// CHECK-AN: notifyOperationInserted: test.foo_c
+// The original op is removed.
+// CHECK-AN: notifyOperationRemoved: test.foo_c
+// CHECK-AN: notifyOperationRemoved: test.foo_b
+// CHECK-AN: notifyOperationRemoved: test.foo_a
+// CHECK-AN: notifyOperationRemoved: test.graph_region
+// CHECK-AN: notifyOperationRemoved: test.inner_op
+// CHECK-AN: notifyOperationRemoved: test.clone_region_in_parent
+// CHECK-AN-LABEL: func @test_clone_nested_graph_region
+// CHECK-AN: "test.dummy_op"() ({
+// CHECK-AN-NEXT: "test.another_op"() : () -> ()
+// CHECK-AN-NEXT: ^bb1: // no predecessors
+// CHECK-AN-NEXT: "test.inner_op"() ({
+// CHECK-AN-NEXT: test.graph_region {
+// CHECK-AN-NEXT: %{{.*}} = "test.foo_a"(%{{.*}}) : (i1) -> i1
+// CHECK-AN-NEXT: %{{.*}} = "test.foo_b"(%{{.*}}) : (i1) -> i1
+// CHECK-AN-NEXT: }
+// CHECK-AN-NEXT: "test.foo_c"() : () -> ()
+// CHECK-AN-NEXT: }) : () -> ()
+// CHECK-AN-NEXT: }) : () -> ()
+func.func @test_clone_nested_graph_region() {
+ "test.dummy_op"() ({
+ ^bb0():
+ "test.clone_region_in_parent"() ({
+ ^bb1():
+ "test.inner_op"() ({
+ test.graph_region {
+ %0 = "test.foo_a"(%1) : (i1) -> (i1)
+ %1 = "test.foo_b"(%0) : (i1) -> (i1)
+ }
+ "test.foo_c"() : () -> ()
+ }): () -> ()
+ }) {worklist} : () -> ()
+ "test.another_op"() : () -> ()
+ }) {worklist} : () -> ()
+}
+
+// -----
+
+// CHECK-AN: notifyBlockCreated: block ^bb1 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.dummy_op
+// CHECK-AN: notifyOperationInserted: cf.br
+// CHECK-AN: notifyBlockCreated: block ^bb2 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.foo
+// CHECK-AN: notifyOperationInserted: cf.br
+// CHECK-AN: notifyBlockCreated: block ^bb3 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.bar
+// CHECK-AN: notifyOperationInserted: cf.br
+// CHECK-AN: notifyOperationRemoved: cf.br
+// CHECK-AN: notifyOperationRemoved: test.bar
+// CHECK-AN: notifyOperationRemoved: cf.br
+// CHECK-AN: notifyOperationRemoved: test.foo
+// CHECK-AN: notifyOperationRemoved: cf.br
+// CHECK-AN: notifyOperationRemoved: test.dummy_op
+// CHECK-AN: notifyOperationRemoved: test.clone_region_in_parent
+// CHECK-AN-LABEL: func @test_clone_cyclic_blocks
+func.func @test_clone_cyclic_blocks() {
+ "test.dummy_op"() ({
+ ^bb0():
+ "test.clone_region_in_parent"() ({
+ %x = "test.dummy_op"() : () -> (i1)
+ cf.br ^bb1(%x: i1)
+ ^bb1(%arg0: i1):
+ "test.foo"(%x) : (i1) -> ()
+ cf.br ^bb2(%arg0: i1)
+ ^bb2(%arg1: i1):
+ "test.bar"(%x) : (i1) -> ()
+ cf.br ^bb1(%arg1: i1)
+ }) {worklist} : () -> ()
+ "test.qux"() : () -> ()
+ }) : () -> ()
+ "test.another_op"() : () -> ()
+ return
+}
+
+// -----
+
+// CHECK-AN: notifyBlockCreated: block ^bb3 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.nested_dummy
+// CHECK-AN: notifyBlockCreated: block ^bb1 from region 0 from operation 'test.nested_dummy'
+// CHECK-AN: notifyOperationInserted: test.qux_unreachable
+// CHECK-AN: notifyBlockCreated: block ^bb0 from region 0 from operation 'test.nested_dummy'
+// CHECK-AN: notifyOperationInserted: test.qux_reachable
+// CHECK-AN: notifyOperationInserted: test.bar
+// CHECK-AN: notifyOperationInserted: cf.br
+// CHECK-AN: notifyBlockCreated: block ^bb2 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.foo
+// CHECK-AN: notifyBlockCreated: block ^bb1 from region 0 from operation 'test.dummy_op'
+// CHECK-AN: notifyOperationInserted: test.dummy_op
+// CHECK-AN: notifyOperationRemoved: test.dummy_op
+// CHECK-AN: notifyOperationRemoved: test.foo
+// CHECK-AN: notifyOperationRemoved: cf.br
+// CHECK-AN: notifyOperationRemoved: test.bar
+// CHECK-AN...
[truncated]
|
matthias-springer
force-pushed
the
op_builder_clone_notifications
branch
from
26be7a8 to
3051c7d
Compare
matthias-springer
changed the title
cloneRegionBefore
jpienaar
approved these changes
Feb 2, 2024
Similar to `OpBuilder::clone`, operation/block insertion notifications should be sent when cloning the contents of a region. E.g., this is to ensure that the newly created operations are put on the worklist of the greedy pattern rewriter driver. Also move `cloneRegionBefore` from `RewriterBase` to `OpBuilder`. It only creates new IR, so it should be part of the builder API (like `clone(Operation &)`). The function does not have to be virtual. Now that notifications are properly sent, the override in the dialect conversion is no longer needed. BEGIN_PUBLIC No public commit message for presubmit. END_PUBLIC
matthias-springer
force-pushed
the
op_builder_clone_notifications
branch
from
3051c7d to
c117f54
Compare
agozillon
pushed a commit
to agozillon/llvm-project
that referenced
this pull request
Feb 5, 2024
Similar to `OpBuilder::clone`, operation/block insertion notifications should be sent when cloning the contents of a region. E.g., this is to ensure that the newly created operations are put on the worklist of the greedy pattern rewriter driver. Also move `cloneRegionBefore` from `RewriterBase` to `OpBuilder`. It only creates new IR, so it should be part of the builder API (like `clone(Operation &)`). The function does not have to be virtual. Now that notifications are properly sent, the override in the dialect conversion is no longer needed.
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.
Similar to
OpBuilder::clone, operation/block insertion notifications should be sent when cloning the contents of a region. E.g., this is to ensure that the newly created operations are put on the worklist of the greedy pattern rewriter driver.Also move
cloneRegionBeforefromRewriterBasetoOpBuilder. It only creates new IR, so it should be part of the builder API (likeclone(Operation &)). The function does not have to be virtual. Now that notifications are properly sent, the override in the dialect conversion is no longer needed.