[mlir][Interfaces][NFC] Simplify and align RegionSuccessor design / API
#174945
Conversation
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
matthias-springer
requested review from
PeimingLiu,
aartbik,
andykaylor,
bcardosolopes,
fabianmcg,
ftynse,
lanza,
nicolasvasilache,
rolfmorel,
xlauko and
yinying-lisa-li
as code owners
llvmbot
added
mlir:gpu
mlir:affine
mlir:sparse
Member
|
@llvm/pr-subscribers-clangir @llvm/pr-subscribers-mlir-gpu Author: Matthias Springer (matthias-springer) ChangesSimplify the design of Add a new helper function
Also clean up the documentation to use the same terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from #161575. That PR introduced Patch is 34.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174945.diff 21 Files Affected:
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 063896b00a5a5..91abd97fd75ea 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -1167,8 +1167,7 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
SmallVectorImpl<RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1218,7 +1217,7 @@ void cir::ScopeOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The only region always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(getODSResults(0)));
return;
}
@@ -1383,8 +1382,7 @@ Block *cir::BrCondOp::getSuccessorForOperands(ArrayRef<Attribute> operands) {
void cir::CaseOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
regions.push_back(RegionSuccessor(&getCaseRegion()));
@@ -1410,8 +1408,7 @@ void cir::CaseOp::build(OpBuilder &builder, OperationState &result,
void cir::SwitchOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ion) {
if (!point.isParent()) {
- region.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ region.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1625,8 +1622,7 @@ void cir::GlobalOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `ctor` and `dtor` regions always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -2311,7 +2307,7 @@ void cir::TernaryOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `true` and the `false` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), this->getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(this->getODSResults(0)));
return;
}
@@ -2518,8 +2514,7 @@ void cir::AwaitOp::getSuccessorRegions(
// If any index all the underlying regions branch back to the parent
// operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -3433,8 +3428,7 @@ void cir::TryOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `try` and the `catchers` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 90c960843787e..20dceb21bfecf 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -4699,7 +4699,7 @@ void fir::IfOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(mlir::RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(mlir::RegionSuccessor::parent(getResults()));
return;
}
@@ -4710,7 +4710,7 @@ void fir::IfOp::getSuccessorRegions(
mlir::Region *elseRegion = &this->getElseRegion();
if (elseRegion->empty())
regions.push_back(
- mlir::RegionSuccessor(getOperation(), getOperation()->getResults()));
+ mlir::RegionSuccessor::parent(getOperation()->getResults()));
else
regions.push_back(mlir::RegionSuccessor(elseRegion));
}
@@ -4729,7 +4729,7 @@ void fir::IfOp::getEntrySuccessorRegions(
if (!getElseRegion().empty())
regions.emplace_back(&getElseRegion());
else
- regions.emplace_back(getOperation(), getOperation()->getResults());
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
}
}
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
index b76c2891fad5a..3b2b0ba50ebb4 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
@@ -210,19 +210,19 @@ class RegionSuccessor {
: successor(region), inputs(regionInputs) {
assert(region && "Region must not be null");
}
+
/// Initialize a successor that branches back to/out of the parent operation.
/// The target must be one of the recursive parent operations.
- RegionSuccessor(Operation *successorOp, Operation::result_range results)
- : successor(successorOp), inputs(ValueRange(results)) {
- assert(successorOp && "Successor op must not be null");
+ static RegionSuccessor parent(Operation::result_range results) {
+ return RegionSuccessor(results);
}
/// Return the given region successor. Returns nullptr if the successor is the
/// parent operation.
- Region *getSuccessor() const { return dyn_cast<Region *>(successor); }
+ Region *getSuccessor() const { return successor; }
/// Return true if the successor is the parent operation.
- bool isParent() const { return isa<Operation *>(successor); }
+ bool isParent() const { return successor == nullptr; }
/// Return the inputs to the successor that are remapped by the exit values of
/// the current region.
@@ -237,7 +237,11 @@ class RegionSuccessor {
}
private:
- llvm::PointerUnion<Region *, Operation *> successor{nullptr};
+ /// Private constructor to encourage the use of `RegionSuccessor::parent`.
+ RegionSuccessor(Operation::result_range results)
+ : successor(nullptr), inputs(ValueRange(results)) {}
+
+ Region *successor = nullptr;
ValueRange inputs;
};
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index ecad424e30c75..ff99e220c179f 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -117,25 +117,31 @@ def BranchOpInterface : OpInterface<"BranchOpInterface"> {
def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
let description = [{
- This interface provides information for region-holding operations that exhibit
- branching behavior between held regions. I.e., this interface allows for
- expressing control flow information for region holding operations.
+ This interface provides information for region-holding operations that
+ exhibit branching behavior between held regions. I.e., this interface allows
+ for expressing control flow information for region holding operations.
This interface is meant to model well-defined cases of control-flow and
value propagation, where what occurs along control-flow edges is assumed to
be side-effect free.
A "region branch point" indicates a point from which a branch originates. It
- can indicate either a terminator in any of the immediately nested region of
- this op or `RegionBranchPoint::parent()`. In the latter case, the branch
- originates from outside of the op, i.e., when first executing this op.
-
- A "region successor" indicates the target of a branch. It can indicate
- either a region of this op or this op itself. In the former case, the region
- successor is a region pointer and a range of block arguments to which the
- "successor operands" are forwarded to. In the latter case, the control flow
- leaves this op and the region successor is a range of results of this op to
- which the successor operands are forwarded to.
+ can indicate:
+ 1. A terminator in any of the immediately nested region of this op.
+ 2. `RegionBranchPoint::parent()`: the branch originates from outside of the
+ op, i.e., when first executing this op.
+
+ When branching from a region branch point to a region successor, the
+ "successor operands" to be forwarded from the region branch point can be
+ specified with `getEntrySuccessorOperands` /
+ `RegionBranchTerminatorOpInterface::getSuccessorOperands`.
+
+ A "region successor" indicates the target of a branch. It can indicate:
+ 1. A region of this op and a range of entry block arguments ("successor
+ inputs") to which the successor operands are forwarded to.
+ 2. `RegionSuccessor::parent()` and a range of op results of this op
+ ("successor inputs") to which the successor operands are forwarded to.
+ The control flow leaves this op.
By default, successor operands and successor block arguments/successor
results must have the same type. `areTypesCompatible` can be implemented to
@@ -151,19 +157,22 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
```
- `scf.for` has one region. The `scf.yield` has two region successors: the
- region body itself and the `scf.for` op. `%b` is an entry successor
- operand. `%c` is a successor operand. `%a` is a successor block argument.
- `%r` is a successor result.
+ `scf.for` has one region. There are two region branch points with two
+ identical region successors:
+ * parent => parent(%r), region0(%a)
+ * `scf.yield` => parent(%r), region0(%a)
+
+ `%a` and %r are successor inputs. `%b` is an entry successor operand. `%c`
+ is a successor operand.
}];
let cppNamespace = "::mlir";
let methods = [
InterfaceMethod<[{
- Returns the operands of this operation that are forwarded to the region
- successor's block arguments or this operation's results when branching
- to `successor`. `successor` is guaranteed to be among the successors that are
- returned by `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
+ Returns the operands of this operation that are forwarded to the
+ successor inputs when branching to `successor`. `successor` is
+ guaranteed to be among the successors that are returned by
+ `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
Example: In the above example, this method returns the operand %b of the
`scf.for` op, regardless of the value of `successor`. I.e., this op always
@@ -215,10 +224,10 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
by `point`.
Example: In the above example, this method returns the region of the
- `scf.for` and this operation for either region branch point (`parent`
- and the region of the `scf.for`). An implementation may choose to filter
- out region successors when it is statically known (e.g., by examining
- the operands of this op) that those successors are not branched to.
+ `scf.for` and `parent` for either region branch point. An implementation
+ may choose to filter out region successors when it is statically known
+ (e.g., by examining the operands of this op) that those successors are
+ not branched to.
}],
"void", "getSuccessorRegions",
(ins "::mlir::RegionBranchPoint":$point,
@@ -227,7 +236,6 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
InterfaceMethod<[{
Returns the potential region successors when branching from any
terminator in `region`.
- These are the regions that may be selected during the flow of control.
}],
"void", "getSuccessorRegions",
(ins "::mlir::Region&":$region,
@@ -244,7 +252,8 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
}]>,
InterfaceMethod<[{
- Returns the potential branching point (predecessors) for a given successor.
+ Returns the potential branching points (predecessors) for a given
+ region successor.
}],
"void", "getPredecessors",
(ins "::mlir::RegionSuccessor":$successor,
@@ -370,7 +379,7 @@ def RegionBranchTerminatorOpInterface :
let description = [{
This interface provides information for branching terminator operations
in the presence of a parent `RegionBranchOpInterface` implementation. It
- specifies which operands are passed to which successor region.
+ specifies which operands are passed to which region successor.
}];
let cppNamespace = "::mlir";
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index 24cb123e51877..be53a4e56f37a 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -109,7 +109,7 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
LDBG() << " Processing region branch operation";
return collectUnderlyingAddressValues2(
- branch, RegionSuccessor(op, op->getResults()), result,
+ branch, RegionSuccessor::parent(op->getResults()), result,
result.getResultNumber(), maxDepth, visited, output);
}
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index b9d861830dd38..31b388ebfcede 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -130,7 +130,7 @@ AbstractSparseForwardDataFlowAnalysis::visitOperation(Operation *op) {
// The results of a region branch operation are determined by control-flow.
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
visitRegionSuccessors(getProgramPointAfter(branch), branch,
- /*successor=*/{branch, branch->getResults()},
+ RegionSuccessor::parent(branch->getResults()),
resultLattices);
return success();
}
@@ -313,8 +313,8 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
visitNonControlFlowArgumentsImpl(
branch,
- RegionSuccessor(
- branch, branch->getResults().slice(firstIndex, inputs.size())),
+ RegionSuccessor::parent(
+ branch->getResults().slice(firstIndex, inputs.size())),
lattices, firstIndex);
} else {
if (!inputs.empty())
diff --git a/mlir/lib/Analysis/SliceWalk.cpp b/mlir/lib/Analysis/SliceWalk.cpp
index 863f260cd4b6a..19636eeccc339 100644
--- a/mlir/lib/Analysis/SliceWalk.cpp
+++ b/mlir/lib/Analysis/SliceWalk.cpp
@@ -114,7 +114,7 @@ mlir::getControlFlowPredecessors(Value value) {
if (!regionOp)
return std::nullopt;
// Add the control flow predecessor operands to the work list.
- RegionSuccessor region(regionOp, regionOp->getResults());
+ RegionSuccessor region = RegionSuccessor::parent(regionOp->getResults());
SmallVector<Value> predecessorOperands = getRegionPredecessorOperands(
regionOp, region, opResult.getResultNumber());
return predecessorOperands;
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index c6addfb010856..df1b93e367fc6 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -2741,7 +2741,7 @@ void AffineForOp::getSuccessorRegions(
// From the loop body, if the trip count is one, we can only branch back
// to the parent.
if (tripCount == 1) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
if (tripCount == 0)
@@ -2752,7 +2752,7 @@ void AffineForOp::getSuccessorRegions(
return;
}
if (tripCount.value() == 0) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
}
@@ -2761,7 +2761,7 @@ void AffineForOp::getSuccessorRegions(
// In all other cases, the loop may branch back to itself or the parent
// operation.
regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs()));
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
AffineBound AffineForOp::getLowerBound() {
@@ -3150,7 +3150,7 @@ void AffineIfOp::getSuccessorRegions(
RegionSuccessor(&getThenRegion(), getThenRegion().getArguments()));
// If the "else" region is empty, branch bach into parent.
if (getElseRegion().empty()) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
} else {
regions.push_back(
RegionSuccessor(&getElseRegion(), getElseRegion().getArguments()));
@@ -3160,7 +3160,7 @@ void AffineIfOp::getSuccessorRegions(
// If the predecessor is the `else`/`then` region, then branching into parent
// op is valid.
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
LogicalResult AffineIfOp::verify() {
diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp
index e19b917787df1..11fd87ed925d8 100644
--- a/mlir/lib/Dialect/Async/IR/Async.cpp
+++ b/mlir/lib/Dialect/Async/IR/Async.cpp
@@ -55,7 +55,7 @@ void ExecuteOp::getSuccessorRegions(RegionBranchPoint point,
if (!point.isParent() &&
point.getTerminatorPredecessorOrNull()->getParentRegion() ==
&getBodyRegion()) {
- regions.push_back(RegionSuccessor(getOperation(), getBodyResults()));
+ regions.push_back(RegionSuccessor::parent(getBodyResults()));
return;
}
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
index 36a759c279eb7..9e8746cb8ea35 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
@@ -564,8 +564,8 @@ BufferDeallocation::updateFunctionSignature(FunctionOpInterface op) {
op.getFunctionBody().getOps<RegionBranchTerminatorOpInterface>(),
[&](RegionBranchTerminatorOpInterface branchOp) {
return branchOp
- .getSuccessorOperands(RegionSuccessor(
- op.getOperation(), op.getOperation()->getResults()))
+ .getSuccessorOperands(
+ RegionSuccessor::parent(op.getOperation()->getResults()))
.getTypes();
}));
if (!llvm::all_equal(returnOperandTypes))
@@ -946,7 +946,7 @@ BufferDeallocation::handleInterface(RegionBranchTerminatorOpInterface op) {
// which condition they are taken, etc.
MutableOperandRange operands = op.getMutableSuccessorOperands(
- RegionSuccessor(op.getOperation(), op.getOperation()->getResults()));
+ RegionSuccessor::parent(op.getOperation()->getResults()));
SmallVector<Value> updatedOwnerships;
auto result = deallocation_impl::insertDeallocOpForReturnLike(
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index b0566dd10f490..ca29ff833535d 100644
--- a/mlir/lib/Dialect/EmitC/IR/Emit...
[truncated]
|
Member
|
@llvm/pr-subscribers-mlir-bufferization Author: Matthias Springer (matthias-springer) ChangesSimplify the design of Add a new helper function
Also clean up the documentation to use the same terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from #161575. That PR introduced Patch is 34.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174945.diff 21 Files Affected:
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 063896b00a5a5..91abd97fd75ea 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -1167,8 +1167,7 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
SmallVectorImpl<RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1218,7 +1217,7 @@ void cir::ScopeOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The only region always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(getODSResults(0)));
return;
}
@@ -1383,8 +1382,7 @@ Block *cir::BrCondOp::getSuccessorForOperands(ArrayRef<Attribute> operands) {
void cir::CaseOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
regions.push_back(RegionSuccessor(&getCaseRegion()));
@@ -1410,8 +1408,7 @@ void cir::CaseOp::build(OpBuilder &builder, OperationState &result,
void cir::SwitchOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ion) {
if (!point.isParent()) {
- region.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ region.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1625,8 +1622,7 @@ void cir::GlobalOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `ctor` and `dtor` regions always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -2311,7 +2307,7 @@ void cir::TernaryOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `true` and the `false` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), this->getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(this->getODSResults(0)));
return;
}
@@ -2518,8 +2514,7 @@ void cir::AwaitOp::getSuccessorRegions(
// If any index all the underlying regions branch back to the parent
// operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -3433,8 +3428,7 @@ void cir::TryOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `try` and the `catchers` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 90c960843787e..20dceb21bfecf 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -4699,7 +4699,7 @@ void fir::IfOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(mlir::RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(mlir::RegionSuccessor::parent(getResults()));
return;
}
@@ -4710,7 +4710,7 @@ void fir::IfOp::getSuccessorRegions(
mlir::Region *elseRegion = &this->getElseRegion();
if (elseRegion->empty())
regions.push_back(
- mlir::RegionSuccessor(getOperation(), getOperation()->getResults()));
+ mlir::RegionSuccessor::parent(getOperation()->getResults()));
else
regions.push_back(mlir::RegionSuccessor(elseRegion));
}
@@ -4729,7 +4729,7 @@ void fir::IfOp::getEntrySuccessorRegions(
if (!getElseRegion().empty())
regions.emplace_back(&getElseRegion());
else
- regions.emplace_back(getOperation(), getOperation()->getResults());
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
}
}
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
index b76c2891fad5a..3b2b0ba50ebb4 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
@@ -210,19 +210,19 @@ class RegionSuccessor {
: successor(region), inputs(regionInputs) {
assert(region && "Region must not be null");
}
+
/// Initialize a successor that branches back to/out of the parent operation.
/// The target must be one of the recursive parent operations.
- RegionSuccessor(Operation *successorOp, Operation::result_range results)
- : successor(successorOp), inputs(ValueRange(results)) {
- assert(successorOp && "Successor op must not be null");
+ static RegionSuccessor parent(Operation::result_range results) {
+ return RegionSuccessor(results);
}
/// Return the given region successor. Returns nullptr if the successor is the
/// parent operation.
- Region *getSuccessor() const { return dyn_cast<Region *>(successor); }
+ Region *getSuccessor() const { return successor; }
/// Return true if the successor is the parent operation.
- bool isParent() const { return isa<Operation *>(successor); }
+ bool isParent() const { return successor == nullptr; }
/// Return the inputs to the successor that are remapped by the exit values of
/// the current region.
@@ -237,7 +237,11 @@ class RegionSuccessor {
}
private:
- llvm::PointerUnion<Region *, Operation *> successor{nullptr};
+ /// Private constructor to encourage the use of `RegionSuccessor::parent`.
+ RegionSuccessor(Operation::result_range results)
+ : successor(nullptr), inputs(ValueRange(results)) {}
+
+ Region *successor = nullptr;
ValueRange inputs;
};
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index ecad424e30c75..ff99e220c179f 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -117,25 +117,31 @@ def BranchOpInterface : OpInterface<"BranchOpInterface"> {
def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
let description = [{
- This interface provides information for region-holding operations that exhibit
- branching behavior between held regions. I.e., this interface allows for
- expressing control flow information for region holding operations.
+ This interface provides information for region-holding operations that
+ exhibit branching behavior between held regions. I.e., this interface allows
+ for expressing control flow information for region holding operations.
This interface is meant to model well-defined cases of control-flow and
value propagation, where what occurs along control-flow edges is assumed to
be side-effect free.
A "region branch point" indicates a point from which a branch originates. It
- can indicate either a terminator in any of the immediately nested region of
- this op or `RegionBranchPoint::parent()`. In the latter case, the branch
- originates from outside of the op, i.e., when first executing this op.
-
- A "region successor" indicates the target of a branch. It can indicate
- either a region of this op or this op itself. In the former case, the region
- successor is a region pointer and a range of block arguments to which the
- "successor operands" are forwarded to. In the latter case, the control flow
- leaves this op and the region successor is a range of results of this op to
- which the successor operands are forwarded to.
+ can indicate:
+ 1. A terminator in any of the immediately nested region of this op.
+ 2. `RegionBranchPoint::parent()`: the branch originates from outside of the
+ op, i.e., when first executing this op.
+
+ When branching from a region branch point to a region successor, the
+ "successor operands" to be forwarded from the region branch point can be
+ specified with `getEntrySuccessorOperands` /
+ `RegionBranchTerminatorOpInterface::getSuccessorOperands`.
+
+ A "region successor" indicates the target of a branch. It can indicate:
+ 1. A region of this op and a range of entry block arguments ("successor
+ inputs") to which the successor operands are forwarded to.
+ 2. `RegionSuccessor::parent()` and a range of op results of this op
+ ("successor inputs") to which the successor operands are forwarded to.
+ The control flow leaves this op.
By default, successor operands and successor block arguments/successor
results must have the same type. `areTypesCompatible` can be implemented to
@@ -151,19 +157,22 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
```
- `scf.for` has one region. The `scf.yield` has two region successors: the
- region body itself and the `scf.for` op. `%b` is an entry successor
- operand. `%c` is a successor operand. `%a` is a successor block argument.
- `%r` is a successor result.
+ `scf.for` has one region. There are two region branch points with two
+ identical region successors:
+ * parent => parent(%r), region0(%a)
+ * `scf.yield` => parent(%r), region0(%a)
+
+ `%a` and %r are successor inputs. `%b` is an entry successor operand. `%c`
+ is a successor operand.
}];
let cppNamespace = "::mlir";
let methods = [
InterfaceMethod<[{
- Returns the operands of this operation that are forwarded to the region
- successor's block arguments or this operation's results when branching
- to `successor`. `successor` is guaranteed to be among the successors that are
- returned by `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
+ Returns the operands of this operation that are forwarded to the
+ successor inputs when branching to `successor`. `successor` is
+ guaranteed to be among the successors that are returned by
+ `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
Example: In the above example, this method returns the operand %b of the
`scf.for` op, regardless of the value of `successor`. I.e., this op always
@@ -215,10 +224,10 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
by `point`.
Example: In the above example, this method returns the region of the
- `scf.for` and this operation for either region branch point (`parent`
- and the region of the `scf.for`). An implementation may choose to filter
- out region successors when it is statically known (e.g., by examining
- the operands of this op) that those successors are not branched to.
+ `scf.for` and `parent` for either region branch point. An implementation
+ may choose to filter out region successors when it is statically known
+ (e.g., by examining the operands of this op) that those successors are
+ not branched to.
}],
"void", "getSuccessorRegions",
(ins "::mlir::RegionBranchPoint":$point,
@@ -227,7 +236,6 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
InterfaceMethod<[{
Returns the potential region successors when branching from any
terminator in `region`.
- These are the regions that may be selected during the flow of control.
}],
"void", "getSuccessorRegions",
(ins "::mlir::Region&":$region,
@@ -244,7 +252,8 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
}]>,
InterfaceMethod<[{
- Returns the potential branching point (predecessors) for a given successor.
+ Returns the potential branching points (predecessors) for a given
+ region successor.
}],
"void", "getPredecessors",
(ins "::mlir::RegionSuccessor":$successor,
@@ -370,7 +379,7 @@ def RegionBranchTerminatorOpInterface :
let description = [{
This interface provides information for branching terminator operations
in the presence of a parent `RegionBranchOpInterface` implementation. It
- specifies which operands are passed to which successor region.
+ specifies which operands are passed to which region successor.
}];
let cppNamespace = "::mlir";
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index 24cb123e51877..be53a4e56f37a 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -109,7 +109,7 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
LDBG() << " Processing region branch operation";
return collectUnderlyingAddressValues2(
- branch, RegionSuccessor(op, op->getResults()), result,
+ branch, RegionSuccessor::parent(op->getResults()), result,
result.getResultNumber(), maxDepth, visited, output);
}
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index b9d861830dd38..31b388ebfcede 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -130,7 +130,7 @@ AbstractSparseForwardDataFlowAnalysis::visitOperation(Operation *op) {
// The results of a region branch operation are determined by control-flow.
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
visitRegionSuccessors(getProgramPointAfter(branch), branch,
- /*successor=*/{branch, branch->getResults()},
+ RegionSuccessor::parent(branch->getResults()),
resultLattices);
return success();
}
@@ -313,8 +313,8 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
visitNonControlFlowArgumentsImpl(
branch,
- RegionSuccessor(
- branch, branch->getResults().slice(firstIndex, inputs.size())),
+ RegionSuccessor::parent(
+ branch->getResults().slice(firstIndex, inputs.size())),
lattices, firstIndex);
} else {
if (!inputs.empty())
diff --git a/mlir/lib/Analysis/SliceWalk.cpp b/mlir/lib/Analysis/SliceWalk.cpp
index 863f260cd4b6a..19636eeccc339 100644
--- a/mlir/lib/Analysis/SliceWalk.cpp
+++ b/mlir/lib/Analysis/SliceWalk.cpp
@@ -114,7 +114,7 @@ mlir::getControlFlowPredecessors(Value value) {
if (!regionOp)
return std::nullopt;
// Add the control flow predecessor operands to the work list.
- RegionSuccessor region(regionOp, regionOp->getResults());
+ RegionSuccessor region = RegionSuccessor::parent(regionOp->getResults());
SmallVector<Value> predecessorOperands = getRegionPredecessorOperands(
regionOp, region, opResult.getResultNumber());
return predecessorOperands;
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index c6addfb010856..df1b93e367fc6 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -2741,7 +2741,7 @@ void AffineForOp::getSuccessorRegions(
// From the loop body, if the trip count is one, we can only branch back
// to the parent.
if (tripCount == 1) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
if (tripCount == 0)
@@ -2752,7 +2752,7 @@ void AffineForOp::getSuccessorRegions(
return;
}
if (tripCount.value() == 0) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
}
@@ -2761,7 +2761,7 @@ void AffineForOp::getSuccessorRegions(
// In all other cases, the loop may branch back to itself or the parent
// operation.
regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs()));
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
AffineBound AffineForOp::getLowerBound() {
@@ -3150,7 +3150,7 @@ void AffineIfOp::getSuccessorRegions(
RegionSuccessor(&getThenRegion(), getThenRegion().getArguments()));
// If the "else" region is empty, branch bach into parent.
if (getElseRegion().empty()) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
} else {
regions.push_back(
RegionSuccessor(&getElseRegion(), getElseRegion().getArguments()));
@@ -3160,7 +3160,7 @@ void AffineIfOp::getSuccessorRegions(
// If the predecessor is the `else`/`then` region, then branching into parent
// op is valid.
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
LogicalResult AffineIfOp::verify() {
diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp
index e19b917787df1..11fd87ed925d8 100644
--- a/mlir/lib/Dialect/Async/IR/Async.cpp
+++ b/mlir/lib/Dialect/Async/IR/Async.cpp
@@ -55,7 +55,7 @@ void ExecuteOp::getSuccessorRegions(RegionBranchPoint point,
if (!point.isParent() &&
point.getTerminatorPredecessorOrNull()->getParentRegion() ==
&getBodyRegion()) {
- regions.push_back(RegionSuccessor(getOperation(), getBodyResults()));
+ regions.push_back(RegionSuccessor::parent(getBodyResults()));
return;
}
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
index 36a759c279eb7..9e8746cb8ea35 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
@@ -564,8 +564,8 @@ BufferDeallocation::updateFunctionSignature(FunctionOpInterface op) {
op.getFunctionBody().getOps<RegionBranchTerminatorOpInterface>(),
[&](RegionBranchTerminatorOpInterface branchOp) {
return branchOp
- .getSuccessorOperands(RegionSuccessor(
- op.getOperation(), op.getOperation()->getResults()))
+ .getSuccessorOperands(
+ RegionSuccessor::parent(op.getOperation()->getResults()))
.getTypes();
}));
if (!llvm::all_equal(returnOperandTypes))
@@ -946,7 +946,7 @@ BufferDeallocation::handleInterface(RegionBranchTerminatorOpInterface op) {
// which condition they are taken, etc.
MutableOperandRange operands = op.getMutableSuccessorOperands(
- RegionSuccessor(op.getOperation(), op.getOperation()->getResults()));
+ RegionSuccessor::parent(op.getOperation()->getResults()));
SmallVector<Value> updatedOwnerships;
auto result = deallocation_impl::insertDeallocOpForReturnLike(
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index b0566dd10f490..ca29ff833535d 100644
--- a/mlir/lib/Dialect/EmitC/IR/Emit...
[truncated]
|
Member
|
@llvm/pr-subscribers-mlir-async Author: Matthias Springer (matthias-springer) ChangesSimplify the design of Add a new helper function
Also clean up the documentation to use the same terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from #161575. That PR introduced Patch is 34.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174945.diff 21 Files Affected:
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 063896b00a5a5..91abd97fd75ea 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -1167,8 +1167,7 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
SmallVectorImpl<RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1218,7 +1217,7 @@ void cir::ScopeOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The only region always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(getODSResults(0)));
return;
}
@@ -1383,8 +1382,7 @@ Block *cir::BrCondOp::getSuccessorForOperands(ArrayRef<Attribute> operands) {
void cir::CaseOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
regions.push_back(RegionSuccessor(&getCaseRegion()));
@@ -1410,8 +1408,7 @@ void cir::CaseOp::build(OpBuilder &builder, OperationState &result,
void cir::SwitchOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ion) {
if (!point.isParent()) {
- region.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ region.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1625,8 +1622,7 @@ void cir::GlobalOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `ctor` and `dtor` regions always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -2311,7 +2307,7 @@ void cir::TernaryOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `true` and the `false` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), this->getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(this->getODSResults(0)));
return;
}
@@ -2518,8 +2514,7 @@ void cir::AwaitOp::getSuccessorRegions(
// If any index all the underlying regions branch back to the parent
// operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -3433,8 +3428,7 @@ void cir::TryOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `try` and the `catchers` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 90c960843787e..20dceb21bfecf 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -4699,7 +4699,7 @@ void fir::IfOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(mlir::RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(mlir::RegionSuccessor::parent(getResults()));
return;
}
@@ -4710,7 +4710,7 @@ void fir::IfOp::getSuccessorRegions(
mlir::Region *elseRegion = &this->getElseRegion();
if (elseRegion->empty())
regions.push_back(
- mlir::RegionSuccessor(getOperation(), getOperation()->getResults()));
+ mlir::RegionSuccessor::parent(getOperation()->getResults()));
else
regions.push_back(mlir::RegionSuccessor(elseRegion));
}
@@ -4729,7 +4729,7 @@ void fir::IfOp::getEntrySuccessorRegions(
if (!getElseRegion().empty())
regions.emplace_back(&getElseRegion());
else
- regions.emplace_back(getOperation(), getOperation()->getResults());
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
}
}
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
index b76c2891fad5a..3b2b0ba50ebb4 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
@@ -210,19 +210,19 @@ class RegionSuccessor {
: successor(region), inputs(regionInputs) {
assert(region && "Region must not be null");
}
+
/// Initialize a successor that branches back to/out of the parent operation.
/// The target must be one of the recursive parent operations.
- RegionSuccessor(Operation *successorOp, Operation::result_range results)
- : successor(successorOp), inputs(ValueRange(results)) {
- assert(successorOp && "Successor op must not be null");
+ static RegionSuccessor parent(Operation::result_range results) {
+ return RegionSuccessor(results);
}
/// Return the given region successor. Returns nullptr if the successor is the
/// parent operation.
- Region *getSuccessor() const { return dyn_cast<Region *>(successor); }
+ Region *getSuccessor() const { return successor; }
/// Return true if the successor is the parent operation.
- bool isParent() const { return isa<Operation *>(successor); }
+ bool isParent() const { return successor == nullptr; }
/// Return the inputs to the successor that are remapped by the exit values of
/// the current region.
@@ -237,7 +237,11 @@ class RegionSuccessor {
}
private:
- llvm::PointerUnion<Region *, Operation *> successor{nullptr};
+ /// Private constructor to encourage the use of `RegionSuccessor::parent`.
+ RegionSuccessor(Operation::result_range results)
+ : successor(nullptr), inputs(ValueRange(results)) {}
+
+ Region *successor = nullptr;
ValueRange inputs;
};
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index ecad424e30c75..ff99e220c179f 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -117,25 +117,31 @@ def BranchOpInterface : OpInterface<"BranchOpInterface"> {
def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
let description = [{
- This interface provides information for region-holding operations that exhibit
- branching behavior between held regions. I.e., this interface allows for
- expressing control flow information for region holding operations.
+ This interface provides information for region-holding operations that
+ exhibit branching behavior between held regions. I.e., this interface allows
+ for expressing control flow information for region holding operations.
This interface is meant to model well-defined cases of control-flow and
value propagation, where what occurs along control-flow edges is assumed to
be side-effect free.
A "region branch point" indicates a point from which a branch originates. It
- can indicate either a terminator in any of the immediately nested region of
- this op or `RegionBranchPoint::parent()`. In the latter case, the branch
- originates from outside of the op, i.e., when first executing this op.
-
- A "region successor" indicates the target of a branch. It can indicate
- either a region of this op or this op itself. In the former case, the region
- successor is a region pointer and a range of block arguments to which the
- "successor operands" are forwarded to. In the latter case, the control flow
- leaves this op and the region successor is a range of results of this op to
- which the successor operands are forwarded to.
+ can indicate:
+ 1. A terminator in any of the immediately nested region of this op.
+ 2. `RegionBranchPoint::parent()`: the branch originates from outside of the
+ op, i.e., when first executing this op.
+
+ When branching from a region branch point to a region successor, the
+ "successor operands" to be forwarded from the region branch point can be
+ specified with `getEntrySuccessorOperands` /
+ `RegionBranchTerminatorOpInterface::getSuccessorOperands`.
+
+ A "region successor" indicates the target of a branch. It can indicate:
+ 1. A region of this op and a range of entry block arguments ("successor
+ inputs") to which the successor operands are forwarded to.
+ 2. `RegionSuccessor::parent()` and a range of op results of this op
+ ("successor inputs") to which the successor operands are forwarded to.
+ The control flow leaves this op.
By default, successor operands and successor block arguments/successor
results must have the same type. `areTypesCompatible` can be implemented to
@@ -151,19 +157,22 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
```
- `scf.for` has one region. The `scf.yield` has two region successors: the
- region body itself and the `scf.for` op. `%b` is an entry successor
- operand. `%c` is a successor operand. `%a` is a successor block argument.
- `%r` is a successor result.
+ `scf.for` has one region. There are two region branch points with two
+ identical region successors:
+ * parent => parent(%r), region0(%a)
+ * `scf.yield` => parent(%r), region0(%a)
+
+ `%a` and %r are successor inputs. `%b` is an entry successor operand. `%c`
+ is a successor operand.
}];
let cppNamespace = "::mlir";
let methods = [
InterfaceMethod<[{
- Returns the operands of this operation that are forwarded to the region
- successor's block arguments or this operation's results when branching
- to `successor`. `successor` is guaranteed to be among the successors that are
- returned by `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
+ Returns the operands of this operation that are forwarded to the
+ successor inputs when branching to `successor`. `successor` is
+ guaranteed to be among the successors that are returned by
+ `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
Example: In the above example, this method returns the operand %b of the
`scf.for` op, regardless of the value of `successor`. I.e., this op always
@@ -215,10 +224,10 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
by `point`.
Example: In the above example, this method returns the region of the
- `scf.for` and this operation for either region branch point (`parent`
- and the region of the `scf.for`). An implementation may choose to filter
- out region successors when it is statically known (e.g., by examining
- the operands of this op) that those successors are not branched to.
+ `scf.for` and `parent` for either region branch point. An implementation
+ may choose to filter out region successors when it is statically known
+ (e.g., by examining the operands of this op) that those successors are
+ not branched to.
}],
"void", "getSuccessorRegions",
(ins "::mlir::RegionBranchPoint":$point,
@@ -227,7 +236,6 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
InterfaceMethod<[{
Returns the potential region successors when branching from any
terminator in `region`.
- These are the regions that may be selected during the flow of control.
}],
"void", "getSuccessorRegions",
(ins "::mlir::Region&":$region,
@@ -244,7 +252,8 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
}]>,
InterfaceMethod<[{
- Returns the potential branching point (predecessors) for a given successor.
+ Returns the potential branching points (predecessors) for a given
+ region successor.
}],
"void", "getPredecessors",
(ins "::mlir::RegionSuccessor":$successor,
@@ -370,7 +379,7 @@ def RegionBranchTerminatorOpInterface :
let description = [{
This interface provides information for branching terminator operations
in the presence of a parent `RegionBranchOpInterface` implementation. It
- specifies which operands are passed to which successor region.
+ specifies which operands are passed to which region successor.
}];
let cppNamespace = "::mlir";
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index 24cb123e51877..be53a4e56f37a 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -109,7 +109,7 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
LDBG() << " Processing region branch operation";
return collectUnderlyingAddressValues2(
- branch, RegionSuccessor(op, op->getResults()), result,
+ branch, RegionSuccessor::parent(op->getResults()), result,
result.getResultNumber(), maxDepth, visited, output);
}
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index b9d861830dd38..31b388ebfcede 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -130,7 +130,7 @@ AbstractSparseForwardDataFlowAnalysis::visitOperation(Operation *op) {
// The results of a region branch operation are determined by control-flow.
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
visitRegionSuccessors(getProgramPointAfter(branch), branch,
- /*successor=*/{branch, branch->getResults()},
+ RegionSuccessor::parent(branch->getResults()),
resultLattices);
return success();
}
@@ -313,8 +313,8 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
visitNonControlFlowArgumentsImpl(
branch,
- RegionSuccessor(
- branch, branch->getResults().slice(firstIndex, inputs.size())),
+ RegionSuccessor::parent(
+ branch->getResults().slice(firstIndex, inputs.size())),
lattices, firstIndex);
} else {
if (!inputs.empty())
diff --git a/mlir/lib/Analysis/SliceWalk.cpp b/mlir/lib/Analysis/SliceWalk.cpp
index 863f260cd4b6a..19636eeccc339 100644
--- a/mlir/lib/Analysis/SliceWalk.cpp
+++ b/mlir/lib/Analysis/SliceWalk.cpp
@@ -114,7 +114,7 @@ mlir::getControlFlowPredecessors(Value value) {
if (!regionOp)
return std::nullopt;
// Add the control flow predecessor operands to the work list.
- RegionSuccessor region(regionOp, regionOp->getResults());
+ RegionSuccessor region = RegionSuccessor::parent(regionOp->getResults());
SmallVector<Value> predecessorOperands = getRegionPredecessorOperands(
regionOp, region, opResult.getResultNumber());
return predecessorOperands;
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index c6addfb010856..df1b93e367fc6 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -2741,7 +2741,7 @@ void AffineForOp::getSuccessorRegions(
// From the loop body, if the trip count is one, we can only branch back
// to the parent.
if (tripCount == 1) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
if (tripCount == 0)
@@ -2752,7 +2752,7 @@ void AffineForOp::getSuccessorRegions(
return;
}
if (tripCount.value() == 0) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
}
@@ -2761,7 +2761,7 @@ void AffineForOp::getSuccessorRegions(
// In all other cases, the loop may branch back to itself or the parent
// operation.
regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs()));
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
AffineBound AffineForOp::getLowerBound() {
@@ -3150,7 +3150,7 @@ void AffineIfOp::getSuccessorRegions(
RegionSuccessor(&getThenRegion(), getThenRegion().getArguments()));
// If the "else" region is empty, branch bach into parent.
if (getElseRegion().empty()) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
} else {
regions.push_back(
RegionSuccessor(&getElseRegion(), getElseRegion().getArguments()));
@@ -3160,7 +3160,7 @@ void AffineIfOp::getSuccessorRegions(
// If the predecessor is the `else`/`then` region, then branching into parent
// op is valid.
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
LogicalResult AffineIfOp::verify() {
diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp
index e19b917787df1..11fd87ed925d8 100644
--- a/mlir/lib/Dialect/Async/IR/Async.cpp
+++ b/mlir/lib/Dialect/Async/IR/Async.cpp
@@ -55,7 +55,7 @@ void ExecuteOp::getSuccessorRegions(RegionBranchPoint point,
if (!point.isParent() &&
point.getTerminatorPredecessorOrNull()->getParentRegion() ==
&getBodyRegion()) {
- regions.push_back(RegionSuccessor(getOperation(), getBodyResults()));
+ regions.push_back(RegionSuccessor::parent(getBodyResults()));
return;
}
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
index 36a759c279eb7..9e8746cb8ea35 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
@@ -564,8 +564,8 @@ BufferDeallocation::updateFunctionSignature(FunctionOpInterface op) {
op.getFunctionBody().getOps<RegionBranchTerminatorOpInterface>(),
[&](RegionBranchTerminatorOpInterface branchOp) {
return branchOp
- .getSuccessorOperands(RegionSuccessor(
- op.getOperation(), op.getOperation()->getResults()))
+ .getSuccessorOperands(
+ RegionSuccessor::parent(op.getOperation()->getResults()))
.getTypes();
}));
if (!llvm::all_equal(returnOperandTypes))
@@ -946,7 +946,7 @@ BufferDeallocation::handleInterface(RegionBranchTerminatorOpInterface op) {
// which condition they are taken, etc.
MutableOperandRange operands = op.getMutableSuccessorOperands(
- RegionSuccessor(op.getOperation(), op.getOperation()->getResults()));
+ RegionSuccessor::parent(op.getOperation()->getResults()));
SmallVector<Value> updatedOwnerships;
auto result = deallocation_impl::insertDeallocOpForReturnLike(
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index b0566dd10f490..ca29ff833535d 100644
--- a/mlir/lib/Dialect/EmitC/IR/Emit...
[truncated]
|
Member
|
@llvm/pr-subscribers-openacc Author: Matthias Springer (matthias-springer) ChangesSimplify the design of Add a new helper function
Also clean up the documentation to use the same terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from #161575. That PR introduced Patch is 34.40 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/174945.diff 21 Files Affected:
diff --git a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
index 063896b00a5a5..91abd97fd75ea 100644
--- a/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
+++ b/clang/lib/CIR/Dialect/IR/CIRDialect.cpp
@@ -1167,8 +1167,7 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
SmallVectorImpl<RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1218,7 +1217,7 @@ void cir::ScopeOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The only region always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(getODSResults(0)));
return;
}
@@ -1383,8 +1382,7 @@ Block *cir::BrCondOp::getSuccessorForOperands(ArrayRef<Attribute> operands) {
void cir::CaseOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
regions.push_back(RegionSuccessor(&getCaseRegion()));
@@ -1410,8 +1408,7 @@ void cir::CaseOp::build(OpBuilder &builder, OperationState &result,
void cir::SwitchOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ion) {
if (!point.isParent()) {
- region.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ region.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -1625,8 +1622,7 @@ void cir::GlobalOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `ctor` and `dtor` regions always branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -2311,7 +2307,7 @@ void cir::TernaryOp::getSuccessorRegions(
mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> ®ions) {
// The `true` and the `false` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(RegionSuccessor(getOperation(), this->getODSResults(0)));
+ regions.push_back(RegionSuccessor::parent(this->getODSResults(0)));
return;
}
@@ -2518,8 +2514,7 @@ void cir::AwaitOp::getSuccessorRegions(
// If any index all the underlying regions branch back to the parent
// operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
@@ -3433,8 +3428,7 @@ void cir::TryOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `try` and the `catchers` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(
- RegionSuccessor(getOperation(), getOperation()->getResults()));
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
return;
}
diff --git a/flang/lib/Optimizer/Dialect/FIROps.cpp b/flang/lib/Optimizer/Dialect/FIROps.cpp
index 90c960843787e..20dceb21bfecf 100644
--- a/flang/lib/Optimizer/Dialect/FIROps.cpp
+++ b/flang/lib/Optimizer/Dialect/FIROps.cpp
@@ -4699,7 +4699,7 @@ void fir::IfOp::getSuccessorRegions(
llvm::SmallVectorImpl<mlir::RegionSuccessor> ®ions) {
// The `then` and the `else` region branch back to the parent operation.
if (!point.isParent()) {
- regions.push_back(mlir::RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(mlir::RegionSuccessor::parent(getResults()));
return;
}
@@ -4710,7 +4710,7 @@ void fir::IfOp::getSuccessorRegions(
mlir::Region *elseRegion = &this->getElseRegion();
if (elseRegion->empty())
regions.push_back(
- mlir::RegionSuccessor(getOperation(), getOperation()->getResults()));
+ mlir::RegionSuccessor::parent(getOperation()->getResults()));
else
regions.push_back(mlir::RegionSuccessor(elseRegion));
}
@@ -4729,7 +4729,7 @@ void fir::IfOp::getEntrySuccessorRegions(
if (!getElseRegion().empty())
regions.emplace_back(&getElseRegion());
else
- regions.emplace_back(getOperation(), getOperation()->getResults());
+ regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
}
}
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
index b76c2891fad5a..3b2b0ba50ebb4 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.h
@@ -210,19 +210,19 @@ class RegionSuccessor {
: successor(region), inputs(regionInputs) {
assert(region && "Region must not be null");
}
+
/// Initialize a successor that branches back to/out of the parent operation.
/// The target must be one of the recursive parent operations.
- RegionSuccessor(Operation *successorOp, Operation::result_range results)
- : successor(successorOp), inputs(ValueRange(results)) {
- assert(successorOp && "Successor op must not be null");
+ static RegionSuccessor parent(Operation::result_range results) {
+ return RegionSuccessor(results);
}
/// Return the given region successor. Returns nullptr if the successor is the
/// parent operation.
- Region *getSuccessor() const { return dyn_cast<Region *>(successor); }
+ Region *getSuccessor() const { return successor; }
/// Return true if the successor is the parent operation.
- bool isParent() const { return isa<Operation *>(successor); }
+ bool isParent() const { return successor == nullptr; }
/// Return the inputs to the successor that are remapped by the exit values of
/// the current region.
@@ -237,7 +237,11 @@ class RegionSuccessor {
}
private:
- llvm::PointerUnion<Region *, Operation *> successor{nullptr};
+ /// Private constructor to encourage the use of `RegionSuccessor::parent`.
+ RegionSuccessor(Operation::result_range results)
+ : successor(nullptr), inputs(ValueRange(results)) {}
+
+ Region *successor = nullptr;
ValueRange inputs;
};
diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index ecad424e30c75..ff99e220c179f 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -117,25 +117,31 @@ def BranchOpInterface : OpInterface<"BranchOpInterface"> {
def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
let description = [{
- This interface provides information for region-holding operations that exhibit
- branching behavior between held regions. I.e., this interface allows for
- expressing control flow information for region holding operations.
+ This interface provides information for region-holding operations that
+ exhibit branching behavior between held regions. I.e., this interface allows
+ for expressing control flow information for region holding operations.
This interface is meant to model well-defined cases of control-flow and
value propagation, where what occurs along control-flow edges is assumed to
be side-effect free.
A "region branch point" indicates a point from which a branch originates. It
- can indicate either a terminator in any of the immediately nested region of
- this op or `RegionBranchPoint::parent()`. In the latter case, the branch
- originates from outside of the op, i.e., when first executing this op.
-
- A "region successor" indicates the target of a branch. It can indicate
- either a region of this op or this op itself. In the former case, the region
- successor is a region pointer and a range of block arguments to which the
- "successor operands" are forwarded to. In the latter case, the control flow
- leaves this op and the region successor is a range of results of this op to
- which the successor operands are forwarded to.
+ can indicate:
+ 1. A terminator in any of the immediately nested region of this op.
+ 2. `RegionBranchPoint::parent()`: the branch originates from outside of the
+ op, i.e., when first executing this op.
+
+ When branching from a region branch point to a region successor, the
+ "successor operands" to be forwarded from the region branch point can be
+ specified with `getEntrySuccessorOperands` /
+ `RegionBranchTerminatorOpInterface::getSuccessorOperands`.
+
+ A "region successor" indicates the target of a branch. It can indicate:
+ 1. A region of this op and a range of entry block arguments ("successor
+ inputs") to which the successor operands are forwarded to.
+ 2. `RegionSuccessor::parent()` and a range of op results of this op
+ ("successor inputs") to which the successor operands are forwarded to.
+ The control flow leaves this op.
By default, successor operands and successor block arguments/successor
results must have the same type. `areTypesCompatible` can be implemented to
@@ -151,19 +157,22 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
```
- `scf.for` has one region. The `scf.yield` has two region successors: the
- region body itself and the `scf.for` op. `%b` is an entry successor
- operand. `%c` is a successor operand. `%a` is a successor block argument.
- `%r` is a successor result.
+ `scf.for` has one region. There are two region branch points with two
+ identical region successors:
+ * parent => parent(%r), region0(%a)
+ * `scf.yield` => parent(%r), region0(%a)
+
+ `%a` and %r are successor inputs. `%b` is an entry successor operand. `%c`
+ is a successor operand.
}];
let cppNamespace = "::mlir";
let methods = [
InterfaceMethod<[{
- Returns the operands of this operation that are forwarded to the region
- successor's block arguments or this operation's results when branching
- to `successor`. `successor` is guaranteed to be among the successors that are
- returned by `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
+ Returns the operands of this operation that are forwarded to the
+ successor inputs when branching to `successor`. `successor` is
+ guaranteed to be among the successors that are returned by
+ `getEntrySuccessorRegions`/`getSuccessorRegions(parent())`.
Example: In the above example, this method returns the operand %b of the
`scf.for` op, regardless of the value of `successor`. I.e., this op always
@@ -215,10 +224,10 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
by `point`.
Example: In the above example, this method returns the region of the
- `scf.for` and this operation for either region branch point (`parent`
- and the region of the `scf.for`). An implementation may choose to filter
- out region successors when it is statically known (e.g., by examining
- the operands of this op) that those successors are not branched to.
+ `scf.for` and `parent` for either region branch point. An implementation
+ may choose to filter out region successors when it is statically known
+ (e.g., by examining the operands of this op) that those successors are
+ not branched to.
}],
"void", "getSuccessorRegions",
(ins "::mlir::RegionBranchPoint":$point,
@@ -227,7 +236,6 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
InterfaceMethod<[{
Returns the potential region successors when branching from any
terminator in `region`.
- These are the regions that may be selected during the flow of control.
}],
"void", "getSuccessorRegions",
(ins "::mlir::Region&":$region,
@@ -244,7 +252,8 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
}
}]>,
InterfaceMethod<[{
- Returns the potential branching point (predecessors) for a given successor.
+ Returns the potential branching points (predecessors) for a given
+ region successor.
}],
"void", "getPredecessors",
(ins "::mlir::RegionSuccessor":$successor,
@@ -370,7 +379,7 @@ def RegionBranchTerminatorOpInterface :
let description = [{
This interface provides information for branching terminator operations
in the presence of a parent `RegionBranchOpInterface` implementation. It
- specifies which operands are passed to which successor region.
+ specifies which operands are passed to which region successor.
}];
let cppNamespace = "::mlir";
diff --git a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
index 24cb123e51877..be53a4e56f37a 100644
--- a/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
+++ b/mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp
@@ -109,7 +109,7 @@ static void collectUnderlyingAddressValues(OpResult result, unsigned maxDepth,
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
LDBG() << " Processing region branch operation";
return collectUnderlyingAddressValues2(
- branch, RegionSuccessor(op, op->getResults()), result,
+ branch, RegionSuccessor::parent(op->getResults()), result,
result.getResultNumber(), maxDepth, visited, output);
}
diff --git a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
index b9d861830dd38..31b388ebfcede 100644
--- a/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
+++ b/mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp
@@ -130,7 +130,7 @@ AbstractSparseForwardDataFlowAnalysis::visitOperation(Operation *op) {
// The results of a region branch operation are determined by control-flow.
if (auto branch = dyn_cast<RegionBranchOpInterface>(op)) {
visitRegionSuccessors(getProgramPointAfter(branch), branch,
- /*successor=*/{branch, branch->getResults()},
+ RegionSuccessor::parent(branch->getResults()),
resultLattices);
return success();
}
@@ -313,8 +313,8 @@ void AbstractSparseForwardDataFlowAnalysis::visitRegionSuccessors(
firstIndex = cast<OpResult>(inputs.front()).getResultNumber();
visitNonControlFlowArgumentsImpl(
branch,
- RegionSuccessor(
- branch, branch->getResults().slice(firstIndex, inputs.size())),
+ RegionSuccessor::parent(
+ branch->getResults().slice(firstIndex, inputs.size())),
lattices, firstIndex);
} else {
if (!inputs.empty())
diff --git a/mlir/lib/Analysis/SliceWalk.cpp b/mlir/lib/Analysis/SliceWalk.cpp
index 863f260cd4b6a..19636eeccc339 100644
--- a/mlir/lib/Analysis/SliceWalk.cpp
+++ b/mlir/lib/Analysis/SliceWalk.cpp
@@ -114,7 +114,7 @@ mlir::getControlFlowPredecessors(Value value) {
if (!regionOp)
return std::nullopt;
// Add the control flow predecessor operands to the work list.
- RegionSuccessor region(regionOp, regionOp->getResults());
+ RegionSuccessor region = RegionSuccessor::parent(regionOp->getResults());
SmallVector<Value> predecessorOperands = getRegionPredecessorOperands(
regionOp, region, opResult.getResultNumber());
return predecessorOperands;
diff --git a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
index c6addfb010856..df1b93e367fc6 100644
--- a/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
+++ b/mlir/lib/Dialect/Affine/IR/AffineOps.cpp
@@ -2741,7 +2741,7 @@ void AffineForOp::getSuccessorRegions(
// From the loop body, if the trip count is one, we can only branch back
// to the parent.
if (tripCount == 1) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
if (tripCount == 0)
@@ -2752,7 +2752,7 @@ void AffineForOp::getSuccessorRegions(
return;
}
if (tripCount.value() == 0) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
return;
}
}
@@ -2761,7 +2761,7 @@ void AffineForOp::getSuccessorRegions(
// In all other cases, the loop may branch back to itself or the parent
// operation.
regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs()));
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
AffineBound AffineForOp::getLowerBound() {
@@ -3150,7 +3150,7 @@ void AffineIfOp::getSuccessorRegions(
RegionSuccessor(&getThenRegion(), getThenRegion().getArguments()));
// If the "else" region is empty, branch bach into parent.
if (getElseRegion().empty()) {
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
} else {
regions.push_back(
RegionSuccessor(&getElseRegion(), getElseRegion().getArguments()));
@@ -3160,7 +3160,7 @@ void AffineIfOp::getSuccessorRegions(
// If the predecessor is the `else`/`then` region, then branching into parent
// op is valid.
- regions.push_back(RegionSuccessor(getOperation(), getResults()));
+ regions.push_back(RegionSuccessor::parent(getResults()));
}
LogicalResult AffineIfOp::verify() {
diff --git a/mlir/lib/Dialect/Async/IR/Async.cpp b/mlir/lib/Dialect/Async/IR/Async.cpp
index e19b917787df1..11fd87ed925d8 100644
--- a/mlir/lib/Dialect/Async/IR/Async.cpp
+++ b/mlir/lib/Dialect/Async/IR/Async.cpp
@@ -55,7 +55,7 @@ void ExecuteOp::getSuccessorRegions(RegionBranchPoint point,
if (!point.isParent() &&
point.getTerminatorPredecessorOrNull()->getParentRegion() ==
&getBodyRegion()) {
- regions.push_back(RegionSuccessor(getOperation(), getBodyResults()));
+ regions.push_back(RegionSuccessor::parent(getBodyResults()));
return;
}
diff --git a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
index 36a759c279eb7..9e8746cb8ea35 100644
--- a/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
+++ b/mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp
@@ -564,8 +564,8 @@ BufferDeallocation::updateFunctionSignature(FunctionOpInterface op) {
op.getFunctionBody().getOps<RegionBranchTerminatorOpInterface>(),
[&](RegionBranchTerminatorOpInterface branchOp) {
return branchOp
- .getSuccessorOperands(RegionSuccessor(
- op.getOperation(), op.getOperation()->getResults()))
+ .getSuccessorOperands(
+ RegionSuccessor::parent(op.getOperation()->getResults()))
.getTypes();
}));
if (!llvm::all_equal(returnOperandTypes))
@@ -946,7 +946,7 @@ BufferDeallocation::handleInterface(RegionBranchTerminatorOpInterface op) {
// which condition they are taken, etc.
MutableOperandRange operands = op.getMutableSuccessorOperands(
- RegionSuccessor(op.getOperation(), op.getOperation()->getResults()));
+ RegionSuccessor::parent(op.getOperation()->getResults()));
SmallVector<Value> updatedOwnerships;
auto result = deallocation_impl::insertDeallocOpForReturnLike(
diff --git a/mlir/lib/Dialect/EmitC/IR/EmitC.cpp b/mlir/lib/Dialect/EmitC/IR/EmitC.cpp
index b0566dd10f490..ca29ff833535d 100644
--- a/mlir/lib/Dialect/EmitC/IR/Emit...
[truncated]
|
🪟 Windows x64 Test Results
✅ The build succeeded and all tests passed. |
🐧 Linux x64 Test Results
✅ The build succeeded and all tests passed. |
matthias-springer
force-pushed
the
users/matthias-springer/region_successor
branch
2 times, most recently
from
d49fe5f to
f15ee78
Compare
matthias-springer
force-pushed
the
users/matthias-springer/region_successor
branch
2 times, most recently
from
dcdcc3d to
b5f98fb
Compare
matthias-springer
force-pushed
the
users/matthias-springer/region_successor
branch
from
b5f98fb to
ca09188
Compare
matthias-springer
changed the title
RegionSuccessor design / APIRegionSuccessor design / API
bcardosolopes
approved these changes
Jan 13, 2026
Member
bcardosolopes
left a comment
bcardosolopes
left a comment
There was a problem hiding this comment.
Thanks for fixing the CIR bits, from that perspective LGTM
linuxlonelyeagle
approved these changes
Jan 14, 2026
matthias-springer
deleted the
users/matthias-springer/region_successor
branch
Member
|
Just for context the intention of the I don't have a strong opinion whether it makes sense to have this functionality in-tree already tho or whether a future PR with signaling control flow should reintroduce this, but I suppose the cost of the latter should be low while the interface is being simplified. |
krzysz00
added a commit
to iree-org/llvm-project
that referenced
this pull request
Jan 15, 2026
…design / API (llvm#174945)" This reverts commit 5f3b40e.
Priyanshu3820
pushed a commit
to Priyanshu3820/llvm-project
that referenced
this pull request
Jan 18, 2026
… API (llvm#174945) Simplify the design of `RegionSuccessor`. There is no need to store the `Operation *` pointer when branching out of the region branch op (to the parent). There is no API to even access the `Operation *` pointer. Add a new helper function `RegionSuccessor::parent` to construct a region successor that points to the parent. This aligns the `RegionSuccessor` design and API with `RegionBranchPoint`: * Both classes now have a `parent()` helper function. `ClassName::parent()` can be used in documentation to precisely describe the source/target of a region branch. * Both classes now use `nullptr` internally to represent "parent". This API change also protects against incorrect API usage: users can no longer pass an incorrect parent op. If a region successor is not a region of the region branch op, it *must* branch out of region branch op itself ("parent"). However, the previous API allowed passing other operations. There was one such API violation in a [test case](https://github.com/llvm/llvm-project/pull/174945/files#diff-d5717e4a8d7344b2ff77762b8fa480bcfec0eeee97a86195c787d791a6217e13L71). Also clean up the documentation to use the correct terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from llvm#161575. That PR introduced `llvm::PointerUnion<Region *, Operation *> successor{nullptr};`. It is unclear from the commit message why that change was made. Note for LLVM integration: You may have to slightly modify `getSuccessorRegion` implementations: Replace `RegionSuccessor(getOperation(), getOperation()->getResults())` with `RegionSuccessor::parent(getResults())`.
kuhar
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 20, 2026
Reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. New reverts: * Revert llvm/llvm-project#174945 until torch-mlir adapts to the change Local changes: * llvm/llvm-project#172595 disables a load->extract folder on to_buffer ops when the buffer isn't declared read_only. IREE would only mark to_buffer operations on tensor constants as read_only when the constant only has one use, for reasons I'm not clear on and that I couldn't find. That restriction was leading to some of the winnograd tests failing because they couldn't bufferize. So, we drop the one use restriction in order to make everything pass again. (Claude was used to find this one.)
amd-eochoalo
pushed a commit
to iree-org/llvm-project
that referenced
this pull request
Jan 20, 2026
…design / API (llvm#174945)" This reverts commit 5f3b40e.
amd-eochoalo
added a commit
to iree-org/iree
that referenced
this pull request
Jan 20, 2026
Reverts carried forward: Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932 Revert llvm/llvm-project#174945 until torch-mlir adapts to the change
amd-eochoalo
added a commit
to iree-org/iree
that referenced
this pull request
Jan 21, 2026
Reverts carried forward: * Local revert of llvm/llvm-project#169614 due to #22649 Reverts dropped: * Revert llvm/llvm-project#174945 (torch also updated accordingly) Also includes torch integrate. Non-trivial changes include changes to the RegionSuccessor API.
BStott6
pushed a commit
to BStott6/llvm-project
that referenced
this pull request
Jan 22, 2026
… API (llvm#174945) Simplify the design of `RegionSuccessor`. There is no need to store the `Operation *` pointer when branching out of the region branch op (to the parent). There is no API to even access the `Operation *` pointer. Add a new helper function `RegionSuccessor::parent` to construct a region successor that points to the parent. This aligns the `RegionSuccessor` design and API with `RegionBranchPoint`: * Both classes now have a `parent()` helper function. `ClassName::parent()` can be used in documentation to precisely describe the source/target of a region branch. * Both classes now use `nullptr` internally to represent "parent". This API change also protects against incorrect API usage: users can no longer pass an incorrect parent op. If a region successor is not a region of the region branch op, it *must* branch out of region branch op itself ("parent"). However, the previous API allowed passing other operations. There was one such API violation in a [test case](https://github.com/llvm/llvm-project/pull/174945/files#diff-d5717e4a8d7344b2ff77762b8fa480bcfec0eeee97a86195c787d791a6217e13L71). Also clean up the documentation to use the correct terminology (such as "successor operands", "successor inputs") consistently. Note: This PR effectively rolls back some changes from llvm#161575. That PR introduced `llvm::PointerUnion<Region *, Operation *> successor{nullptr};`. It is unclear from the commit message why that change was made. Note for LLVM integration: You may have to slightly modify `getSuccessorRegion` implementations: Replace `RegionSuccessor(getOperation(), getOperation()->getResults())` with `RegionSuccessor::parent(getResults())`.
keshavvinayak01
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 27, 2026
Reverts carried forward: * Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932. New reverts: * Revert llvm/llvm-project#174945 until torch-mlir adapts to the change Local changes: * llvm/llvm-project#172595 disables a load->extract folder on to_buffer ops when the buffer isn't declared read_only. IREE would only mark to_buffer operations on tensor constants as read_only when the constant only has one use, for reasons I'm not clear on and that I couldn't find. That restriction was leading to some of the winnograd tests failing because they couldn't bufferize. So, we drop the one use restriction in order to make everything pass again. (Claude was used to find this one.) Signed-off-by: Keshav Vinayak Jha <[email protected]>
keshavvinayak01
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 27, 2026
Reverts carried forward: Local revert of llvm/llvm-project#169614 due to llvm/llvm-project#172932 Revert llvm/llvm-project#174945 until torch-mlir adapts to the change Signed-off-by: Keshav Vinayak Jha <[email protected]>
keshavvinayak01
pushed a commit
to iree-org/iree
that referenced
this pull request
Jan 27, 2026
Reverts carried forward: * Local revert of llvm/llvm-project#169614 due to #22649 Reverts dropped: * Revert llvm/llvm-project#174945 (torch also updated accordingly) Also includes torch integrate. Non-trivial changes include changes to the RegionSuccessor API. Signed-off-by: Keshav Vinayak Jha <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Simplify the design of
RegionSuccessor. There is no need to store theOperation *pointer when branching out of the region branch op (to the parent). There is no API to even access theOperation *pointer.Add a new helper function
RegionSuccessor::parentto construct a region successor that points to the parent. This aligns theRegionSuccessordesign and API withRegionBranchPoint:parent()helper function.ClassName::parent()can be used in documentation to precisely describe the source/target of a region branch.nullptrinternally to represent "parent".This API change also protects against incorrect API usage: users can no longer pass an incorrect parent op. If a region successor is not a region of the region branch op, it must branch out of region branch op itself ("parent"). However, the previous API allowed passing other operations. There was one such API violation in a test case.
Also clean up the documentation to use the correct terminology (such as "successor operands", "successor inputs") consistently.
Note: This PR effectively rolls back some changes from #161575. That PR introduced
llvm::PointerUnion<Region *, Operation *> successor{nullptr};. It is unclear from the commit message why that change was made.Note for LLVM integration: You may have to slightly modify
getSuccessorRegionimplementations: ReplaceRegionSuccessor(getOperation(), getOperation()->getResults())withRegionSuccessor::parent(getResults()).