[Mlir-commits] [mlir] [mlir][scf] Add `ReturnLike` to `scf::InParallelOp` (PR #148237)

[Fabian Mora]

fabianmcg wrote:

> Could you elaborate more on the reasoning here? 

Sure.

Right now, `scf.forall` control-flow semantics are broken. `scf.forall` declares to be `RegionBranchOpInterface` but `scf.in_parallel` is not a `RegionBranchTerminatorOpInterface`, and this breaks things like dataflow. Furthermore, I'd argue this breaks the implicit contract of region control-flow, where each `RegionBranchOpInterface` should be matched by a `RegionBranchTerminatorOpInterface`.

There were 2 bad options to start fixing the control-flow semantics in this case:
1. Remove `RegionBranchOpInterface` from `scf.forall`. This sounds like a breaking change that might break semantics for everyone. Further, in the future we would re-introduce it once the whole semantics issue is fixed.
2. Start fixing `scf.in_parallel`, and try to break as few things as possible during the process to make it comply with `RegionBranchTerminatorOpInterface`.

I opted for 2. This fixes `scf.in_parallel` semantics for the buffer case (as the buffer case has no arguments) but not the tensor case, because as you mentioned this op doesn't forward its operands to the parent op.

The proper fix to this whole situation, (IMO) is to add a second region to `scf.forall` and do the in parallel computations there for tensors, then `RegionBranchTerminatorOpInterface` can always apply. But that's a large breaking change specially for testing, so I'm trying to figure out ways to keep the changes small, and this was my choice among a set of bad choices (I'm open to hear alternatives) as a first step, as it fixes the easy half of the problem.

Now, outside of this patch I would argue that control-flow semantics (outside blocks) in MLIR are somewhat broken, as this is not the first time I encounter a situation like this. But that's a larger conversation and out of scope for this patch.

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