[Mlir-commits] [mlir] [mlir]use correct iterator when eraseOp (PR #83444)
Congcong Cai
llvmlistbot at llvm.org
Mon Mar 4 22:06:26 PST 2024
https://github.com/HerrCai0907 updated https://github.com/llvm/llvm-project/pull/83444
>From 4f7b50d8906ee116f846701355904cba04e13022 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Thu, 29 Feb 2024 19:28:50 +0800
Subject: [PATCH 1/3] [mlir]use correct iterator when eraseOp
`llvm::post_order(&r.front())` is equal to `r.front().getSuccessor(...)`.
It will visit the succ block of current block. But actually here need to visit all block in this region.
Fixes: #77420.
---
mlir/lib/IR/PatternMatch.cpp | 6 +++---
mlir/test/Transforms/gh-77420.mlir | 21 +++++++++++++++++++
.../test-strict-pattern-driver.mlir | 14 ++++++-------
3 files changed, 31 insertions(+), 10 deletions(-)
create mode 100644 mlir/test/Transforms/gh-77420.mlir
diff --git a/mlir/lib/IR/PatternMatch.cpp b/mlir/lib/IR/PatternMatch.cpp
index 5ba5328f14b89e..56e3c44acf1913 100644
--- a/mlir/lib/IR/PatternMatch.cpp
+++ b/mlir/lib/IR/PatternMatch.cpp
@@ -229,14 +229,14 @@ void RewriterBase::eraseOp(Operation *op) {
// until the region is empty. (The block graph could be disconnected.)
while (!r.empty()) {
SmallVector<Block *> erasedBlocks;
- for (Block *b : llvm::post_order(&r.front())) {
+ for (Block &b : llvm::reverse(r.getBlocks())) {
// Visit ops in reverse order.
for (Operation &op :
- llvm::make_early_inc_range(ReverseIterator::makeIterable(*b)))
+ llvm::make_early_inc_range(ReverseIterator::makeIterable(b)))
eraseTree(&op);
// Do not erase the block immediately. This is not supprted by the
// post_order iterator.
- erasedBlocks.push_back(b);
+ erasedBlocks.push_back(&b);
}
for (Block *b : erasedBlocks) {
// Explicitly drop all uses in case there is a cycle in the block
diff --git a/mlir/test/Transforms/gh-77420.mlir b/mlir/test/Transforms/gh-77420.mlir
new file mode 100644
index 00000000000000..0037cec0a96ab3
--- /dev/null
+++ b/mlir/test/Transforms/gh-77420.mlir
@@ -0,0 +1,21 @@
+// RUN: mlir-opt --canonicalize %s | FileCheck %s
+
+
+module {
+
+// CHECK: func.func @f() {
+// CHECK-NEXT: return
+// CHECK-NEXT: }
+ func.func @f() {
+ return
+ ^bb1: // no predecessors
+ omp.parallel {
+ %0 = llvm.intr.stacksave : !llvm.ptr
+ llvm.br ^bb1
+ ^bb1: // pred: ^bb0
+ omp.terminator
+ }
+ return
+ }
+
+}
diff --git a/mlir/test/Transforms/test-strict-pattern-driver.mlir b/mlir/test/Transforms/test-strict-pattern-driver.mlir
index c87444cba8e1a2..cdade72b69e0aa 100644
--- a/mlir/test/Transforms/test-strict-pattern-driver.mlir
+++ b/mlir/test/Transforms/test-strict-pattern-driver.mlir
@@ -134,13 +134,13 @@ func.func @test_remove_cyclic_blocks() {
// -----
-// CHECK-AN: notifyOperationErased: test.dummy_op
// CHECK-AN: notifyOperationErased: test.bar
-// CHECK-AN: notifyOperationErased: test.qux
// CHECK-AN: notifyOperationErased: test.qux_unreachable
+// CHECK-AN: notifyOperationErased: test.qux
// CHECK-AN: notifyOperationErased: test.nested_dummy
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.foo
+// CHECK-AN: notifyOperationErased: test.dummy_op
// CHECK-AN: notifyOperationErased: test.erase_op
// CHECK-AN-LABEL: func @test_remove_dead_blocks()
// CHECK-AN-NEXT: return
@@ -172,13 +172,13 @@ func.func @test_remove_dead_blocks() {
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.bar
// CHECK-AN: notifyOperationErased: cf.br
-// CHECK-AN: notifyOperationErased: test.nested_b
-// CHECK-AN: notifyOperationErased: test.nested_a
-// CHECK-AN: notifyOperationErased: test.nested_d
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.nested_e
+// CHECK-AN: notifyOperationErased: test.nested_d
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.nested_c
+// CHECK-AN: notifyOperationErased: test.nested_b
+// CHECK-AN: notifyOperationErased: test.nested_a
// CHECK-AN: notifyOperationErased: test.foo
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.dummy_op
@@ -214,9 +214,9 @@ func.func @test_remove_nested_ops() {
// CHECK-AN: notifyOperationErased: test.qux
// CHECK-AN: notifyOperationErased: cf.br
-// CHECK-AN: notifyOperationErased: test.foo
-// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.bar
+// CHECK-AN: notifyOperationErased: cf.br
+// CHECK-AN: notifyOperationErased: test.foo
// CHECK-AN: notifyOperationErased: cf.cond_br
// CHECK-AN-LABEL: func @test_remove_diamond(
// CHECK-AN-NEXT: return
>From 4ece190d3a6a13889e8d95d59bb4290b7699012c Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Fri, 1 Mar 2024 07:44:55 +0800
Subject: [PATCH 2/3] still use post-order but ignore nullptr
---
mlir/lib/IR/PatternMatch.cpp | 9 ++++++---
.../Transforms/test-strict-pattern-driver.mlir | 14 +++++++-------
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/mlir/lib/IR/PatternMatch.cpp b/mlir/lib/IR/PatternMatch.cpp
index 56e3c44acf1913..3edeb50b18266a 100644
--- a/mlir/lib/IR/PatternMatch.cpp
+++ b/mlir/lib/IR/PatternMatch.cpp
@@ -229,14 +229,17 @@ void RewriterBase::eraseOp(Operation *op) {
// until the region is empty. (The block graph could be disconnected.)
while (!r.empty()) {
SmallVector<Block *> erasedBlocks;
- for (Block &b : llvm::reverse(r.getBlocks())) {
+ // Some blocks may have invalid successor, use a set including nullptr
+ // to avoid null pointer.
+ std::set<Block *> visited{nullptr};
+ for (Block *b : llvm::post_order_ext(&r.front(), visited)) {
// Visit ops in reverse order.
for (Operation &op :
- llvm::make_early_inc_range(ReverseIterator::makeIterable(b)))
+ llvm::make_early_inc_range(ReverseIterator::makeIterable(*b)))
eraseTree(&op);
// Do not erase the block immediately. This is not supprted by the
// post_order iterator.
- erasedBlocks.push_back(&b);
+ erasedBlocks.push_back(b);
}
for (Block *b : erasedBlocks) {
// Explicitly drop all uses in case there is a cycle in the block
diff --git a/mlir/test/Transforms/test-strict-pattern-driver.mlir b/mlir/test/Transforms/test-strict-pattern-driver.mlir
index cdade72b69e0aa..c87444cba8e1a2 100644
--- a/mlir/test/Transforms/test-strict-pattern-driver.mlir
+++ b/mlir/test/Transforms/test-strict-pattern-driver.mlir
@@ -134,13 +134,13 @@ func.func @test_remove_cyclic_blocks() {
// -----
+// CHECK-AN: notifyOperationErased: test.dummy_op
// CHECK-AN: notifyOperationErased: test.bar
-// CHECK-AN: notifyOperationErased: test.qux_unreachable
// CHECK-AN: notifyOperationErased: test.qux
+// CHECK-AN: notifyOperationErased: test.qux_unreachable
// CHECK-AN: notifyOperationErased: test.nested_dummy
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.foo
-// CHECK-AN: notifyOperationErased: test.dummy_op
// CHECK-AN: notifyOperationErased: test.erase_op
// CHECK-AN-LABEL: func @test_remove_dead_blocks()
// CHECK-AN-NEXT: return
@@ -172,13 +172,13 @@ func.func @test_remove_dead_blocks() {
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.bar
// CHECK-AN: notifyOperationErased: cf.br
+// CHECK-AN: notifyOperationErased: test.nested_b
+// CHECK-AN: notifyOperationErased: test.nested_a
+// CHECK-AN: notifyOperationErased: test.nested_d
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.nested_e
-// CHECK-AN: notifyOperationErased: test.nested_d
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.nested_c
-// CHECK-AN: notifyOperationErased: test.nested_b
-// CHECK-AN: notifyOperationErased: test.nested_a
// CHECK-AN: notifyOperationErased: test.foo
// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.dummy_op
@@ -214,9 +214,9 @@ func.func @test_remove_nested_ops() {
// CHECK-AN: notifyOperationErased: test.qux
// CHECK-AN: notifyOperationErased: cf.br
-// CHECK-AN: notifyOperationErased: test.bar
-// CHECK-AN: notifyOperationErased: cf.br
// CHECK-AN: notifyOperationErased: test.foo
+// CHECK-AN: notifyOperationErased: cf.br
+// CHECK-AN: notifyOperationErased: test.bar
// CHECK-AN: notifyOperationErased: cf.cond_br
// CHECK-AN-LABEL: func @test_remove_diamond(
// CHECK-AN-NEXT: return
>From 11145f0b3b71396c930c284cdbc3c0e897bcd7a5 Mon Sep 17 00:00:00 2001
From: Congcong Cai <congcongcai0907 at 163.com>
Date: Fri, 1 Mar 2024 09:12:24 +0800
Subject: [PATCH 3/3] use small ptr set
---
mlir/lib/IR/PatternMatch.cpp | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mlir/lib/IR/PatternMatch.cpp b/mlir/lib/IR/PatternMatch.cpp
index 3edeb50b18266a..8796289d725707 100644
--- a/mlir/lib/IR/PatternMatch.cpp
+++ b/mlir/lib/IR/PatternMatch.cpp
@@ -231,7 +231,7 @@ void RewriterBase::eraseOp(Operation *op) {
SmallVector<Block *> erasedBlocks;
// Some blocks may have invalid successor, use a set including nullptr
// to avoid null pointer.
- std::set<Block *> visited{nullptr};
+ llvm::SmallPtrSet<Block *, 4> visited{nullptr};
for (Block *b : llvm::post_order_ext(&r.front(), visited)) {
// Visit ops in reverse order.
for (Operation &op :
More information about the Mlir-commits
mailing list