Skip to content

Commit 3a439e2

Browse files
[mlir][dataflow] disallow outside use of propagateIfChanged for DataFlowSolver (#120885)
Detailed writeup is in google/heir#1153. See also #120881. In short, `propagateIfChanged` is used outside of the `DataFlowAnalysis` scope, because it is public, but it does not propagate as expected as the `DataFlowSolver` has stopped running. To solve such misuse, `propagateIfChanged` should be made protected/private. For downstream users affected by this, to correctly propagate the change, the Analysis should be re-run (check #120881) instead of just a `propagateIfChanged` The change to `IntegerRangeAnalysis` is just a expansion of the `solver->propagateIfChanged`. The `Lattice` has already been updated by the `join`. Propagation is done by `onUpdate`. Cc @Mogball for review
1 parent 3c64f86 commit 3a439e2

File tree

2 files changed

+12
-0
lines changed

2 files changed

+12
-0
lines changed

mlir/include/mlir/Analysis/DataFlowFramework.h

+5
Original file line numberDiff line numberDiff line change
@@ -401,6 +401,8 @@ class DataFlowSolver {
401401

402402
/// Propagate an update to an analysis state if it changed by pushing
403403
/// dependent work items to the back of the queue.
404+
/// This should only be used when DataFlowSolver is running.
405+
/// Otherwise, the solver won't process the work items.
404406
void propagateIfChanged(AnalysisState *state, ChangeResult changed);
405407

406408
/// Get the configuration of the solver.
@@ -410,6 +412,9 @@ class DataFlowSolver {
410412
/// Configuration of the dataflow solver.
411413
DataFlowConfig config;
412414

415+
/// The solver is working on the worklist.
416+
bool isRunning = false;
417+
413418
/// The solver's work queue. Work items can be inserted to the front of the
414419
/// queue to be processed greedily, speeding up computations that otherwise
415420
/// quickly degenerate to quadratic due to propagation of state updates.

mlir/lib/Analysis/DataFlowFramework.cpp

+7
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
#include "mlir/IR/Location.h"
1111
#include "mlir/IR/Operation.h"
1212
#include "mlir/IR/Value.h"
13+
#include "llvm/ADT/ScopeExit.h"
1314
#include "llvm/ADT/iterator.h"
1415
#include "llvm/Config/abi-breaking.h"
1516
#include "llvm/Support/Casting.h"
@@ -104,6 +105,10 @@ Location LatticeAnchor::getLoc() const {
104105
//===----------------------------------------------------------------------===//
105106

106107
LogicalResult DataFlowSolver::initializeAndRun(Operation *top) {
108+
// Enable enqueue to the worklist.
109+
isRunning = true;
110+
auto guard = llvm::make_scope_exit([&]() { isRunning = false; });
111+
107112
// Initialize the analyses.
108113
for (DataFlowAnalysis &analysis : llvm::make_pointee_range(childAnalyses)) {
109114
DATAFLOW_DEBUG(llvm::dbgs()
@@ -134,6 +139,8 @@ LogicalResult DataFlowSolver::initializeAndRun(Operation *top) {
134139

135140
void DataFlowSolver::propagateIfChanged(AnalysisState *state,
136141
ChangeResult changed) {
142+
assert(isRunning &&
143+
"DataFlowSolver is not running, should not use propagateIfChanged");
137144
if (changed == ChangeResult::Change) {
138145
DATAFLOW_DEBUG(llvm::dbgs() << "Propagating update to " << state->debugName
139146
<< " of " << state->anchor << "\n"

0 commit comments

Comments
 (0)