[mlir][scf] fix getSuccessorRegions func in scf.forall

[cxy-1993]

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.

[llvmbot]

@llvm/pr-subscribers-mlir-scf

@llvm/pr-subscribers-mlir

Author: donald chen (cxy-1993)

Changes

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.

---

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

1 Files Affected:

• (modified) mlir/lib/Dialect/SCF/IR/SCF.cpp (+7-5)

diff --git a/mlir/lib/Dialect/SCF/IR/SCF.cpp b/mlir/lib/Dialect/SCF/IR/SCF.cpp
index 5a3bd984530db..2e0a7b85af756 100644
--- a/mlir/lib/Dialect/SCF/IR/SCF.cpp
+++ b/mlir/lib/Dialect/SCF/IR/SCF.cpp
@@ -1927,11 +1927,13 @@ void ForallOp::getCanonicalizationPatterns(RewritePatternSet &results,
 /// not a constant.
 void ForallOp::getSuccessorRegions(RegionBranchPoint point,
                                    SmallVectorImpl<RegionSuccessor> &regions) {
-  // Both the operation itself and the region may be branching into the body or
-  // back into the operation itself. It is possible for loop not to enter the
-  // body.
-  regions.push_back(RegionSuccessor(&getRegion()));
-  regions.push_back(RegionSuccessor());
+  // 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()));
+  else
+    regions.push_back(RegionSuccessor());
 }
 
 //===----------------------------------------------------------------------===//

[ftynse]

Is this stemming from some real use case or bug?

The control flow interfaces are not modeling parallelism currently, and this is a bit tricky. In particular, control flow interfaces are used by dataflow analyses to understand whether a block may be executed before or after another block, including a different "instance" of the same block. This may be the case here. I'd like to better think through this before we decide one way or another.

[cxy-1993]

Is this stemming from some real use case or bug?

The control flow interfaces are not modeling parallelism currently, and this is a bit tricky. In particular, control flow interfaces are used by dataflow analyses to understand whether a block may be executed before or after another block, including a different "instance" of the same block. This may be the case here. I'd like to better think through this before we decide one way or another.

We're using dataflow analysis on a very large program, and the compilation time is excessively long because scf.forall assumes the body may execute after body.

The current implementation doesn't cause functional issues, but it's conservative, increasing compilation time and potentially making the analysis overly cautious. The forall semantic is meant to indicate parallel execution across multiple threads, so for a single thread, it's more reasonable for the forall body to exit after execution (similar to an outlined gpu program).

[joker-eph]

There can't be any "data" flowing from one instance of a parallel block to another.
I don't think they are ordered and having the dynamic iteration N+1 being the successor or the dynamic iteration N does not really makes sense to me?

I don't quite get why this was modeled this way in the first place though, I may be missing something here.

[matthias-springer]

For reference, I found these old discussions / PRs:

• https://discourse.llvm.org/t/why-scf-forall-op-doesnt-have-regionbranchop-interface/70789
• https://reviews.llvm.org/D151287
• https://reviews.llvm.org/D155822

The last PR may have accidentally changed the behavior.

[cxy-1993]

Ping, looks like we've reached a consensus?