[mlir][Analysis][NFC] Clean up visitNonControlFlowArguments#175978
Closed
matthias-springer wants to merge 1 commit intomainfrom
Closed
[mlir][Analysis][NFC] Clean up visitNonControlFlowArguments#175978matthias-springer wants to merge 1 commit intomainfrom
visitNonControlFlowArguments#175978matthias-springer wants to merge 1 commit intomainfrom
Conversation
| setAllToEntryStates(resultLattices); | ||
| } | ||
|
|
||
| /// Given an operation with possible region control-flow, the lattices of the |
Member
Author
There was a problem hiding this comment.
This comment looks incorrect / incomplete to me. I believe the argLattices could also be lattices for op results in case of a RegionBranchOpInterface op with "parent" successor.
Member
Author
|
@linuxlonelyeagle What do you think about this PR? You probably know this part of the code base better than me. I feel like the documentation of |
visitNonControlFlowArgumentsvisitNonControlFlowArguments
| unsigned firstIndex) { | ||
| ArrayRef<StateT *> argLattices) { | ||
| auto regionBranchOp = dyn_cast<RegionBranchOpInterface>(op); | ||
| if (!regionBranchOp) { |
Member
There was a problem hiding this comment.
op maybe not a RegionBranchOpInterface. So, there is a reason for adding this check.
Member
Author
There was a problem hiding this comment.
Correct, this function may be called even for non-RegionBranchOpInterface ops.
| @@ -314,18 +313,14 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors( | |||
| if (!point->isBlockStart()) { | |||
| if (!inputs.empty()) | |||
| firstIndex = cast<OpResult>(inputs.front()).getResultNumber(); | |||
Member
There was a problem hiding this comment.
Is the firstIndex argument here consistent with the one calculated via getSuccessorInputs?
Member
Author
There was a problem hiding this comment.
There are two getSuccessorInputs functions: RegionBranchOpInterface::getSuccessorInputs and PredecessorState::getSuccessorInputs.
The latter is populated from the former:
// Add the parent op as a predecessor.
auto *predecessors = getOrCreate<PredecessorState>(point);
propagateIfChanged(
predecessors,
predecessors->join(predecessorOp,
regionBranchOp.getSuccessorInputs(successor)));
So firstIndex should be consistent with what the RegionBranchOpInterface returns.
bf8c119 to
3b1d7ed
Compare
Base automatically changed from
users/matthias-springer/split_successor_inputs
to
main
January 16, 2026 09:16
6d5a0b7 to
473f52b
Compare
Member
|
Please wait @ftynse to review it. |
473f52b to
401cad6
Compare
Member
|
@llvm/pr-subscribers-mlir Author: Matthias Springer (matthias-springer) ChangesRemove Depends on #175815. Full diff: https://github.com/llvm/llvm-project/pull/175978.diff 4 Files Affected:
diff --git a/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h b/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
index e549a56a6f960..feef04e58e253 100644
--- a/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/IntegerRangeAnalysis.h
@@ -66,11 +66,9 @@ 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,
- ValueRange successorInputs,
- ArrayRef<IntegerValueRangeLattice *> argLattices,
- unsigned firstIndex) override;
+ void visitNonControlFlowArguments(
+ Operation *op, 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 02f699de06f99..f8d18e94e5df0 100644
--- a/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
+++ b/mlir/include/mlir/Analysis/DataFlow/SparseAnalysis.h
@@ -212,11 +212,10 @@ class AbstractSparseForwardDataFlowAnalysis : public DataFlowAnalysis {
/// Given an operation with region control-flow, the lattices of the operands,
/// and a region successor, compute the lattice values for block arguments
/// that are not accounted for by the branching control flow (ex. the bounds
- /// of loops).
+ /// of loops)
virtual void visitNonControlFlowArgumentsImpl(
Operation *op, const RegionSuccessor &successor,
- ValueRange successorInputs, ArrayRef<AbstractSparseLattice *> argLattices,
- unsigned firstIndex) = 0;
+ ArrayRef<AbstractSparseLattice *> argLattices) = 0;
/// Get the lattice element of a value.
virtual AbstractSparseLattice *getLatticeElement(Value value) = 0;
@@ -325,13 +324,31 @@ class SparseForwardDataFlowAnalysis
/// operands, and a region successor, compute the lattice values for block
/// arguments that are not accounted for by the branching control flow (ex.
/// the bounds of loops). By default, this method marks all such lattice
- /// 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. If the given operation
+ /// does not implement the `RegionBranchOpInterface`, this method marks all
+ /// lattice elements as having reached a pessimistic fixpoint.
virtual void visitNonControlFlowArguments(Operation *op,
const RegionSuccessor &successor,
- ValueRange successorInputs,
- ArrayRef<StateT *> argLattices,
- unsigned firstIndex) {
+ ArrayRef<StateT *> argLattices) {
+ auto regionBranchOp = dyn_cast<RegionBranchOpInterface>(op);
+ if (!regionBranchOp) {
+ // There are no forwarded arguments. Mark all lattice elements as having
+ // reached a pessimistic fixpoint.
+ setAllToEntryStates(argLattices);
+ return;
+ }
+
+ // Mark only the lattice elements that are not forwarded as having reached
+ // a pessimistic fixpoint.
+ ValueRange successorInputs = regionBranchOp.getSuccessorInputs(successor);
+ unsigned firstIndex = 0;
+ if (!successorInputs.empty() &&
+ isa<BlockArgument>(successorInputs.front())) {
+ firstIndex = cast<BlockArgument>(successorInputs.front()).getArgNumber();
+ } else if (!successorInputs.empty() &&
+ isa<OpResult>(successorInputs.front())) {
+ firstIndex = cast<OpResult>(successorInputs.front()).getResultNumber();
+ }
setAllToEntryStates(argLattices.take_front(firstIndex));
setAllToEntryStates(
argLattices.drop_front(firstIndex + successorInputs.size()));
@@ -385,13 +402,11 @@ class SparseForwardDataFlowAnalysis
}
void visitNonControlFlowArgumentsImpl(
Operation *op, const RegionSuccessor &successor,
- ValueRange successorInputs, ArrayRef<AbstractSparseLattice *> argLattices,
- unsigned firstIndex) override {
+ ArrayRef<AbstractSparseLattice *> argLattices) override {
visitNonControlFlowArguments(
- op, successor, successorInputs,
+ op, successor,
{reinterpret_cast<StateT *const *>(argLattices.begin()),
- argLattices.size()},
- firstIndex);
+ argLattices.size()});
}
void setToEntryState(AbstractSparseLattice *lattice) override {
return setToEntryState(reinterpret_cast<StateT *>(lattice));
diff --git a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
index 012d8384d3098..cf2dedc85d05f 100644
--- a/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/IntegerRangeAnalysis.cpp
@@ -138,8 +138,8 @@ LogicalResult IntegerRangeAnalysis::visitOperation(
}
void IntegerRangeAnalysis::visitNonControlFlowArguments(
- Operation *op, const RegionSuccessor &successor, ValueRange successorInputs,
- ArrayRef<IntegerValueRangeLattice *> argLattices, unsigned firstIndex) {
+ Operation *op, const RegionSuccessor &successor,
+ ArrayRef<IntegerValueRangeLattice *> argLattices) {
if (auto inferrable = dyn_cast<InferIntRangeInterface>(op)) {
LDBG() << "Inferring ranges for "
<< OpWithFlags(op, OpPrintingFlags().skipRegions());
@@ -207,8 +207,8 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
std::optional<llvm::SmallVector<Value>> maybeIvs =
loop.getLoopInductionVars();
if (!maybeIvs) {
- return SparseForwardDataFlowAnalysis ::visitNonControlFlowArguments(
- op, successor, successorInputs, argLattices, firstIndex);
+ return SparseForwardDataFlowAnalysis::visitNonControlFlowArguments(
+ op, successor, argLattices);
}
// This shouldn't be returning nullopt if there are indunction variables.
SmallVector<OpFoldResult> lowerBounds = *loop.getLoopLowerBounds();
@@ -246,5 +246,5 @@ void IntegerRangeAnalysis::visitNonControlFlowArguments(
}
return SparseForwardDataFlowAnalysis::visitNonControlFlowArguments(
- op, successor, successorInputs, argLattices, firstIndex);
+ op, successor, argLattices);
}
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index f86bb55df3ac5..ec2de30a674e8 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -187,8 +187,7 @@ void AbstractSparseForwardDataFlowAnalysis::visitBlock(Block *block) {
// Otherwise, we can't reason about the data-flow.
return visitNonControlFlowArgumentsImpl(
- block->getParentOp(), RegionSuccessor(block->getParent()), ValueRange(),
- argLattices, /*firstIndex=*/0);
+ block->getParentOp(), RegionSuccessor(block->getParent()), argLattices);
}
// Iterate over the predecessors of the non-entry block.
@@ -314,18 +313,14 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
if (!point->isBlockStart()) {
if (!inputs.empty())
firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
- visitNonControlFlowArgumentsImpl(
- branch, RegionSuccessor::parent(),
- branch->getResults().slice(firstIndex, inputs.size()), lattices,
- firstIndex);
+ visitNonControlFlowArgumentsImpl(branch, RegionSuccessor::parent(),
+ lattices);
} 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);
+ visitNonControlFlowArgumentsImpl(branch, RegionSuccessor(region),
+ lattices);
}
}
|
Member
Author
|
Closing in favor of #175210. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove
successorInputsandfirstIndexfromvisitNonControlFlowArguments. These can be queried from theRegionBranchOpInterface. If the op does not implement that interface, we can assume that there are no successor inputs.Depends on #175815.