[Mlir-commits] [mlir] [mlir][emitc] Lower SCF using memrefs (PR #93371)

[Matthias Gehre]

mgehre-amd wrote:

> > I'm not so clear about the motivation of this change. From what I understand, SCF lowered correctly to emitc before.
> 
> Correct, this change is not functional. It's a refactoring trying to separate the concerns of (1) lowering value-carrying control-flow ops to emitc through memory and (2) lowering memory ops to emitc. In fact, we can do (1) within the scf dialect and have `scf-to-emitc` simply reject `scf.{if, for}` with return values. The `scf-to-emitc` pass was written to generate `emitc.variable` ops directly since we didn't have memref-to-emitc lowering. Now that we do, I think it's better to have all memory modeling in emitc handled in a single pass and have other passes generate memref ops as needed. Concrete motivation for this PR is to simplify @simon-camp's [lvalue PR](https://github.com/llvm/llvm-project/pull/91475), which currently modifies `scf-to-emitc` even though its reg2mem logic remains the same. This PR replaces that code dependency with a pass order dependency, which seems cleaner.
> 
> > The generated C++ code looks different in that we now use arrays with one element instead of scalars to model loop carried values. Why is that better?
> 
> This PR promotes scalars to 1-element arrays, which indeed leads to [less natural C/C++ code](https://github.com/llvm/llvm-project/pull/92684#issuecomment-2128949830). Getting back to natural C/C++ code can be achieved by either: (a) Having `scf-to-emitc` generate rank-0 memrefs and `memref-to-emitc` lower them to `emitc.variable` ops, as suggested [here](https://github.com/llvm/llvm-project/pull/92684). (b) Having `memref-to-emitc` lower rank-1 memrefs to `emitc.variable` ops where possible. This seems less desired as such generic memory transformations (SROA) belong in the memref dialect (but that leads back to option (a)).

Ok, now I'm better seeing the whole picture. Let me try to summarize my current understanding:
1) the current scf-to-emitc conversion needs to use memory semantics to transport values across scopes.
2) it uses an ad-hoc lowering to reference (aka lvalue) semantics. Through that, it obtain natural C/C++ code.
3) you would like to unify the handling of memory semantics into a single place
4) single place here means single dialect
5) the only dialect before emitc that has memory semantics seems to be `memref`. There is no other dialect before emitc that models pointer or reference semantics.
6) that's why this PR turns the memory semantics required for scf into `memref`
7) and later the `memref-to-emitc` turns that into arrays.

As alternative to 7), you have shown in https://github.com/llvm/llvm-project/pull/92684 how we could do mem2reg as part of the memref-to-emitc lowering to turn memrefs into scalars in emitc. That gives natural C/C++ code but fails to generally convert all rank 0 memrefs - because turning rank 0 memref function arguments into scalars looses their reference semantics.

Two things occur to me:
a) We could fix the function argument issue in #92684 by recognizing that the `memref<f32>` should not be turned into `f32` (like mem2reg) but instead into `lvalue<f32>` (preserving reference semantics). This would essentially keep all lowering code there currently intact (because it turns `memref.alloca memref<f32>` into `emitc.variable  f32`, which will have `lvalue<f32>` type). The key difference is that function arguments of type `memref<f32>` need to also become `lvalue<f32>`.

And that the emitter needs to understand that `lvalue<f32> %a`  as function argument means emitting it as `float& a` (i.e. reference). I think this is the correct thing to do for lvalue function arguments in #91475, independent of whether we decide to lower rank 0 memrefs into lvalues. @simon-camp, what do you think?

b) Looking more closely at this PR, I don't see the lowering code benefit from unifying handling of memory semantics into the memref dialect. The code complexity is essentially the same, trading op names like `emitc.variable` for `memref.alloca`.
This PR brings a new `memref.load` op into the mix, but I expect #91475 to have a emitc.lvalue_to_rvalue at the same spot.





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