-
Notifications
You must be signed in to change notification settings - Fork 15.9k
[mlir][dataflow] Drop the firstIndex argument of visitNonControlFlowArguments #175210
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[mlir][dataflow] Drop the firstIndex argument of visitNonControlFlowArguments #175210
Conversation
|
@llvm/pr-subscribers-mlir Author: lonely eagle (linuxlonelyeagle) ChangesThis PR primarily drops the Full diff: https://github.com/llvm/llvm-project/pull/175210.diff 5 Files Affected:
diff --git a/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h b/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
index 4975cedb282e4..a673a62dcf349 100644
--- a/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
@@ -66,10 +66,13 @@ class IntegerRangeAnalysis
/// function calls `InferIntRangeInterface` to provide values for block
/// arguments or tries to reduce the range on loop induction variables with
/// known bounds.
- void
- visitNonControlFlowArguments(Operation *op, const RegionSuccessor &successor,
- ArrayRef<IntegerValueRangeLattice *> argLattices,
- unsigned firstIndex) override;
+ void visitNonControlFlowArguments(
+ Operation *op, const RegionSuccessor &successor,
+ ArrayRef<IntegerValueRangeLattice *> argLattices) override;
+
+ void visitNonControlFlowArguments(
+ const RegionSuccessor &successor,
+ ArrayRef<IntegerValueRangeLattice *> argLattices) override;
};
/// Succeeds if an op can be converted to its unsigned equivalent without
diff --git a/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h b/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
index 1bb42a246b701..012a6008c5e5b 100644
--- a/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
@@ -215,7 +215,11 @@ class AbstractSparseForwardDataFlowAnalysis : public DataFlowAnalysis {
/// of loops).
virtual void visitNonControlFlowArgumentsImpl(
Operation *op, const RegionSuccessor &successor,
- ArrayRef<AbstractSparseLattice *> argLattices, unsigned firstIndex) = 0;
+ ArrayRef<AbstractSparseLattice *> argLattices) = 0;
+
+ virtual void visitNonControlFlowArgumentsImpl(
+ const RegionSuccessor &successor,
+ ArrayRef<AbstractSparseLattice *> argLattices) = 0;
/// Get the lattice element of a value.
virtual AbstractSparseLattice *getLatticeElement(Value value) = 0;
@@ -328,11 +332,11 @@ class SparseForwardDataFlowAnalysis
/// index of the first element of `argLattices` that is set by control-flow.
virtual void visitNonControlFlowArguments(Operation *op,
const RegionSuccessor &successor,
- ArrayRef<StateT *> argLattices,
- unsigned firstIndex) {
- setAllToEntryStates(argLattices.take_front(firstIndex));
- setAllToEntryStates(argLattices.drop_front(
- firstIndex + successor.getSuccessorInputs().size()));
+ ArrayRef<StateT *> argLattices) {}
+
+ virtual void visitNonControlFlowArguments(const RegionSuccessor &successor,
+ ArrayRef<StateT *> argLattices) {
+ setAllToEntryStates(argLattices);
}
protected:
@@ -383,13 +387,18 @@ class SparseForwardDataFlowAnalysis
}
void visitNonControlFlowArgumentsImpl(
Operation *op, const RegionSuccessor &successor,
- ArrayRef<AbstractSparseLattice *> argLattices,
- unsigned firstIndex) override {
+ ArrayRef<AbstractSparseLattice *> argLattices) override {
visitNonControlFlowArguments(
op, successor,
{reinterpret_cast<StateT *const *>(argLattices.begin()),
- argLattices.size()},
- firstIndex);
+ argLattices.size()});
+ }
+ void visitNonControlFlowArgumentsImpl(
+ const RegionSuccessor &successor,
+ ArrayRef<AbstractSparseLattice *> argLattices) override {
+ visitNonControlFlowArguments(
+ successor, {reinterpret_cast<StateT *const *>(argLattices.begin()),
+ argLattices.size()});
}
void setToEntryState(AbstractSparseLattice *lattice) override {
return setToEntryState(reinterpret_cast<StateT *>(lattice));
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
index b76c2891fad5a..76aa20f11d84b 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
@@ -228,6 +228,24 @@ class RegionSuccessor {
/// the current region.
ValueRange getSuccessorInputs() const { return inputs; }
+ ValueRange getRegionNonforwardedArguments() const {
+ if (isParent())
+ return {};
+ SmallVector<Value> nonForwardArguments;
+ MutableArrayRef<BlockArgument> arguments = getSuccessor()->getArguments();
+ if (arguments.empty())
+ return {};
+ ValueRange inputs = getSuccessorInputs();
+ if (inputs.empty())
+ return arguments;
+ for (BlockArgument argument : arguments) {
+ if (!llvm::is_contained(inputs, argument)) {
+ nonForwardArguments.push_back(argument);
+ }
+ }
+ return nonForwardArguments;
+ }
+
bool operator==(RegionSuccessor rhs) const {
return successor == rhs.successor && inputs == rhs.inputs;
}
diff --git a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
index a93e605445465..84e134171961a 100644
--- a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
@@ -139,7 +139,8 @@ LogicalResult IntegerRangeAnalysis::visitOperation(
void IntegerRangeAnalysis::visitNonControlFlowArguments(
Operation *op, const RegionSuccessor &successor,
- ArrayRef<IntegerValueRangeLattice *> argLattices, unsigned firstIndex) {
+ ArrayRef<IntegerValueRangeLattice *> argLattices) {
+ ValueRange inputs = successor.getSuccessorInputs();
if (auto inferrable = dyn_cast<InferIntRangeInterface>(op)) {
LDBG() << "Inferring ranges for "
<< OpWithFlags(op, OpPrintingFlags().skipRegions());
@@ -152,11 +153,13 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
auto arg = dyn_cast<BlockArgument>(v);
if (!arg)
return;
- if (!llvm::is_contained(successor.getSuccessor()->getArguments(), arg))
+ if (!llvm::is_contained(inputs, arg))
return;
LDBG() << "Inferred range " << attrs;
- IntegerValueRangeLattice *lattice = argLattices[arg.getArgNumber()];
+ unsigned latticeIdx =
+ std::distance(inputs.begin(), llvm::find(inputs, arg));
+ IntegerValueRangeLattice *lattice = argLattices[latticeIdx];
IntegerValueRange oldRange = lattice->getValue();
ChangeResult changed = lattice->join(attrs);
@@ -177,9 +180,13 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
};
inferrable.inferResultRangesFromOptional(argRanges, joinCallback);
- return;
}
+}
+void IntegerRangeAnalysis::visitNonControlFlowArguments(
+ const RegionSuccessor &successor,
+ ArrayRef<IntegerValueRangeLattice *> argLattices) {
+ Operation *op = successor.getSuccessor()->getParentOp();
/// Given a lower bound, upper bound, or step from a LoopLikeInterface return
/// the lower/upper bound for that result if possible.
auto getLoopBoundFromFold = [&](OpFoldResult loopBound, Type boundType,
@@ -204,18 +211,13 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
// Infer bounds for loop arguments that have static bounds
if (auto loop = dyn_cast<LoopLikeOpInterface>(op)) {
- std::optional<llvm::SmallVector<Value>> maybeIvs =
- loop.getLoopInductionVars();
- if (!maybeIvs) {
- return SparseForwardDataFlowAnalysis ::visitNonControlFlowArguments(
- op, successor, argLattices, firstIndex);
- }
// This shouldn't be returning nullopt if there are indunction variables.
+ SmallVector<Value> ivs = successor.getRegionNonforwardedArguments();
SmallVector<OpFoldResult> lowerBounds = *loop.getLoopLowerBounds();
SmallVector<OpFoldResult> upperBounds = *loop.getLoopUpperBounds();
SmallVector<OpFoldResult> steps = *loop.getLoopSteps();
- for (auto [iv, lowerBound, upperBound, step] :
- llvm::zip_equal(*maybeIvs, lowerBounds, upperBounds, steps)) {
+ for (auto [iv, lowerBound, upperBound, step, ivEntry] :
+ llvm::zip_equal(ivs, lowerBounds, upperBounds, steps, argLattices)) {
Block *block = iv.getParentBlock();
APInt min = getLoopBoundFromFold(lowerBound, iv.getType(), block,
/*getUpper=*/false);
@@ -237,14 +239,9 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
// resulting range is meaningless and should not be used in further
// inferences.
if (max.sge(min)) {
- IntegerValueRangeLattice *ivEntry = getLatticeElement(iv);
auto ivRange = ConstantIntRanges::fromSigned(min, max);
propagateIfChanged(ivEntry, ivEntry->join(IntegerValueRange{ivRange}));
}
}
- return;
}
-
- return SparseForwardDataFlowAnalysis::visitNonControlFlowArguments(
- op, successor, argLattices, firstIndex);
-}
+}
\ No newline at end of file
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index b9d861830dd38..9fb7364d9d662 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -183,9 +183,10 @@ void AbstractSparseForwardDataFlowAnalysis::visitBlock(Block *block) {
}
// Otherwise, we can't reason about the data-flow.
- return visitNonControlFlowArgumentsImpl(block->getParentOp(),
- RegionSuccessor(block->getParent()),
- argLattices, /*firstIndex=*/0);
+ return visitNonControlFlowArgumentsImpl(
+ block->getParentOp(),
+ RegionSuccessor(block->getParent(), block->getParent()->getArguments()),
+ argLattices);
}
// Iterate over the predecessors of the non-entry block.
@@ -307,29 +308,24 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
"expected the same number of successor inputs as operands");
unsigned firstIndex = 0;
- if (inputs.size() != lattices.size()) {
- if (!point->isBlockStart()) {
- if (!inputs.empty())
- firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
- visitNonControlFlowArgumentsImpl(
- branch,
- RegionSuccessor(
- branch, branch->getResults().slice(firstIndex, inputs.size())),
- lattices, firstIndex);
- } else {
- if (!inputs.empty())
- firstIndex = cast<BlockArgument>(inputs.front()).getArgNumber();
- Region *region = point->getBlock()->getParent();
- visitNonControlFlowArgumentsImpl(
- branch,
- RegionSuccessor(region, region->getArguments().slice(
- firstIndex, inputs.size())),
- lattices, firstIndex);
- }
+ if (inputs.size() != lattices.size() && point->isBlockStart()) {
+ if (!inputs.empty())
+ firstIndex = cast<BlockArgument>(inputs.front()).getArgNumber();
+ Region *region = point->getBlock()->getParent();
+ RegionSuccessor regionSuccessor(
+ region, region->getArguments().slice(firstIndex, inputs.size()));
+ SmallVector<AbstractSparseLattice *> nonForwardArgs(
+ lattices.take_front(firstIndex));
+ nonForwardArgs.append(SmallVector<AbstractSparseLattice *>(
+ lattices.drop_front(firstIndex + inputs.size())));
+ visitNonControlFlowArgumentsImpl(
+ branch, regionSuccessor, lattices.slice(firstIndex, inputs.size()));
+ visitNonControlFlowArgumentsImpl(regionSuccessor, nonForwardArgs);
}
- for (auto it : llvm::zip(*operands, lattices.drop_front(firstIndex)))
- join(std::get<1>(it), *getLatticeElementFor(point, std::get<0>(it)));
+ for (auto [lattice, operand] :
+ llvm::zip(lattices.drop_front(firstIndex), *operands))
+ join(lattice, *getLatticeElementFor(point, operand));
}
}
@@ -612,16 +608,8 @@ void AbstractSparseBackwardDataFlowAnalysis::visitRegionSuccessors(
if (successor.isParent())
continue;
SmallVector<BlockArgument> noControlFlowArguments;
- MutableArrayRef<BlockArgument> arguments =
- successor.getSuccessor()->getArguments();
- ValueRange inputs = successor.getSuccessorInputs();
- for (BlockArgument argument : arguments) {
- // Visit blockArgument of RegionBranchOp which isn't "control
- // flow block arguments". For example, the IV of a loop.
- if (!llvm::is_contained(inputs, argument)) {
- noControlFlowArguments.push_back(argument);
- }
- }
+ for (Value arg : successor.getRegionNonforwardedArguments())
+ noControlFlowArguments.push_back(cast<BlockArgument>(arg));
visitNonControlFlowArguments(successor, noControlFlowArguments);
}
|
3ffd39b to
cc87b26
Compare
57a1aeb to
0afc994
Compare
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
| return visitNonControlFlowArgumentsImpl( | ||
| block->getParentOp(), RegionSuccessor(block->getParent()), ValueRange(), | ||
| argLattices, /*firstIndex=*/0); | ||
| block->getParentOp(), block->getParent(), block->getParent()->getArguments(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to explain why I introduced the new visitNonControlFlowArgumentsImpl. Since the op in question is not a RegionBranchOp, I believe it’s conceptually inconsistent to use its regions to construct a RegionSuccessor. Doing so might cause confusion for users, and I'd love to hear your thoughts on this. cc: @matthias-springer @ftynse @joker-eph
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also noticed that. RegionSuccessor for ops that don't implement the RegionBranchOpInterface seems fishy.
e30601c to
73789e0
Compare
| /// elements as having reached a pessimistic fixpoint. `firstIndex` is the | ||
| /// index of the first element of `argLattices` that is set by control-flow. | ||
| /// elements as having reached a pessimistic fixpoint. | ||
| virtual void visitNonControlFlowArguments(Operation *op, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can this
Operation *be changed toRegionBranchOpInterface. - I am also wondering about the name of the function: "nonControlFlowArguments" is inconsistent with the naming in the region branch op interface. Should this function be called
visitNonSuccessorInputs? Is this function only called for block arguments or also for op results? (In the latter case, how are non-successor-input op results handled?)edit: I think this can be called for both block arguments and op results, which makes the current name of the function even worse.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can this Operation * be changed to RegionBranchOpInterface.
Yes. - I am also wondering about the name of the function: "nonControlFlowArguments" is inconsistent with the naming in the region branch op interface. Should this function be called visitNonSuccessorInputs?
Yes. - I think this can be called for both block arguments and op results, which makes the current name of the function even worse.
Yes.
| setAllToEntryStates( | ||
| argLattices.drop_front(firstIndex + successorInputs.size())); | ||
| ArrayRef<StateT *> argLattices) { | ||
| setAllToEntryStates(argLattices); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did this implementation change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see
SmallVector<Value> nonSuccessorInputs =
branch.getNonSuccessorInputs(RegionSuccessor::parent());
SmallVector<AbstractSparseLattice *> nonSuccessorInputLattices =
llvm::map_to_vector(nonSuccessorInputs, valueToLattices);
visitNonControlFlowArgumentsImpl(branch, RegionSuccessor::parent(),
nonSuccessorInputs,
nonSuccessorInputLattices);
In this case, all of our argLattices are the lattices corresponding to the noSuccessorInputs.
| /// Given an operation with region non-control-flow, the lattices of the entry | ||
| /// block arguments, compute the lattice values for block arguments.(ex. the | ||
| /// block argument of gpu.launch). | ||
| virtual void visitNonControlFlowArguments(Operation *op, Region *const region, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it correct to say that this function is never called for ops that implement the RegionBranchOpInterface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can see
| return visitNonControlFlowArgumentsImpl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this makes sense to me, but can you split this up in two PRs: adding the second visitNonControlFlowArgumentsImpl overload in a separate PR.
This PR would then just drop the firstIndex parameter, change successorInputs -> nonSuccessorInputs, and pass the argLattices that correspond to the nonSuccessorInputs.
Doing this in two PRs should make it easier for folks to understand the changes and integrate.
| return; | ||
|
|
||
| LDBG() << "Inferred range " << attrs; | ||
| IntegerValueRangeLattice *lattice = argLattices[arg.getArgNumber()]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this still correct? You are passing a slice of argLattices now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is correct. You can see https://github.com/llvm/llvm-project/blob/73789e07e3dead9c90bf746ea67c84a0ba78da39/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp#L189.it is not a slice of argLattices.
"Thanks for the suggestion. I'll update it when I have some time. |
4e04a18 to
0757919
Compare
|
Thanks for cleaning this up, this API is kind of a mess... I'd like to make sure that we get this right. If we get this wrong, it will be a pain because not people understand this part of the code base that well. Sry for asking for so many revisions / clarifications. |
You're welcome.I'd be happy to do that.Also, thank you for the hard work on reviewing my code. |
0757919 to
5d5d906
Compare
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
…onBranchOpInterface op.
5d5d906 to
8df12f9
Compare
8df12f9 to
810a3db
Compare
matthias-springer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there. I updated the commit message, can you double-check if it makes sense?
87b216b to
7be0d92
Compare
Thanks for helping with the PR description! To be honest, you wrote it better than I could. In addition to your changes, I’ve added a link to the previous RFC at the end. |
…rguments (llvm#175210) This PR improves the signature of `visitNonControlFlowArguments`: - The function now takes non-successor-inputs ("non-control-flow arguments") instead of successor inputs. This is more consistent with the naming of the function. - `firstIndex` is no longer needed and dropped. (It was needed only to identify the non-successor-inputs among the block arguments / op results.) Background: Successor inputs are forwarded values (e.g., iter_args / op results of an `scf.for`) and non-successor-inputs are all other block arguments / op results (e.g., the loop induction variable of an `scf.for`.) Note for LLVM integration: `visitNonControlFlowArguments` now receives the non-successor-input directly. You no longer have to find those among the list of all block arguments / op results based on `firstIndex`. RFC: https://discourse.llvm.org/t/rfc-drop-the-firstindex-argument-of-visitnoncontrolflowarguments-of-sparseforwarddataflowanalysis/89419/5
Reverts: - Carries local revert of llvm/llvm-project#169614 due to #22649. - Adds revert of llvm/llvm-project#177982. `reifyResultShapes()` is unimplemented for pack ops on memrefs causing a crash in [getPackUnPackIterationDomain](https://github.com/iree-org/llvm-project/blob/b24bd7161ca7eb6e9652c34b92e200ac16af3628/mlir/lib/Dialect/Linalg/Transforms/TilingInterfaceImpl.cpp#L718) Fixes: - Passes `LLVM::DIFlags::Zero` to address the added argument for L`LVM::DIDerivedTypeAttr::get()` [llvm/llvm-project#177889](llvm/llvm-project#177889) - Removes he `firstIndex` parameter to address the API change to `visitNonControlFlowArguments()` [llvm/llvm-project#175210](llvm/llvm-project#175210) https://github.com/iree-org/llvm-project/tree/sm-iree-integrates/llvm-20260128 Signed-off-by: Ian Wood <[email protected]>
This PR improves the signature of
visitNonControlFlowArguments:firstIndexis no longer needed and dropped. (It was needed only to identify the non-successor-inputs among the block arguments / op results.)Background: Successor inputs are forwarded values (e.g., iter_args / op results of an
scf.for) and non-successor-inputs are all other block arguments / op results (e.g., the loop induction variable of anscf.for.)Note for LLVM integration:
visitNonControlFlowArgumentsnow receives the non-successor-input directly. You no longer have to find those among the list of all block arguments / op results based onfirstIndex.RFC: https://discourse.llvm.org/t/rfc-drop-the-firstindex-argument-of-visitnoncontrolflowarguments-of-sparseforwarddataflowanalysis/89419/5