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

[Matthias Springer]

matthias-springer wrote:

> do we require somewhere that terminators in operations implementing `RegionBranchOpInterface` must implement `RegionBranchTerminatorOpInterface`

Not all terminators are necessarily `RegionBranchTerminatorOpInterface`. You could also have a `BranchOpInterface` terminator. (If there's just a single block in the region and the terminator is a branching terminator for unstructured control flow, we have an infinite loop. You could potentially also have an op which implements both interfaces.) Long story short -- we cannot simplify verify that every terminator implements the `RegionBranchTerminatorOpInterface`.

If you have a transformation that analyzes `RegionBranchTerminatorOpInterface` and `RegionBranchOpInterface`, we must be able to query both interfaces. Otherwise, the transformation may "miss" some control flow edges. That's the case in the patterns that I'm adding in #174094. (For the moment, this is not a problem because those patterns are enabled selectively via a whitelist. But eventually, I'd like to enable them globally.)

I see two options:
1. Require region branch terminators to implement the `RegionBranchTerminatorOpInterface`. This can be checked in the verifier by checking all region branch points. (Similar to how we check that `scf.for` has an `scf.yield` terminator.)
2. Document that implementing the `RegionBranchTerminatorOpInterface` is optional. Add a helper function `RegionBranchOpInterface::modelsCompleteDataflow` (which performs the same check as in Option 1), so transformations can opt out.

> it flows from before the op to the region and then to after the op, no values are being forwarded as-is anywhere (hence no need for `RegionBranchTerminatorOpInterface`).

For an op that does forward any values, we could still require that the `RegionBranchTerminatorOpInterface` interface is implemented, and `getMutableSuccessorOperands` would return an an empty list.

> the parallel loop has always been a bit weird, because the op does more than "control flow" with a special logic to "merge" results

Yes, that the problem that I wanted to address first. It's unclear to me why the op even implements the interface. If the answer is "we need it" for some reason, then Option 1 above is not feasible with the current design.



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