[mlir][Transforms] Fix crash in `-remove-dead-values` on private functions #169269

matthias-springer · 2025-11-24T02:48:20Z

This commit fixes two crashes in the -remove-dead-values pass related to private functions.

Private functions are considered entirely "dead" by the liveness analysis, which drives the -remove-dead-values pass.

The -remove-dead-values pass removes dead block arguments from private functions. Private functions are entirely dead, so all of their block arguments are removed. However, the pass did not correctly update all users of these dropped block arguments.

A side-effecting operation must be removed if one of its operands is dead. Otherwise, the operation would end up with a NULL operand. Note: The liveness analysis would not have marked an SSA value as "dead" if it had a reachable side-effecting users. (Therefore, it is safe to erase such side-effecting operations.)
A branch operation must be removed if one of its non-forwarded operands is dead. (E.g., the condition value of a cf.cond_br.) Whenever a terminator is removed, a ub.unrechable operation is inserted. This fixes [MLIR] RemoveDeadValues pass errors when run on private function #158760.

Depends on #169872.

llvmbot · 2025-11-24T02:48:52Z

@llvm/pr-subscribers-mlir-ub
@llvm/pr-subscribers-mlir-spirv

@llvm/pr-subscribers-mlir-core

Author: Matthias Springer (matthias-springer)

Changes

This commit fixes a crash in the -remove-dead-values pass. This pass removes dead block arguments from functions. However, it did not remove side-effecting operations, even if they are dead. This lead to invalid IR: a block argument was removed, but some of its uses survived.

With this commit, a "simple" operation is erased when it has a dead operand. If the operation were not dead, the liveness analysis would not have marked one of its operands as "dead".

Full diff: https://github.com/llvm/llvm-project/pull/169269.diff

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+38)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+11)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index 989c614ef6617..9d4d24c39c116 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -141,6 +141,33 @@ static bool hasLive(ValueRange values, const DenseSet<Value> &nonLiveSet,
   return false;
 }
 
