https://github.com/matthias-springer updated https://github.com/llvm/llvm-project/pull/169269
>From c1b8c55aa730c27e4cc5c9910e2303604f1d50ed Mon Sep 17 00:00:00 2001 From: Matthias Springer <[email protected]> Date: Mon, 24 Nov 2025 02:42:09 +0000 Subject: [PATCH 1/2] [mlir][Transforms] Fix crash in `-remove-dead-values` for private functions --- mlir/lib/Transforms/RemoveDeadValues.cpp | 38 ++++++++++++++++++++ mlir/test/Transforms/remove-dead-values.mlir | 11 ++++++ 2 files changed, 49 insertions(+) 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. >From a013bc2bd5e059c4701399cbe7ff720b4755870d Mon Sep 17 00:00:00 2001 From: Matthias Springer <[email protected]> Date: Fri, 28 Nov 2025 05:54:43 +0000 Subject: [PATCH 2/2] address comments --- mlir/include/mlir/Transforms/Passes.td | 1 + mlir/lib/Transforms/CMakeLists.txt | 1 + mlir/lib/Transforms/RemoveDeadValues.cpp | 65 +++++++++++--------- mlir/test/Transforms/remove-dead-values.mlir | 16 +++++ 4 files changed, 53 insertions(+), 30 deletions(-) diff --git a/mlir/include/mlir/Transforms/Passes.td b/mlir/include/mlir/Transforms/Passes.td index 28b4a01cf0ecd..55addfdb693e4 100644 --- a/mlir/include/mlir/Transforms/Passes.td +++ b/mlir/include/mlir/Transforms/Passes.td @@ -248,6 +248,7 @@ def RemoveDeadValues : Pass<"remove-dead-values"> { ``` }]; let constructor = "mlir::createRemoveDeadValuesPass()"; + let dependentDialects = ["ub::UBDialect"]; } def PrintIRPass : Pass<"print-ir"> { diff --git a/mlir/lib/Transforms/CMakeLists.txt b/mlir/lib/Transforms/CMakeLists.txt index 54b67f5c7a91e..06161293e907f 100644 --- a/mlir/lib/Transforms/CMakeLists.txt +++ b/mlir/lib/Transforms/CMakeLists.txt @@ -39,4 +39,5 @@ add_mlir_library(MLIRTransforms MLIRSideEffectInterfaces MLIRSupport MLIRTransformUtils + MLIRUBDialect ) diff --git a/mlir/lib/Transforms/RemoveDeadValues.cpp b/mlir/lib/Transforms/RemoveDeadValues.cpp index 9d4d24c39c116..e9ced064c9884 100644 --- a/mlir/lib/Transforms/RemoveDeadValues.cpp +++ b/mlir/lib/Transforms/RemoveDeadValues.cpp @@ -33,6 +33,7 @@ #include "mlir/Analysis/DataFlow/DeadCodeAnalysis.h" #include "mlir/Analysis/DataFlow/LivenessAnalysis.h" +#include "mlir/Dialect/UB/IR/UBOps.h" #include "mlir/IR/Builders.h" #include "mlir/IR/BuiltinAttributes.h" #include "mlir/IR/Dialect.h" @@ -141,33 +142,6 @@ 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, @@ -287,7 +261,12 @@ static SmallVector<OpOperand *> operandsToOpOperands(OperandRange operands) { static void processSimpleOp(Operation *op, RunLivenessAnalysis &la, DenseSet<Value> &nonLiveSet, RDVFinalCleanupList &cl) { - if (hasDead(op->getOperands(), nonLiveSet, la)) { + // Operations that have dead operands can be erased regardless of their + // side effects. The liveness analysis would not have marked an SSA value as + // "dead" if it had a side-effecting user that is reachable. + bool hasDeadOperand = + markLives(op->getOperands(), nonLiveSet, la).flip().any(); + if (hasDeadOperand) { LDBG() << "Simple op has dead operands, so the op must be dead: " << OpWithFlags(op, OpPrintingFlags().skipRegions()); assert(!hasLive(op->getResults(), nonLiveSet, la) && @@ -399,6 +378,8 @@ static void processFuncOp(FunctionOpInterface funcOp, Operation *module, // block other than the entry block, because every block has a terminator. for (Block &block : funcOp.getBlocks()) { Operation *returnOp = block.getTerminator(); + if (!returnOp->hasTrait<OpTrait::ReturnLike>()) + continue; if (returnOp && returnOp->getNumOperands() == numReturns) cl.operands.push_back({returnOp, nonLiveRets}); } @@ -738,7 +719,11 @@ static void processRegionBranchOp(RegionBranchOpInterface regionBranchOp, } /// Steps to process a `BranchOpInterface` operation: -/// Iterate through each successor block of `branchOp`. +/// +/// When a non-forwarded operand is dead (e.g., the condition value of a +/// conditional branch op), the entire operation is dead. +/// +/// Otherwise, iterate through each successor block of `branchOp`. /// (1) For each successor block, gather all operands from all successors. /// (2) Fetch their associated liveness analysis data and collect for future /// removal. @@ -749,7 +734,22 @@ static void processBranchOp(BranchOpInterface branchOp, RunLivenessAnalysis &la, DenseSet<Value> &nonLiveSet, RDVFinalCleanupList &cl) { LDBG() << "Processing branch op: " << *branchOp; + + // Check for dead non-forwarded operands. + BitVector deadNonForwardedOperands = + markLives(branchOp->getOperands(), nonLiveSet, la).flip(); unsigned numSuccessors = branchOp->getNumSuccessors(); + for (unsigned succIdx = 0; succIdx < numSuccessors; ++succIdx) { + SuccessorOperands successorOperands = + branchOp.getSuccessorOperands(succIdx); + // Remove all non-forwarded operands from the bit vector. + for (OpOperand &opOperand : successorOperands.getMutableForwardedOperands()) + deadNonForwardedOperands[opOperand.getOperandNumber()] = false; + } + if (deadNonForwardedOperands.any()) { + cl.operations.push_back(branchOp.getOperation()); + return; + } for (unsigned succIdx = 0; succIdx < numSuccessors; ++succIdx) { Block *successorBlock = branchOp->getSuccessor(succIdx); @@ -824,9 +824,14 @@ static void cleanUpDeadVals(RDVFinalCleanupList &list) { // 3. Operations LDBG() << "Cleaning up " << list.operations.size() << " operations"; - for (auto &op : list.operations) { + for (Operation *op : list.operations) { LDBG() << "Erasing operation: " << OpWithFlags(op, OpPrintingFlags().skipRegions()); + 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()); + } op->dropAllUses(); op->erase(); } diff --git a/mlir/test/Transforms/remove-dead-values.mlir b/mlir/test/Transforms/remove-dead-values.mlir index af157fc8bc5b0..71306676d48e9 100644 --- a/mlir/test/Transforms/remove-dead-values.mlir +++ b/mlir/test/Transforms/remove-dead-values.mlir @@ -698,3 +698,19 @@ func.func @op_block_have_dead_arg(%arg0: index, %arg1: index, %arg2: i1) { // CHECK-NEXT: return return } + +// ----- + +// CHECK-LABEL: func private @remove_dead_branch_op() +// CHECK-NEXT: ub.unreachable +// CHECK-NEXT: ^{{.*}}: +// CHECK-NEXT: return +// CHECK-NEXT: ^{{.*}}: +// CHECK-NEXT: return +func.func private @remove_dead_branch_op(%c: i1, %arg0: i64, %arg1: i64) -> (i64) { + cf.cond_br %c, ^bb1, ^bb2 +^bb1: + return %arg0 : i64 +^bb2: + return %arg1 : i64 +} _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
