[mlir] Make remove-dead-values remove block and successorOperands before delete ops by linuxlonelyeagle · Pull Request #166766 · llvm/llvm-project

linuxlonelyeagle · 2025-11-06T12:48:20Z

Reland #165725, fix the Failed test by removing successor operands before delete operations. Following the deletion of cond.branch, its successor operands will subsequently be removed.

llvmbot · 2025-11-06T12:48:56Z

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-core

Author: lonely eagle (linuxlonelyeagle)

Changes

Reland #165725, fix the Failed test by removing successor operands before delete operations. Following the deletion of cond.branch, its successor operands will subsequently be removed.

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

2 Files Affected:

(modified) mlir/lib/Transforms/RemoveDeadValues.cpp (+46-47)
(modified) mlir/test/Transforms/remove-dead-values.mlir (+15)

diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp
index 41f3f9d76a3b1..872ec9738f26d 100644
--- a/mlir/lib/Transforms/RemoveDeadValues.cpp
+++ b/mlir/lib/Transforms/RemoveDeadValues.cpp
@@ -742,7 +742,48 @@ static void processBranchOp(BranchOpInterface branchOp, RunLivenessAnalysis &la,
 static void cleanUpDeadVals(RDVFinalCleanupList &list) {
   LDBG() << "Starting cleanup of dead values...";
 
-  // 1. Operations
+  // 1. Blocks
+  LDBG() << "Cleaning up " << list.blocks.size() << " block argument lists";
+  for (auto &b : list.blocks) {
+    // blocks that are accessed via multiple codepaths processed once
+    if (b.b->getNumArguments() != b.nonLiveArgs.size())
+      continue;
+    LDBG() << "Erasing " << b.nonLiveArgs.count()
+           << " non-live arguments from block: " << b.b;
+    // it iterates backwards because erase invalidates all successor indexes
+    for (int i = b.nonLiveArgs.size() - 1; i >= 0; --i) {
+      if (!b.nonLiveArgs[i])
+        continue;
+      LDBG() << "  Erasing block argument " << i << ": " << b.b->getArgument(i);
+      b.b->getArgument(i).dropAllUses();
+      b.b->eraseArgument(i);
+    }
+  }
+
+  // 2. Successor Operands
+  LDBG() << "Cleaning up " << list.successorOperands.size()
+         << " successor operand lists";
+  for (auto &op : list.successorOperands) {
+    SuccessorOperands successorOperands =
+        op.branch.getSuccessorOperands(op.successorIndex);
+    // blocks that are accessed via multiple codepaths processed once
+    if (successorOperands.size() != op.nonLiveOperands.size())
+      continue;
+    LDBG() << "Erasing " << op.nonLiveOperands.count()
+           << " non-live successor operands from successor "
+           << op.successorIndex << " of branch: "
+           << OpWithFlags(op.branch, OpPrintingFlags().skipRegions());
+    // it iterates backwards because erase invalidates all successor indexes
+    for (int i = successorOperands.size() - 1; i >= 0; --i) {
+      if (!op.nonLiveOperands[i])
+        continue;
+      LDBG() << "  Erasing successor operand " << i << ": "
+             << successorOperands[i];
+      successorOperands.erase(i);
+    }
+  }
+
+  // 3. Operations
   LDBG() << "Cleaning up " << list.operations.size() << " operations";
   for (auto &op : list.operations) {
     LDBG() << "Erasing operation: "
@@ -751,14 +792,14 @@ static void cleanUpDeadVals(RDVFinalCleanupList &list) {
     op->erase();
   }
 
-  // 2. Values
+  // 4. Values
   LDBG() << "Cleaning up " << list.values.size() << " values";
   for (auto &v : list.values) {
     LDBG() << "Dropping all uses of value: " << v;
     v.dropAllUses();
   }
 
-  // 3. Functions
+  // 5. Functions
   LDBG() << "Cleaning up " << list.functions.size() << " functions";
   // Record which function arguments were erased so we can shrink call-site
   // argument segments for CallOpInterface operations (e.g. ops using
@@ -780,7 +821,7 @@ static void cleanUpDeadVals(RDVFinalCleanupList &list) {
     (void)f.funcOp.eraseResults(f.nonLiveRets);
   }
 
-  // 4. Operands
+  // 6. Operands
   LDBG() << "Cleaning up " << list.operands.size() << " operand lists";
   for (OperationToCleanup &o : list.operands) {
     // Handle call-specific cleanup only when we have a cached callee reference.
@@ -822,7 +863,7 @@ static void cleanUpDeadVals(RDVFinalCleanupList &list) {
     }
   }
 
-  // 5. Results
+  // 7. Results
   LDBG() << "Cleaning up " << list.results.size() << " result lists";
   for (auto &r : list.results) {
     LDBG() << "Erasing " << r.nonLive.count()
@@ -830,48 +871,6 @@ static void cleanUpDeadVals(RDVFinalCleanupList &list) {
            << OpWithFlags(r.op, OpPrintingFlags().skipRegions());
     dropUsesAndEraseResults(r.op, r.nonLive);
   }
-
-  // 6. Blocks
-  LDBG() << "Cleaning up " << list.blocks.size() << " block argument lists";
-  for (auto &b : list.blocks) {
-    // blocks that are accessed via multiple codepaths processed once
-    if (b.b->getNumArguments() != b.nonLiveArgs.size())
-      continue;
-    LDBG() << "Erasing " << b.nonLiveArgs.count()
-           << " non-live arguments from block: " << b.b;
-    // it iterates backwards because erase invalidates all successor indexes
-    for (int i = b.nonLiveArgs.size() - 1; i >= 0; --i) {
-      if (!b.nonLiveArgs[i])
-        continue;
-      LDBG() << "  Erasing block argument " << i << ": " << b.b->getArgument(i);
-      b.b->getArgument(i).dropAllUses();
-      b.b->eraseArgument(i);
-    }
-  }
-
-  // 7. Successor Operands
-  LDBG() << "Cleaning up " << list.successorOperands.size()
-         << " successor operand lists";
-  for (auto &op : list.successorOperands) {
-    SuccessorOperands successorOperands =
-        op.branch.getSuccessorOperands(op.successorIndex);
-    // blocks that are accessed via multiple codepaths processed once
-    if (successorOperands.size() != op.nonLiveOperands.size())
-      continue;
-    LDBG() << "Erasing " << op.nonLiveOperands.count()
-           << " non-live successor operands from successor "
-           << op.successorIndex << " of branch: "
-           << OpWithFlags(op.branch, OpPrintingFlags().skipRegions());
-    // it iterates backwards because erase invalidates all successor indexes
-    for (int i = successorOperands.size() - 1; i >= 0; --i) {
-      if (!op.nonLiveOperands[i])
-        continue;
-      LDBG() << "  Erasing successor operand " << i << ": "
-             << successorOperands[i];
-      successorOperands.erase(i);
-    }
-  }
-
   LDBG() << "Finished cleanup of dead values";
 }
 
diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir
index e7304505c809e..8b5ccdcf204dd 100644
--- a/mlir/test/Transforms/remove-dead-values.mlir
+++ b/mlir/test/Transforms/remove-dead-values.mlir
@@ -674,3 +674,18 @@ func.func @dead_value_loop_ivs_no_result(%lb: index, %ub: index, %step: index, %
   }
   return
 }
+
+// -----
+
+// CHECK-LABEL: func @op_block_have_dead_arg
+func.func @op_block_have_dead_arg(%arg0: index, %arg1: index, %arg2: index, %arg3: i1) {
+  scf.for %iv = %arg0 to %arg1 step %arg2 {
+    scf.execute_region {
+      cf.cond_br %arg3, ^bb1(%arg0 : index), ^bb1(%arg1 : index)
+    ^bb1(%0: index):
+        scf.yield
+    }
+  }
+// CHECK-NEXT: return
+  return
+}

linuxlonelyeagle · 2025-11-06T12:52:47Z

See https://lab.llvm.org/buildbot/#/builders/164/builds/15324

linuxlonelyeagle · 2025-11-12T02:21:39Z

ping for review @joker-eph , thank you.

matthias-springer · 2025-11-18T04:22:17Z

+  // 1. Blocks
+  LDBG() << "Cleaning up " << list.blocks.size() << " block argument lists";
+  for (auto &b : list.blocks) {
+    // blocks that are accessed via multiple codepaths processed once


nit: Can you fix the grammar here?

matthias-springer · 2025-11-18T04:23:04Z

+  LDBG() << "Cleaning up " << list.blocks.size() << " block argument lists";
+  for (auto &b : list.blocks) {
+    // blocks that are accessed via multiple codepaths processed once
+    if (b.b->getNumArguments() != b.nonLiveArgs.size())


I don't see how this check is related to the above comment: blocks that are accessed via multiple codepaths processed once

Can the same block appear multiple times in list.blocks?

When running ninja check-mlir, it deletes a block multiple times during testing.

Do you know why that's the case? It would be great to mention that in a comment.

matthias-springer · 2025-11-18T04:28:09Z

+  for (auto &op : list.successorOperands) {
+    SuccessorOperands successorOperands =
+        op.branch.getSuccessorOperands(op.successorIndex);
+    // blocks that are accessed via multiple codepaths processed once


nit: Can you fix the grammar here?

matthias-springer · 2025-11-18T04:28:40Z

+    SuccessorOperands successorOperands =
+        op.branch.getSuccessorOperands(op.successorIndex);
+    // blocks that are accessed via multiple codepaths processed once
+    if (successorOperands.size() != op.nonLiveOperands.size())


Same question about the size check here as above. Why would the sizes not match?

If you only use mlir-opt, it will match, bug if you use it in the remove-dead-valuse.mlir, it will remove a block multi time, the first time is match, the second time it not match.
if you delete the following code.

if (successorOperands.size() != op.nonLiveOperands.size())

or

if (b.b->getNumArguments() != b.nonLiveArgs.size())

it will crash. How do you think it? cc: @joker-eph @ftynse .

Note @matthias-springer : this is mostly code motion from existing code (see the left side of the diff)

github-actions · 2025-11-18T09:50:36Z

🐧 Linux x64 Test Results

7081 tests passed
594 tests skipped

matthias-springer · 2025-11-18T10:18:44Z

  LDBG() << "Starting cleanup of dead values...";

-  // 1. Operations
+  // 1. Blocks, We must remove the block arguments and successor operands before


So this is a case of "delete stuff inside the operation before the operation itself"? Do we also guarantee that when erasing operations?

"delete stuff inside the operation before the operation itself"?
yes.
"Do we also guarantee that when erasing operations?"
now we remove block argument and successorOperands before delete ops, I think we have guaranteed it.

joker-eph

LG, try to fix @matthias-springer comments though

linuxlonelyeagle requested review from ftynse and joker-eph November 6, 2025 12:48

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

matthias-springer reviewed Nov 18, 2025

View reviewed changes

linuxlonelyeagle added 4 commits November 18, 2025 07:21

Make-remove-dead-values-remove-block-first

08af4dc

update test.

6add201

move delete delete SuccessorOperands before delete op.

42ca93c

rebase code and update code.

0d6a385

linuxlonelyeagle force-pushed the make-remove-dead-values-remove-block-first branch from 06d7450 to 0d6a385 Compare November 18, 2025 09:38

matthias-springer reviewed Nov 18, 2025

View reviewed changes

joker-eph approved these changes Nov 18, 2025

View reviewed changes

matthias-springer approved these changes Nov 20, 2025

View reviewed changes

linuxlonelyeagle merged commit 765208b into llvm:main Nov 20, 2025
10 checks passed

Men-cotton mentioned this pull request Apr 25, 2026

[MLIR] Assertion violation in -remove-dead-values pass: failure message `(start + length) <= owner->getNumOperands() && "invalid range #131449

Closed

Conversation

linuxlonelyeagle commented Nov 6, 2025

Uh oh!

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

Uh oh!

Uh oh!

linuxlonelyeagle commented Nov 6, 2025

Uh oh!

linuxlonelyeagle commented Nov 12, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Nov 18, 2025

🐧 Linux x64 Test Results

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joker-eph left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

llvmbot commented Nov 6, 2025 •

edited

Loading