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

[Gil Rapaport]

aniragil wrote:

>Ok, now I'm better seeing the whole picture. Let me try to summarize my current understanding ...

Right.

>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.

Not exactly: https://github.com/llvm/llvm-project/pull/92684 aims to map rank-0 memrefs (mlir's representation for scalar variables) to `emitc.variable`s that emitc treats as memory locations by allowing assignment to them, thus retaining memory semantics. The patch indeed doesn't take care of function arguments.
My mem2reg/SROA comment referred to the possibility of eliminating memory semantics where possible, replacing memrefs with SSA values. This can be done as a generic memref-to-memref transformation, but any memref surviving it would reach emitc.

>>a) We could fix the function argument issue in https://github.com/llvm/llvm-project/pull/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 https://github.com/llvm/llvm-project/pull/91475, independent of whether we decide to lower rank 0 memrefs into lvalues. @simon-camp, what do you think?

This sounds like correct lowering when emitting C++. For C, which doesn't support references, lowering could be to an `!emitc.ptr`. Users of such arguments would dereference them and callers would pass the address-of the argument (the same process C programmers do when they need to pass variables by reference).
I may be missing something here: what else could generate lvalue arguments other than rank-0 memrefs? (assuming other memrefs lower into `!emitc.array`)

>I'm not sure about the implications of that change, as function arguments are lvalues by design. (In the sense that you are allowed to take the address of a function argument). So I don't know if we need to distinguish lvalues and lvalue references.

Not sure I follow. As any SSA value in mlir, func.func's arguments and return values are SSA values, not memory locations, e.g.

```mlir
func.func @for_yield(%arg0 : index, %arg1 : index, %arg2 : index) -> (f32, f32) {
  %s0 = arith.constant 0.0 : f32
  %s1 = arith.constant 1.0 : f32
  %result:2 = scf.for %i0 = %arg0 to %arg1 step %arg2 iter_args(%si = %s0, %sj = %s1) -> (f32, f32) {
    %sn = arith.addf %si, %sj : f32
    scf.yield %sn, %sn : f32, f32
  }
  return %result#0, %result#1 : f32, f32
}
```

When lowered to C/C++, these arguments should behave as in-function SSA values in emitc, i.e. be lowered to (const) C arguments whose address cannot be taken. Am I missing something?

>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 https://github.com/llvm/llvm-project/pull/91475 to have a emitc.lvalue_to_rvalue at the same spot.

As you state in (5), emitc's memory ops will usually be the result of lowering memref ops (the standard core dialect for modeling memory), with `memref-to-emitc` supporting some subset of the ops and types of the dialect. Specifically, it may support scalars (e.g. by lowering to `emic.variable`/`!emitc.array`) or expect some previous pass to promote them to rank-1 memrefs.
Whatever `memref-to-emitc` does, if `scf-to-emitc` models memory using emitc ops and types it will either
(a) duplicate work already done in `memref-to-emitc`, or
(b) model memory inconsistently with the rest of the emitc program.

If we're at (a), changes in emitc's memory modeling must now affect both `memref-to-emitc` and `scf-to-emitc`, or we risk falling to (b) unintentionally (https://github.com/llvm/llvm-project/pull/91475 is an example for such a change). Motivation for funneling all emitc memory lowering via the memref dialect is to avoid both (a) and (b) by in the spirit of mlir's gradual lowering philosophy.
Another way to achieve that might be to have common code in emitc to handle variable/array generation, loading, storing etc., to be used by `memref-to-emitc`, `scf-to-emitc` and potentially future/out-of-tree lowering passes which have memory aspects to model.

>Side note: If I'm reading the code correctly this also unifies iter_args and results to one memref each instead of two. We could apply this change to the current lowering as well, right?

I think so, yes. It seems redundant to copy the variables again.

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