[Mlir-commits] [mlir] [mlir][SCF] Remove `RegionBranchOpInterface` from `scf.forall` (PR #174221)

[llvmlistbot at llvm.org]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-mlir-scf

Author: Matthias Springer (matthias-springer)

<details>
<summary>Changes</summary>

`scf.forall` does not completely implement the `RegionBranchOpInterface`. `scf.forall.in_parallel` does not implement the `RegionBranchTerminatorOpInterface` and it is unspecified which values are forwarded from the `in_parallel` op to the successor inputs.

Incomplete interface implementation is a problem for transformations that try to understand the data flow by querying the `RegionBranchOpInterface`, such as the patterns in #<!-- -->174094. Such transformations may be produce incorrect IR.


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


3 Files Affected:

- (modified) mlir/include/mlir/Dialect/SCF/IR/SCFOps.td (-1) 
- (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (-17) 
- (modified) mlir/test/Analysis/DataFlow/test-liveness-analysis.mlir (+1-1) 


``````````diff
diff --git a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
index 8bdf3e0b566ef..8c6c5e56e3645 100644
--- a/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
+++ b/mlir/include/mlir/Dialect/SCF/IR/SCFOps.td
@@ -326,7 +326,6 @@ def ForallOp : SCF_Op<"forall", [
            "promoteIfSingleIteration", "yieldTiledValuesAndReplace"]>,
        RecursiveMemoryEffects,
        SingleBlockImplicitTerminator<"scf::InParallelOp">,
-       DeclareOpInterfaceMethods<RegionBranchOpInterface>,
        DestinationStyleOpInterface,
        HasParallelRegion
      ]> {
diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 8803a6d136f7a..0c6d539be0db4 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -2013,23 +2013,6 @@ void ForallOp::getCanonicalizationPatterns(RewritePatternSet &results,
               ForallOpReplaceConstantInductionVar>(context);
 }
 
-/// Given the region at `index`, or the parent operation if `index` is None,
-/// return the successor regions. These are the regions that may be selected
-/// during the flow of control. `operands` is a set of optional attributes that
-/// correspond to a constant value for each operand, or null if that operand is
-/// not a constant.
-void ForallOp::getSuccessorRegions(RegionBranchPoint point,
-                                   SmallVectorImpl<RegionSuccessor> &regions) {
-  // In accordance with the semantics of forall, its body is executed in
-  // parallel by multiple threads. We should not expect to branch back into
-  // the forall body after the region's execution is complete.
-  if (point.isParent())
-    regions.push_back(RegionSuccessor(&getRegion(), getRegionIterArgs()));
-  else
-    regions.push_back(
-        RegionSuccessor(getOperation(), getOperation()->getResults()));
-}
-
 //===----------------------------------------------------------------------===//
 // InParallelOp
 //===----------------------------------------------------------------------===//
diff --git a/mlir/test/Analysis/DataFlow/test-liveness-analysis.mlir b/mlir/test/Analysis/DataFlow/test-liveness-analysis.mlir
index 171a35fdeafb9..5c277f31dd02e 100644
--- a/mlir/test/Analysis/DataFlow/test-liveness-analysis.mlir
+++ b/mlir/test/Analysis/DataFlow/test-liveness-analysis.mlir
@@ -346,7 +346,7 @@ func.func @affine_loop_no_use_iv() {
 // CHECK-LABEL: test_tag: forall:
 // CHECK-NEXT: operand #0: live
 // CHECK-NEXT: region: #0:
-// CHECK-NEXT:   argument: #0: live
+// CHECK-NEXT:   argument: #0: not live
 
 func.func @forall_no_use_iv_has_side_effect_op(%idx1: index, %idx2: index) {
   scf.parallel (%i) = (%idx1) to (%idx2) step (%idx2) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/174221