From 4f7b50d8906ee116f846701355904cba04e13022 Mon Sep 17 00:00:00 2001 From: Congcong Cai 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 5ba5328f14b89..56e3c44acf191 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 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 0000000000000..0037cec0a96ab --- /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 c87444cba8e1a..cdade72b69e0a 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 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 56e3c44acf191..3edeb50b18266 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 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 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 cdade72b69e0a..c87444cba8e1a 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 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 3edeb50b18266..8796289d72570 100644 --- a/mlir/lib/IR/PatternMatch.cpp +++ b/mlir/lib/IR/PatternMatch.cpp @@ -231,7 +231,7 @@ void RewriterBase::eraseOp(Operation *op) { SmallVector erasedBlocks; // Some blocks may have invalid successor, use a set including nullptr // to avoid null pointer. - std::set visited{nullptr}; + llvm::SmallPtrSet visited{nullptr}; for (Block *b : llvm::post_order_ext(&r.front(), visited)) { // Visit ops in reverse order. for (Operation &op :