[clang] [flang] [mlir] [mlir][Interfaces] Simplify and align `RegionSuccessor` design / API (PR #174945)
via cfe-commits
cfe-commits at lists.llvm.org
Thu Jan 8 02:51:49 PST 2026
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-mlir-gpu
Author: Matthias Springer (matthias-springer)
<details>
<summary>Changes</summary>
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".
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 `llvm::PointerUnion<Region *, Operation *> successor{nullptr};`. It is unclear from the commit message why that change was made.
---
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:
- (modified) clang/lib/CIR/Dialect/IR/CIRDialect.cpp (+8-14)
- (modified) flang/lib/Optimizer/Dialect/FIROps.cpp (+3-3)
- (modified) mlir/include/mlir/Interfaces/ControlFlowInterfaces.h (+10-6)
- (modified) mlir/include/mlir/Interfaces/ControlFlowInterfaces.td (+37-28)
- (modified) mlir/lib/Analysis/AliasAnalysis/LocalAliasAnalysis.cpp (+1-1)
- (modified) mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp (+3-3)
- (modified) mlir/lib/Analysis/SliceWalk.cpp (+1-1)
- (modified) mlir/lib/Dialect/Affine/IR/AffineOps.cpp (+5-5)
- (modified) mlir/lib/Dialect/Async/IR/Async.cpp (+1-1)
- (modified) mlir/lib/Dialect/Bufferization/Transforms/OwnershipBasedBufferDeallocation.cpp (+3-3)
- (modified) mlir/lib/Dialect/EmitC/IR/EmitC.cpp (+4-5)
- (modified) mlir/lib/Dialect/GPU/IR/GPUDialect.cpp (+1-1)
- (modified) mlir/lib/Dialect/MemRef/IR/MemRefOps.cpp (+1-1)
- (modified) mlir/lib/Dialect/OpenACC/IR/OpenACC.cpp (+3-3)
- (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+11-13)
- (modified) mlir/lib/Dialect/Shape/IR/Shape.cpp (+1-1)
- (modified) mlir/lib/Dialect/SparseTensor/IR/SparseTensorDialect.cpp (+1-1)
- (modified) mlir/lib/Dialect/Transform/IR/TransformOps.cpp (+3-3)
- (modified) mlir/lib/Dialect/Transform/TuneExtension/TuneExtensionOps.cpp (+1-1)
- (modified) mlir/test/lib/Dialect/Test/TestOpDefs.cpp (+5-5)
- (modified) mlir/unittests/Interfaces/ControlFlowInterfacesTest.cpp (+3-5)
``````````diff
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]
``````````
</details>
https://github.com/llvm/llvm-project/pull/174945
More information about the cfe-commits
mailing list