[mlir][Interfaces][NFC] Document that RegionBranchTerminatorOpInterface is mandatory

[matthias-springer]

Document that implementing the RegionBranchTerminatorOpInterface is mandatory. Omitting this interface on a block terminator that models region branching may lead to invalid/incomplete analyses and transformations.

This commit does not change the op/interface semantics. It just puts in writing an assumption that is made throughout the code base. For example:

• It is baked into the API design of the RegionBranchOpInterface. You cannot query the region successors of block terminators that do not implement the RegionBranchTerminatorOpInterface: RegionBranchOpInterface::getSuccessors() takes a RegionBranchPoint parameter, and such region branch points can be constructed only from RegionBranchTerminatorOpInterface and not arbitrary Operation *.
• Helper functions + default interface method implementations enumerate region branch points by looking for RegionBranchTerminatorOpInterface and ignoring other operations. E.g., RegionBranchOpInterface::getAllRegionBranchPoints, default implementation of RegionBranchOpInterface::getSuccessorRegions(Region &), default implementation of RegionBranchOpInterface::getPredecessorValues.
• Core analyses such as DeadCodeAnalysis look for RegionBranchTerminatorOpInterface and misbehave when the interface is not implemented. The analysis does not (and cannot) query region successors of a region branching terminator that does not implement the RegionBranchTerminatorOpInterface. In practice, this means that forgetting to implement the RegionBranchTerminatorOpInterface may incorrectly classify regions as dead.
• Other analyses / transformations that look for and depend on the implementation of RegionBranchTerminatorOpInterface: mlir::getControlFlowPredecessors, AbstractDenseBackwardDataFlowAnalysis, ownership-based buffer deallocation pass.

[llvmbot]

@llvm/pr-subscribers-mlir

Author: Matthias Springer (matthias-springer)

Changes

Document that implementing the RegionBranchTerminatorOpInterface is mandatory. Omitting this interface on a block terminator that models region branching may lead to invalid/incomplete analyses and transformations.

This commit does not change the op/interface semantics. It just puts in writing an assumption that is made throughout the code base. For example:

• It is baked into the API design of the RegionBranchOpInterface. You cannot query the region successors of block terminators that do not implement the RegionBranchTerminatorOpInterface: RegionBranchOpInterface::getSuccessors() takes a RegionBranchPoint parameter, and such region branch points can be constructed only from RegionBranchTerminatorOpInterface and not arbitrary Operation *.
• Helper functions + default interface method implementations enumerate region branch points by looking for RegionBranchTerminatorOpInterface and ignoring other operations. E.g., RegionBranchOpInterface::getAllRegionBranchPoints, default implementation of RegionBranchOpInterface::getSuccessorRegions(Region &), default implementation of RegionBranchOpInterface::getPredecessorValues.
• Core analyses such as DeadCodeAnalysis look for RegionBranchTerminatorOpInterface and misbehave when the interface is not implemented. The analysis does not (and cannot) query region successors of a region branching terminator that does not implement the RegionBranchTerminatorOpInterface. In practice, this means that forgetting to implement the RegionBranchTerminatorOpInterface may incorrectly classify regions as dead.
• Other analyses / transformations that look for and depend on the implementation of RegionBranchTerminatorOpInterface: mlir::getControlFlowPredecessors, AbstractDenseBackwardDataFlowAnalysis, ownership-based buffer deallocation pass.

---

Full diff: https://github.com/llvm/llvm-project/pull/174978.diff

1 Files Affected:

• (modified) mlir/include/mlir/Interfaces/ControlFlowInterfaces.td (+13-2)

diff --git a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
index ff99e220c179f..74c7841d52b3f 100644
--- a/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
+++ b/mlir/include/mlir/Interfaces/ControlFlowInterfaces.td
@@ -127,7 +127,8 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
 
     A "region branch point" indicates a point from which a branch originates. It
     can indicate:
-    1. A terminator in any of the immediately nested region of this op.
+    1. A `RegionBranchTerminatorOpInterface` 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.
 
@@ -147,6 +148,15 @@ def RegionBranchOpInterface : OpInterface<"RegionBranchOpInterface"> {
     results must have the same type. `areTypesCompatible` can be implemented to
     allow non-equal types.
 
+    Note: This interface works in conjunction with
+    `RegionBranchTerminatorOpInterface`. All immediately nested block
+    terminators that model branching between regions must implement the
+    `RegionBranchTerminatorOpInterface`. Otherwise, analyses/transformations
+    may miss control flow edges and produce incorrect results. Not every block
+    terminator is necessarily a region branch terminator: e.g., in the presence
+    of unstructured control flow, a block terminator could indicate a branch to
+    a different block within the same region.
+
     Example:
 
     ```
@@ -379,7 +389,8 @@ 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 region successor.
+    acts as a marker for valid region branch points and specifies which
+    operands are passed to which region successor.
   }];
   let cppNamespace = "::mlir";
 

[linuxlonelyeagle]

LGTM.please wait for the other reviewer.

[linuxlonelyeagle]

I am thinking about whether this conclusion is correct: A TerminatorOp does not necessarily have to implement the RegionBranchTerminatorOpInterface. If a TerminatorOp does not implement the RegionBranchTerminatorOpInterface, does that mean it has no successors? If this conclusion is correct, I think it would be better to add it to the assumptions or doc.

[matthias-springer]

Yes, that's correct. I added a note to the RegionBranchTerminatorOpInterface documentation.

[linuxlonelyeagle]

Yes, that's correct. I added a note to the RegionBranchTerminatorOpInterface documentation.

Thanks.

[matthias-springer]

Rebased to main, so that I can merge this before #174945 (which is still in review).