+/// Return true iff at least one value in `values` is dead, given the liveness
+/// information in `la`.
+static bool hasDead(ValueRange values, const DenseSet<Value> &nonLiveSet,
+                    RunLivenessAnalysis &la) {
+  for (Value value : values) {
+    if (nonLiveSet.contains(value)) {
+      LDBG() << "Value " << value << " is already marked non-live (dead)";
+      return true;
+    }
+
+    const Liveness *liveness = la.getLiveness(value);
+    if (!liveness) {
+      LDBG() << "Value " << value
+             << " has no liveness info, conservatively considered live";
+      continue;
+    }
+    if (liveness->isLive) {
+      LDBG() << "Value " << value << " is live according to liveness analysis";
+      continue;
+    } else {
+      LDBG() << "Value " << value << " is dead according to liveness analysis";
+      return true;
+    }
+  }
+  return false;
+}
+
 /// Return a BitVector of size `values.size()` where its i-th bit is 1 iff the
 /// i-th value in `values` is live, given the liveness information in `la`.
 static BitVector markLives(ValueRange values, const DenseSet<Value> &nonLiveSet,
@@ -260,6 +287,17 @@ static SmallVector<OpOperand *> operandsToOpOperands(OperandRange operands) {
 static void processSimpleOp(Operation *op, RunLivenessAnalysis &la,
                             DenseSet<Value> &nonLiveSet,
                             RDVFinalCleanupList &cl) {
+  if (hasDead(op->getOperands(), nonLiveSet, la)) {
+    LDBG() << "Simple op has dead operands, so the op must be dead: "
+           << OpWithFlags(op, OpPrintingFlags().skipRegions());
+    assert(!hasLive(op->getResults(), nonLiveSet, la) &&
+           "expected the op to have no live results");
+    cl.operations.push_back(op);
+    collectNonLiveValues(nonLiveSet, op->getResults(),
+                         BitVector(op->getNumResults(), true));
+    return;
+  }
+
   if (!isMemoryEffectFree(op) || hasLive(op->getResults(), nonLiveSet, la)) {
     LDBG() << "Simple op is not memory effect free or has live results, "
               "preserving it: "
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 4bae85dcf4f7d..af157fc8bc5b0 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -118,6 +118,17 @@ func.func @main(%arg0 : i32) {
 
 // -----
 
+// CHECK-LABEL: func.func private @clean_func_op_remove_side_effecting_op() {
+// CHECK-NEXT:    return
+// CHECK-NEXT:  }
+func.func private @clean_func_op_remove_side_effecting_op(%arg0: i32) -> (i32) {
+  // vector.print has a side effect but the op is dead.
+  vector.print %arg0 : i32
+  return %arg0 : i32
+}
+
+// -----
+
 // %arg0 is not live because it is never used. %arg1 is not live because its
 // user `arith.addi` doesn't have any uses and the value that it is forwarded to
 // (%non_live_0) also doesn't have any uses.

llvmbot · 2025-11-24T02:48:53Z

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

This commit fixes a crash in the -remove-dead-values pass. This pass removes dead block arguments from functions. However, it did not remove side-effecting operations, even if they are dead. This lead to invalid IR: a block argument was removed, but some of its uses survived.

With this commit, a "simple" operation is erased when it has a dead operand. If the operation were not dead, the liveness analysis would not have marked one of its operands as "dead".

Full diff: https://github.com/llvm/llvm-project/pull/169269.diff

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+38)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+11)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index 989c614ef6617..9d4d24c39c116 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -141,6 +141,33 @@ static bool hasLive(ValueRange values, const DenseSet<Value> &nonLiveSet,
   return false;
 }
 
+/// Return true iff at least one value in `values` is dead, given the liveness
+/// information in `la`.
+static bool hasDead(ValueRange values, const DenseSet<Value> &nonLiveSet,
+                    RunLivenessAnalysis &la) {
+  for (Value value : values) {
+    if (nonLiveSet.contains(value)) {
+      LDBG() << "Value " << value << " is already marked non-live (dead)";
+      return true;
+    }
+
+    const Liveness *liveness = la.getLiveness(value);
+    if (!liveness) {
+      LDBG() << "Value " << value
+             << " has no liveness info, conservatively considered live";
+      continue;
+    }
+    if (liveness->isLive) {
+      LDBG() << "Value " << value << " is live according to liveness analysis";
+      continue;
+    } else {
+      LDBG() << "Value " << value << " is dead according to liveness analysis";
+      return true;
+    }
+  }
+  return false;
+}
+
 /// Return a BitVector of size `values.size()` where its i-th bit is 1 iff the
 /// i-th value in `values` is live, given the liveness information in `la`.
 static BitVector markLives(ValueRange values, const DenseSet<Value> &nonLiveSet,
@@ -260,6 +287,17 @@ static SmallVector<OpOperand *> operandsToOpOperands(OperandRange operands) {
 static void processSimpleOp(Operation *op, RunLivenessAnalysis &la,
                             DenseSet<Value> &nonLiveSet,
                             RDVFinalCleanupList &cl) {
+  if (hasDead(op->getOperands(), nonLiveSet, la)) {
+    LDBG() << "Simple op has dead operands, so the op must be dead: "
+           << OpWithFlags(op, OpPrintingFlags().skipRegions());
+    assert(!hasLive(op->getResults(), nonLiveSet, la) &&
+           "expected the op to have no live results");
+    cl.operations.push_back(op);
+    collectNonLiveValues(nonLiveSet, op->getResults(),
+                         BitVector(op->getNumResults(), true));
+    return;
+  }
+
   if (!isMemoryEffectFree(op) || hasLive(op->getResults(), nonLiveSet, la)) {
     LDBG() << "Simple op is not memory effect free or has live results, "
               "preserving it: "
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index 4bae85dcf4f7d..af157fc8bc5b0 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -118,6 +118,17 @@ func.func @main(%arg0 : i32) {
 
 // -----
 
+// CHECK-LABEL: func.func private @clean_func_op_remove_side_effecting_op() {
+// CHECK-NEXT:    return
+// CHECK-NEXT:  }
+func.func private @clean_func_op_remove_side_effecting_op(%arg0: i32) -> (i32) {
+  // vector.print has a side effect but the op is dead.
+  vector.print %arg0 : i32
+  return %arg0 : i32
+}
+
+// -----
+
 // %arg0 is not live because it is never used. %arg1 is not live because its
 // user `arith.addi` doesn't have any uses and the value that it is forwarded to
 // (%non_live_0) also doesn't have any uses.

mlir/lib/Transforms/RemoveDeadValues.cpp

mlir/test/Transforms/remove-dead-values.mlir

linuxlonelyeagle · 2025-11-24T03:33:53Z

I'm thinking that in this case, shouldn't we save the print? I believe we should save the print

matthias-springer · 2025-11-24T04:27:44Z

I'm thinking that in this case, shouldn't we save the print? I believe we should save the print

Why would you save the vector.print? In that case, we cannot remove the block argument from the function.

linuxlonelyeagle · 2025-11-24T06:25:02Z

I'm thinking that in this case, shouldn't we save the print? I believe we should save the print

Why would you save the vector.print? In that case, we cannot remove the block argument from the function.

The function added in the test file is the print function parameter, so we should save vector.print.

In that case, we cannot remove the block argument from the function.

Yes. I just had a thought: since the data flow analysis framework relies on dead-code analysis, a private function that is not called by any other public function will not be fully traversed(Since dead code analysis marks it as dead code, the data flow analysis framework will not reach it.). Here it will cause the function's parameter to be dead.

You can see following example is run well.

// $ mlir-opt test.mlir -remove-dead-values
func.func private @clean_func_op_remove_side_effecting_op(%arg0: i32) -> (i32) {
  // vector.print has a side effect but the op is dead.
  vector.print %arg0 : i32
  return %arg0 : i32
}

func.func @main(%arg: i32) {
  func.call @clean_func_op_remove_side_effecting_op(%arg) : (i32) -> (i32)
  return
}

More example. #161471 .Previously, I suggested that the fix should involve directly removing the redundant private function, wrap the symbol-dce functionality in a function-based pass, enabling us to use it in other passes—including the remove-dead-code pass. But @ftynse felt it was not appropriate. Unfortunately, this bug still hasn't been fixed today.

ftynse

Indeed, I think we shouldn't just remove side-effecting operations, this changes the observable behavior of the program. Maybe in some separate -remove-otherwise-dead-side-effecting-operations-i'm-sure-what-i'm-doing pass if really needed. Prints may be innocuous, but this could also remove cf.assert %false or a write to invalid memory location, rendering an incorrect program correct.

mlir/lib/Transforms/RemoveDeadValues.cpp

matthias-springer · 2025-11-24T09:57:55Z

Why does it change the observable behavior when dead operations are removed?

mlir/test/Transforms/remove-dead-values.mlir

matthias-springer · 2025-11-26T07:31:36Z

@ftynse @joker-eph @linuxlonelyeagle What are the next steps here? Are you convinced that this fix is correct? If not, can you think of a case where this fix is incorrect?

Summary:

When you remove an SSA value, you must also remove all uses of that SSA value. Otherwise, you produce invalid IR. (That's the bug that I'm fixing in this PR.)
-remove-dead-values removes only dead SSA values, as indicated by a liveness analysis.
LivenessAnalysis marks all operands of side-effecting ops as "live" [1]. The only exception are ops that are not reachable [2].
If an operand was marked as "dead" but has side effects, we can conclude that the operation must be unreachable.
Unreachable operations can be removed without changing the semantics of the program.
(Private functions are dead unless they are called from a public function.)

[1]

//===----------------------------------------------------------------------===//
// LivenessAnalysis
//===----------------------------------------------------------------------===//

/// For every value, liveness analysis determines whether or not it is "live".
///
/// A value is considered "live" iff it:
///   (1) has memory effects OR
// ...

[2]

RunLivenessAnalysis::RunLivenessAnalysis(Operation *op) {
  // ...
  // The framework doesn't visit operations in dead blocks, so we need to
  // explicitly mark them as dead.

linuxlonelyeagle · 2025-11-26T12:18:50Z

Thank you for inviting me to participate in the discussion.

`` marks all operands of side-effecting ops as "live" [1]. The only exception are ops that are not reachable [2].
@ftynse @joker-eph @linuxlonelyeagle What are the next steps here? Are you convinced that this fix is correct? If not, can you think of a case where this fix is incorrect?

Summary:

When you remove an SSA value, you must also remove all uses of that SSA value. Otherwise, you produce invalid IR. (That's the bug that I'm fixing in this PR.)

#158760, The problem you are solving should be similar to this one.

(Private functions are dead unless they are called from a public function.)

I have always suggested deleting useless private functions directly and introducing this feature in remove-dead-values.
This function hasn't been called yet, has it? Since we can delete useless values, why can't we delete functions(use -symbol-dce)?
This idea may not be very conservative(I admit that this idea is very radical). Let's first remove the useless functions, they will work very well.
We can solve this problem once and for all because we can now only deal with meaningful IR.🙏

matthias-springer · 2025-11-27T09:19:17Z

I took a look at #158760. It is indeed similar. It's the same problem that I am fixing here, but for branch ops. My fix from this PR here won't help because the op is not a "simple" op.

I see 5 options to fix issues such as #158760 and the one from this PR:

Remove unreachable functions/symbols at the beginning. (As you suggested.)
Fix the way operations are erased, similar to what this PR does.
When erasing an SSA value that still has uses, insert a ub.poison operation.
~~Add an option to the liveness analysis to treat private functions like public functions.~~ edit: This by itself is not enough because block arguments of public functions can still be dead.
Change -remove-dead-values such that only those operations are visited that were visited by the dataflow analysis.

I am not very happy with option 1 because it conceptually does not fit in well with -remove-dead-values. The -remove-dead-values pass is driven by a data flow analysis and symbol/function DCE cannot be done with a data flow analysis. We could call the symbol DCE pass from this pass, but it would be nice (design-wise) to avoid calling another pass/functionality that is not fully dataflow-driven.

As for option 2, the problem in #158760 is that the condition value of a cf.cond_br is dead. We cannot just remove that op because the basic block would loose its terminator. ~~Maybe we could insert an unconditional cf.br op. (Difficult because we match for BranchOpInterface and not specific ops.) Or we could insert a ub.poison for the condition.~~ edit: We can insert ub.unreachable.

Which brings me to option 3 ~~, which I would like to explore a bit more...~~ edit: This seems more complicated than I thought.

joker-eph · 2025-11-27T14:30:38Z

We cannot just remove that op because the basic block would loose its terminator. Maybe we could insert an unconditional cf.br op. (Difficult because we match for BranchOpInterface and not specific ops.) Or we could insert a ub.poison for the condition.

Seems to me that this is a case where we would insert a "unreachable" terminator instead?

matthias-springer · 2025-11-28T06:03:12Z

Seems to me that this is a case where we would insert a "unreachable" terminator instead?

I like this approach. This allows us to fix the -remove-dead-values pass without having to run a second analysis such as "finding dead symbols" -- the pass stays relatively simple.

I updated the pull request accordingly.

joker-eph · 2025-11-28T09:35:19Z

mlir/lib/Transforms/RemoveDeadValues.cpp

+    if (op->hasTrait<OpTrait::IsTerminator>()) {
+      // When erasing a terminator, insert an unreachable op in its place.
+      OpBuilder b(op);
+      ub::UnreachableOp::create(b, op->getLoc());
+    }


Suggested change

if (op->hasTrait<OpTrait::IsTerminator>()) {

// When erasing a terminator, insert an unreachable op in its place.

OpBuilder b(op);

ub::UnreachableOp::create(b, op->getLoc());

}

// When erasing a terminator, insert an unreachable op in its place.

if (op->hasTrait<OpTrait::IsTerminator>())

ub::UnreachableOp::create(OpBuilder{op}, op->getLoc());

Nit: simplifying a bit.

This is giving me a compilation error:

llvm-project/mlir/lib/Transforms/RemoveDeadValues.cpp:832:7: error: no matching function for call to 'create' 832 | ub::UnreachableOp::create(OpBuilder{op}, op->getLoc()); | ^~~~~~~~~~~~~~~~~~~~~~~~~ llvm-project/build/tools/mlir/include/mlir/Dialect/UB/IR/UBOps.h.inc:377:24: note: candidate function not viable: expects an lvalue for 1st argument 377 | static UnreachableOp create(::mlir::OpBuilder &builder, ::mlir::Location location); | ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~

That's annoying, we should make it so that it is possible.

(not a blocker here)

linuxlonelyeagle

I also think it's good to do this.

…ctions

matthias-springer · 2025-11-28T11:39:51Z

@ftynse Can I merge this?

llvmbot added mlir:core MLIR Core Infrastructure mlir labels Nov 24, 2025

matthias-springer requested a review from linuxlonelyeagle November 24, 2025 02:48

matthias-springer requested review from joker-eph and srishti-pm November 24, 2025 02:49

linuxlonelyeagle reviewed Nov 24, 2025

View reviewed changes

mlir/lib/Transforms/RemoveDeadValues.cpp Outdated Show resolved Hide resolved

linuxlonelyeagle reviewed Nov 24, 2025

View reviewed changes

mlir/test/Transforms/remove-dead-values.mlir Show resolved Hide resolved

ftynse requested changes Nov 24, 2025

View reviewed changes

mlir/lib/Transforms/RemoveDeadValues.cpp Outdated Show resolved Hide resolved

joker-eph reviewed Nov 24, 2025

View reviewed changes

mlir/test/Transforms/remove-dead-values.mlir Show resolved Hide resolved

matthias-springer force-pushed the users/matthias-springer/fix_crash_rdv branch from 78783ae to d2349ca Compare November 28, 2025 05:55

matthias-springer requested review from antiagainst and kuhar as code owners November 28, 2025 05:55

llvmbot added mlir:spirv mlir:ub labels Nov 28, 2025

matthias-springer changed the base branch from main to users/matthias-springer/ub_unreachable November 28, 2025 05:55

matthias-springer force-pushed the users/matthias-springer/fix_crash_rdv branch from d2349ca to a013bc2 Compare November 28, 2025 06:07

matthias-springer requested review from ftynse, joker-eph and linuxlonelyeagle November 28, 2025 06:07

joker-eph reviewed Nov 28, 2025

View reviewed changes

joker-eph approved these changes Nov 28, 2025

View reviewed changes

linuxlonelyeagle approved these changes Nov 28, 2025

View reviewed changes

Base automatically changed from users/matthias-springer/ub_unreachable to main November 28, 2025 10:35

matthias-springer added 2 commits November 28, 2025 11:37

[mlir][Transforms] Fix crash in -remove-dead-values for private fun…

378dcbd

…ctions

address comments

c08ef69

matthias-springer force-pushed the users/matthias-springer/fix_crash_rdv branch from a013bc2 to c08ef69 Compare November 28, 2025 11:37

[mlir][Transforms] Fix crash in -remove-dead-values on private functions #169269

Are you sure you want to change the base?

[mlir][Transforms] Fix crash in -remove-dead-values on private functions #169269

Conversation

matthias-springer commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Nov 24, 2025

Uh oh!

Uh oh!

Uh oh!

linuxlonelyeagle commented Nov 24, 2025

Uh oh!

matthias-springer commented Nov 24, 2025

Uh oh!

linuxlonelyeagle commented Nov 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ftynse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matthias-springer commented Nov 24, 2025

Uh oh!

Uh oh!

matthias-springer commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linuxlonelyeagle commented Nov 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthias-springer commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joker-eph commented Nov 27, 2025

Uh oh!

matthias-springer commented Nov 28, 2025

Uh oh!

joker-eph Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

matthias-springer Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

joker-eph Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

joker-eph Nov 28, 2025

Choose a reason for hiding this comment

Uh oh!

linuxlonelyeagle left a comment

Choose a reason for hiding this comment

Uh oh!

matthias-springer commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

[mlir][Transforms] Fix crash in `-remove-dead-values` on private functions #169269

[mlir][Transforms] Fix crash in `-remove-dead-values` on private functions #169269

matthias-springer commented Nov 24, 2025 •

edited

Loading

llvmbot commented Nov 24, 2025 •

edited

Loading

linuxlonelyeagle commented Nov 24, 2025 •

edited

Loading

matthias-springer commented Nov 26, 2025 •

edited

Loading

linuxlonelyeagle commented Nov 26, 2025 •

edited

Loading

matthias-springer commented Nov 27, 2025 •

edited

Loading