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

[Fabian Mora]

fabianmcg wrote:

> I suggested on another thread and have a custom version of `mlir-opt` that accepts the previous syntax/structure and prints the new structure. Doing it with commented-out FileCheck lines is a bit tricky though.

Yes, that's why I mentioned clang's Rewriter class, it would allow to keep the comments. However, since then I think it's easier  to parse the comments into ops.

> I don't quite see what's broken in terms of control-flow here?

I think the semantics of the implementations are broken here, eg. `forall` having the interface but `in_parallel` not having the matching interface. In the same way, I don't think `SSACFG` region semantics are necessarily broken, but the implementation with `RegionBranch*` likely is.

> However it does not just forward its operands to the parent, since it relies on its body to form the actual values to yield (in a non-trivial manner).

That's another thing I considered, extending the interface and other data structures to also work with ranges of ops. However, there were two things I learned while trying to do that approach:

1. MLIR relies heavily in control-flow operands to be actual op operands, eg. dataflow relies on getting the operand number.
2. Mutation as it's currently prescribed by the interface is impossible for non values. This is one of the things I think it's broken in the interface, IMO mutation should be handled by the interface and we should remove the notion of operands from the interface.

Consequently, the proper fix there it's an even bigger breaking change. I'd argue this needs to happen anyways, but I don't want to propose that until we know how generic control-flow looks in MLIR, as the new system could potentially replace all control-flow and fix things in one go.

> That wouldn't help alone: you would need to materialize the reduction in the same way that XLA/HLO does with its reduce: by taking two values, writing the combiner, and yield the new value (instead of the "magic" `parallel_insert_slice` model existing now)

Yeah, a change in `parallel_insert_slice` is also needed to yield a value. However, I see it as a potentially easier change, with the correct handling happening in the `parallel_result` region of `forall`.


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