[mlir][nfc] Minor cleanups in DeadCodeAnalysis #159232
Conversation
|
@llvm/pr-subscribers-mlir Author: Zhixun Tan (phisiart) Changes
One caveat is that, before this PR, in Full diff: https://github.com/llvm/llvm-project/pull/159232.diff 2 Files Affected:
diff --git a/mlir/include/mlir/Analysis/DataFlow/DeadCodeAnalysis.h b/mlir/include/mlir/Analysis/DataFlow/DeadCodeAnalysis.h
index c7c405e1423cb..ca7fdcb11b321 100644
--- a/mlir/include/mlir/Analysis/DataFlow/DeadCodeAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/DeadCodeAnalysis.h
@@ -16,6 +16,7 @@
#define MLIR_ANALYSIS_DATAFLOW_DEADCODEANALYSIS_H
#include "mlir/Analysis/DataFlowFramework.h"
+#include "mlir/Interfaces/ControlFlowInterfaces.h"
#include "mlir/IR/SymbolTable.h"
#include "llvm/ADT/SmallPtrSet.h"
#include <optional>
@@ -200,6 +201,13 @@ class DeadCodeAnalysis : public DataFlowAnalysis {
/// which are live from the current block.
void visitBranchOperation(BranchOpInterface branch);
+ /// Visit region branch edges from `predecessorOp` to a list of successors.
+ /// For each edge, mark the successor program point as executable, and record
+ /// the predecessor information in its `PredecessorState`.
+ void visitRegionBranchEdges(
+ RegionBranchOpInterface regionBranchOp, Operation *predecessorOp,
+ const SmallVector<RegionSuccessor> &successors);
+
/// Visit the given region branch operation, which defines regions, and
/// compute any necessary lattice state. This also resolves the lattice state
/// of both the operation results and any nested regions.
diff --git a/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp b/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
index 131c49c44171b..de1ed39ed4fdb 100644
--- a/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/DeadCodeAnalysis.cpp
@@ -444,30 +444,21 @@ void DeadCodeAnalysis::visitCallOperation(CallOpInterface call) {
/// Get the constant values of the operands of an operation. If any of the
/// constant value lattices are uninitialized, return std::nullopt to indicate
/// the analysis should bail out.
-static std::optional<SmallVector<Attribute>> getOperandValuesImpl(
- Operation *op,
- function_ref<const Lattice<ConstantValue> *(Value)> getLattice) {
+std::optional<SmallVector<Attribute>>
+DeadCodeAnalysis::getOperandValues(Operation *op) {
SmallVector<Attribute> operands;
operands.reserve(op->getNumOperands());
for (Value operand : op->getOperands()) {
- const Lattice<ConstantValue> *cv = getLattice(operand);
+ Lattice<ConstantValue> *cv = getOrCreate<Lattice<ConstantValue>>(operand);
+ cv->useDefSubscribe(this);
// If any of the operands' values are uninitialized, bail out.
if (cv->getValue().isUninitialized())
- return {};
+ return std::nullopt;
operands.push_back(cv->getValue().getConstantValue());
}
return operands;
}
-std::optional<SmallVector<Attribute>>
-DeadCodeAnalysis::getOperandValues(Operation *op) {
- return getOperandValuesImpl(op, [&](Value value) {
- auto *lattice = getOrCreate<Lattice<ConstantValue>>(value);
- lattice->useDefSubscribe(this);
- return lattice;
- });
-}
-
void DeadCodeAnalysis::visitBranchOperation(BranchOpInterface branch) {
LDBG() << "visitBranchOperation: "
<< OpWithFlags(branch.getOperation(), OpPrintingFlags().skipRegions());
@@ -498,23 +489,8 @@ void DeadCodeAnalysis::visitRegionBranchOperation(
SmallVector<RegionSuccessor> successors;
branch.getEntrySuccessorRegions(*operands, successors);
- for (const RegionSuccessor &successor : successors) {
- // The successor can be either an entry block or the parent operation.
- ProgramPoint *point =
- successor.getSuccessor()
- ? getProgramPointBefore(&successor.getSuccessor()->front())
- : getProgramPointAfter(branch);
- // Mark the entry block as executable.
- auto *state = getOrCreate<Executable>(point);
- propagateIfChanged(state, state->setToLive());
- LDBG() << "Marked region successor live: " << point;
- // Add the parent op as a predecessor.
- auto *predecessors = getOrCreate<PredecessorState>(point);
- propagateIfChanged(
- predecessors,
- predecessors->join(branch, successor.getSuccessorInputs()));
- LDBG() << "Added region branch as predecessor for successor: " << point;
- }
+
+ visitRegionBranchEdges(branch, branch.getOperation(), successors);
}
void DeadCodeAnalysis::visitRegionTerminator(Operation *op,
@@ -530,26 +506,30 @@ void DeadCodeAnalysis::visitRegionTerminator(Operation *op,
else
branch.getSuccessorRegions(op->getParentRegion(), successors);
- // Mark successor region entry blocks as executable and add this op to the
- // list of predecessors.
+ visitRegionBranchEdges(branch, op, successors);
+}
+
+void DeadCodeAnalysis::visitRegionBranchEdges(
+ RegionBranchOpInterface regionBranchOp, Operation *predecessorOp,
+ const SmallVector<RegionSuccessor> &successors) {
for (const RegionSuccessor &successor : successors) {
- PredecessorState *predecessors;
- if (Region *region = successor.getSuccessor()) {
- auto *state =
- getOrCreate<Executable>(getProgramPointBefore(®ion->front()));
- propagateIfChanged(state, state->setToLive());
- LDBG() << "Marked region entry block live for region: " << region;
- predecessors = getOrCreate<PredecessorState>(
- getProgramPointBefore(®ion->front()));
- } else {
- // Add this terminator as a predecessor to the parent op.
- predecessors =
- getOrCreate<PredecessorState>(getProgramPointAfter(branch));
- }
- propagateIfChanged(predecessors,
- predecessors->join(op, successor.getSuccessorInputs()));
- LDBG() << "Added region terminator as predecessor for successor: "
- << (successor.getSuccessor() ? "region entry" : "parent op");
+ // The successor can be either an entry block or the parent operation.
+ ProgramPoint *point =
+ successor.getSuccessor()
+ ? getProgramPointBefore(&successor.getSuccessor()->front())
+ : getProgramPointAfter(regionBranchOp);
+
+ // Mark the entry block as executable.
+ auto *state = getOrCreate<Executable>(point);
+ propagateIfChanged(state, state->setToLive());
+ LDBG() << "Marked region successor live: " << point;
+
+ // Add the parent op as a predecessor.
+ auto *predecessors = getOrCreate<PredecessorState>(point);
+ propagateIfChanged(
+ predecessors,
+ predecessors->join(predecessorOp, successor.getSuccessorInputs()));
+ LDBG() << "Added region branch as predecessor for successor: " << point;
}
}
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
phisiart
force-pushed
the
dead-code-analysis-cleanup
branch
from
d5bcab5 to
f35fd08
Compare
…ranchOperation,Terminator} into a new function DeadCodeAnalysis::visitRegionBranchEdges.
phisiart
force-pushed
the
dead-code-analysis-cleanup
branch
from
f35fd08 to
6e9c472
Compare
Agreed that this NFC is borderline. It is true that this is observable by some other analysis, but on the other hand, the existing behavior (i.e. before this PR) is buggy:
There is no good reason for this discrepancy anyway, so in that sense, this PR either fixes a subtle bug, or is truly NFC. |
Remove
getOperandValuesImplsince its only used once.Extract common logic from
DeadCodeAnalysis::visitRegion{BranchOperation,Terminator}into a new functionDeadCodeAnalysis::visitRegionBranchEdges.In particular, both functions do the following:
Detect live region branch edges (similar to CFGEdge);
For each edge, mark the successor program point as executable (so that subsequent program gets visited);
For each edge, store the information of the predecessor op and arguments (so that other analyses know what states to join into the successor program point).
One caveat is that, before this PR, in
visitRegionTerminator, the successor program point is only marked as live if it is the start of a block; after this PR, the successor program point is consistently marked as live regardless what it is, which makes the behavior equal tovisitBranchOperation. This minor fix improves consistency, but at this point it is still NFC, because the rest of the dataflow analysis framework only cares about liveness at block level, and the liveness information in the middle of a block isn't read anyway. This probably will change once early-exits are supported.