Skip to content

[MLIR] Erase unreachable blocks before applying patterns in the greedy rewriter #153957

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 18, 2025
Merged

[MLIR] Erase unreachable blocks before applying patterns in the greedy rewriter #153957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion mlir/lib/Transforms/Utils/GreedyPatternRewriteDriver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -871,7 +871,18 @@ LogicalResult RegionPatternRewriteDriver::simplify(bool *changed) && {

ctx->executeAction<GreedyPatternRewriteIteration>(
[&] {
continueRewrites = processWorklist();
continueRewrites = false;

// Erase unreachable blocks
// Operations like:
// %add = arith.addi %add, %add : i64
// are legal in unreachable code. Unfortunately many patterns would be
// unsafe to apply on such IR and can lead to crashes or infinite
// loops.
continueRewrites |=
succeeded(eraseUnreachableBlocks(rewriter, region));

continueRewrites |= processWorklist();

// After applying patterns, make sure that the CFG of each of the
// regions is kept up to date.
Expand Down
15 changes: 15 additions & 0 deletions mlir/test/Dialect/Arith/canonicalize.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3363,3 +3363,18 @@ func.func @bf16_fma(%arg0: vector<32x32x32xbf16>, %arg1: vector<32x32x32xbf16>,
}
}
#-}

// CHECK-LABEL: func @unreachable()
// CHECK-NEXT: return
// CHECK-NOT: arith
func.func @unreachable() {
return
^unreachable:
%c1_i64 = arith.constant 1 : i64
// This self referencing operation is legal in an unreachable block.
// Many patterns are unsafe with respect to this kind of situation,
// check that we don't infinite loop here.
%add = arith.addi %add, %c1_i64 : i64
Copy link
Member

@matthias-springer matthias-springer Aug 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't an SSACFG region require that all definitions come before their uses?

Copy link
Collaborator Author

@joker-eph joker-eph Aug 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not in unreachable blocks where self-references are allowed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason why we allow this? Is it also allowed to use values that are going to be defined later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is because an unreachable block is considering to self-dominate itself: so there is no violation of the dominance.
This isn't an MLIR thing by the way, that's how LLVM works already.

One of the reason why this is really useful is that it preserves the ability to perform local transformation without having to look for "effects at a distance".
That is: simple patterns of replacement doing RAUW, or updating the CFG can easily lead to this situation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. The moment a block is unreachable, we completely turn off all dominance checking. It's basically like a graph region from that point on. This IR also verifies:

func.func @unreachable() {
  return
^unreachable:
  %add = arith.addi %add, %c1_i64 : i64
  %c1_i64 = arith.constant 1 : i64
  cf.br ^unreachable
}

One of the reason why this is really useful is that it preserves the ability to perform local transformation without having to look for "effects at a distance".

I don't follow. When you do a local transformation, you don't know that you are operating on unreachable IR. RAUW etc. must be guarded with extra checks to ensure that no dominance violations are created.

Copy link
Collaborator Author

@joker-eph joker-eph Aug 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. When you do a local transformation, you don't know that you are operating on unreachable IR.

Exactly, so you don't know you would violate things (like creating a self-referencing operation).
For example the "self-referencing operation can be created from folding the second add here:

^unreachable(%arg : i64):
  %add = arith.addi %add_plus_zero, %c1_i64 : i64
  %add_plus_zero  = arith.addi %arg, %c0_i64 : i64
  cf.cond_br %cond  ^unreachable, ^other

The folder would just do RAUW here.
Of course no one builds the IR that way, but you start with a CFG which has blocks which are dynamically unreachable, then you propagate some constants, simplify some branches, end up with some unreachable blocks, and after more simplification you end up here.

cf.br ^unreachable
}

16 changes: 7 additions & 9 deletions mlir/test/Transforms/test-canonicalize.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize))' | FileCheck %s
// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize))' | FileCheck %s --check-prefixes=CHECK,RS
// RUN: mlir-opt %s -pass-pipeline='builtin.module(func.func(canonicalize{region-simplify=disabled}))' | FileCheck %s --check-prefixes=CHECK,NO-RS

// CHECK-LABEL: func @remove_op_with_inner_ops_pattern
Expand Down Expand Up @@ -80,12 +80,10 @@ func.func @test_dialect_canonicalizer() -> (i32) {

// Check that the option to control region simplification actually works
// CHECK-LABEL: test_region_simplify
func.func @test_region_simplify() {
// CHECK-NEXT: return
// NO-RS-NEXT: ^bb1
// NO-RS-NEXT: return
// CHECK-NEXT: }
return
^bb1:
return
func.func @test_region_simplify(%input1 : i32, %cond : i1) -> i32 {
// RS-NEXT: "test.br"(%arg0)[^bb1] : (i32) -> ()
// NO-RS-NEXT: "test.br"(%arg0, %arg0)[^bb1] : (i32, i32) -> ()
"test.br"(%input1, %input1)[^bb1] : (i32, i32) -> ()
^bb1(%used_arg : i32, %unused_arg : i32):
return %used_arg : i32
}