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

[Simon Camphausen]

simon-camp wrote:

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

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.

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

I would expect that the changes to lvalue variables look about equivalent to the changes here (after adopting the semantics to what we agreed on in #91475).

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?

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