[mlir][Interfaces][NFC] Simplify and align RegionSuccessor design / API

clang/lib/CIR/Dialect/IR/CIRDialect.cpp

@@ -324,7 +324,7 @@ void cir::ConditionOp::getSuccessorRegions(
   // Parent is a loop: condition may branch to the body or to the parent op.
   if (auto loopOp = dyn_cast<LoopOpInterface>(getOperation()->getParentOp())) {
     regions.emplace_back(&loopOp.getBody(), loopOp.getBody().getArguments());
-    regions.emplace_back(getOperation(), loopOp->getResults());
+    regions.push_back(RegionSuccessor::parent(loopOp->getResults()));
   }
 
   // Parent is an await: condition may branch to resume or suspend regions.
@@ -1168,8 +1168,7 @@ void cir::IfOp::getSuccessorRegions(mlir::RegionBranchPoint point,
                                     SmallVectorImpl<RegionSuccessor> &regions) {
   // 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;
   }
 
@@ -1219,7 +1218,7 @@ void cir::ScopeOp::getSuccessorRegions(
     mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &regions) {
   // 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;
   }
 
@@ -1384,8 +1383,7 @@ Block *cir::BrCondOp::getSuccessorForOperands(ArrayRef<Attribute> operands) {
 void cir::CaseOp::getSuccessorRegions(
     mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &regions) {
   if (!point.isParent()) {
-    regions.push_back(
-        RegionSuccessor(getOperation(), getOperation()->getResults()));
+    regions.push_back(RegionSuccessor::parent(getOperation()->getResults()));
     return;
   }
   regions.push_back(RegionSuccessor(&getCaseRegion()));
@@ -1411,8 +1409,7 @@ void cir::CaseOp::build(OpBuilder &builder, OperationState &result,
 void cir::SwitchOp::getSuccessorRegions(
     mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &region) {
   if (!point.isParent()) {
-    region.push_back(
-        RegionSuccessor(getOperation(), getOperation()->getResults()));
+    region.push_back(RegionSuccessor::parent(getOperation()->getResults()));
     return;
   }
 
@@ -1626,8 +1623,7 @@ void cir::GlobalOp::getSuccessorRegions(
     mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &regions) {
   // 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;
   }
 
@@ -2312,7 +2308,7 @@ void cir::TernaryOp::getSuccessorRegions(
     mlir::RegionBranchPoint point, SmallVectorImpl<RegionSuccessor> &regions) {
   // 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;
   }
 
@@ -2617,8 +2613,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;
   }
 
@@ -3532,8 +3527,7 @@ void cir::TryOp::getSuccessorRegions(
     llvm::SmallVectorImpl<mlir::RegionSuccessor> &regions) {
   // 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;
   }
 

clang/lib/CIR/Interfaces/CIRLoopOpInterface.cpp

@@ -30,7 +30,7 @@ void LoopOpInterface::getLoopOpSuccessorRegions(
 
   // Branching from condition: go to body or exit.
   if (&op.getCond() == parentRegion) {
-    regions.emplace_back(mlir::RegionSuccessor(op, op->getResults()));
+    regions.emplace_back(mlir::RegionSuccessor::parent(op->getResults()));
     regions.emplace_back(&op.getBody(), op.getBody().getArguments());
     return;
   }

flang/lib/Optimizer/Dialect/FIROps.cpp

@@ -4699,7 +4699,7 @@ void fir::IfOp::getSuccessorRegions(
     llvm::SmallVectorImpl<mlir::RegionSuccessor> &regions) {
   // 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,8 @@ void fir::IfOp::getEntrySuccessorRegions(
     if (!getElseRegion().empty())
       regions.emplace_back(&getElseRegion());
     else
-      regions.emplace_back(getOperation(), getOperation()->getResults());
+      regions.push_back(
+          mlir::RegionSuccessor::parent(getOperation()->getResults()));
   }
 }
 

mlir/include/mlir/Interfaces/ControlFlowInterfaces.h

@@ -213,19 +213,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.
@@ -240,7 +240,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;
 };
 

mlir/include/mlir/Interfaces/ControlFlowInterfaces.td

@@ -117,26 +117,32 @@ 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 `RegionBranchTerminatorOpInterface` terminator in any
-    of the immediately nested regions 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 `RegionBranchTerminatorOpInterface` terminator in any of the
+       immediately nested regions 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
@@ -162,19 +168,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
@@ -226,10 +235,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,
@@ -238,7 +247,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,
@@ -255,7 +263,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,

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);
   }
 

mlir/lib/Analysis/DataFlow/SparseAnalysis.cpp

@@ -134,7 +134,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();
   }
@@ -317,8 +317,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())

mlir/lib/Analysis/SliceWalk.cpp

@@ -68,7 +68,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;
     // TODO (#175168): This assumes that there are no non-successor-inputs
     // in front of the op result.

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() {

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;
   }
 

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(