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

[Matthias Springer]

matthias-springer wrote:

> > Long story short -- we cannot simplify verify that every terminator implements the RegionBranchTerminatorOpInterface.
> 
> I never said that all terminators should implement it, only
> 
> > terminators in operations implementing RegionBranchOpInterface

I think we're still talking about different things. There are legitimate cases where even a terminator inside of a `RegionBranchOpInterface` cannot implement the `RegionBranchTerminatorOpInterface` (case 3 below) -- in the presence of unstructured control flow, a region may have multiple blocks. There are 3 cases:

1. A block terminator is conceptually a region branch terminator and implements the `RegionBranchTerminatorOpInterface`.
2. A block terminator is conceptually a region branch terminator but does not implement the `RegionBranchTerminatorOpInterface`. (E.g., because we say that it's optional to implement that interface.)
3. A block terminator is conceptually *not* a region branch terminator and does not implement the `RegionBranchTerminatorOpInterface`. (E.g., because it a unstructured control flow branching op.)

I'd like to see if we can rule out case 2 to simplify the design.

Here's a problem with case 2 that I hinted to above: While the `RegionBranchOpInterface` itself captures the entire region control flow, the current API does not allow you to query the region successor in case 2. `RegionBranchOpInterface::getSuccessorRegions` takes a `RegionBranchPoint` parameter, but to construct a `RegionBranchPoint` you need a `RegionBranchTerminatorOpInterface` op. You cannot construct a `RegionBranchPoint` from just an `Operation *`.

If we relax this API, I agree that we could probably safely allow case 2 and even support conservative value propagation as you described above. But that means that we also have to check/update all existing analyses/transformations that query control flow from the `RegionBranchOpInterface`. They all implicitly assume that case 2 does not exist (due to the API limitation). Given that all analyses/transformations already operate under that assumption, is there any harm in forbidding case 2? I'm going to update this PR accordingly so we can see what that design looks like in detail.

> I see a getSuccessorRegions function on the terminator interface, but its default implementation defers to the region interface on the parent and we don't have non-default implementation upstream. IMO it should be turned into a non-method and just an extra declaration on the interface.

I just looked into this. It's a bit different from `RegionBranchOpInterface::getSuccessorRegions`: it takes an additional `ArrayRef<Attribute> operands` parameter. This `RegionBranchTerminatorOpInterface::getSuccessorRegions` interface method is similar to `RegionBranchOpInterface::getEntrySuccessorRegions`, and while these interface methods may not be used much, at least the design is consistent.


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