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

[Oleksandr Alex Zinenko]

ftynse 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

which is exactly 

> 1. Require region branch terminators to implement the RegionBranchTerminatorOpInterface. This can be checked in the verifier by checking all region branch points.

Currently we don't seem to require it. I have written such transformations and in a conservative way: `dyn_cast` the terminator to the interface and assume the worst case (any terminator operand may flow to any block argument / parent result) if not implemented. I don't think not implementing RegionBranchTerminatorOpInterface makes us miss any control flow edge as these are not specified by this interface. They are specified by the `RegionBranchOpInterface::getSuccessorRegion`, the terminator interface only saying which operands are forwarded along those edges. 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.

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