[Mlir-commits] [mlir] [mlir][EmitC] Model lvalues as a type in EmitC (PR #91475)

[Gil Rapaport]

aniragil wrote:

> There is one thing we should agree on before this is merged. As the patch introduces breaking changes, @simon-camp and I discussed offline that this requires a PSA. An non-breaking intermediate solution would be to _relax_ the restriction on `emitc.variable` and to allow `EmitCType` as a result. This would mean to replace
> 
> ```mlir
> let results = (outs Res<AnyTypeOf<[EmitC_ArrayType, EmitC_LValueType]>, "",
> ```
> 
> with
> 
> ```mlir
> let results = (outs Res<AnyTypeOf<[EmitCType, EmitC_ArrayType, EmitC_LValueType]>, "",
> ```
> 
> while we ignore the changes regarding the memory effects. Furthermore, the modifications to `emitc.apply` would need to be rolled-back and instead an `emitc.dereference` (https://en.cppreference.com/w/c/language/operator_member_access) / `emitc.indirection` (https://en.cppreference.com/w/cpp/language/operator_member_access) and `emitc.address_of` should be introduced (which we probably want to do in a follow up anyway). With this, the `emitc.variable` as well as `emitc.apply` op could still be used as before but users can adapt to the new behavior before the result type of the `emitc.variable` op gets restricted and the `emitc.apply` op is removed.

Sounds good (what about the changes to `emitc.subscript`?)
We can take it further by keeping `emitc.variable` as-is along with `emitc.apply` and introducing a new `emitc.define` op along with `emitc.address_of` and `emitc.dereference` to work on lvalues. Then, some time after the PSA, erase both `emitc.variable` and `emitc.apply` from the dialect, which might be cleaner/safer than altering their semantics.
(BTW, we could perhaps extend the existing `emitc.subscript` op to do dereferencing. It already accepts `!emitc.ptr`, but still requires an index. If we allow zero indices for pointers it could translate to `*p`).

> However, adopting to the behavior shouldn't be too hard, thus we tend to keep the effort low and just go with a PSA and before merging this in **x?** weeks. WDYT?

I'm also fine with just giving the community a heads up and pushing the patch as is in a **x** weeks. As this patch is quite large, **x** should probably be relatively small to prevent it from bit rotting and to allow further development of emitc (we can perhaps wait a longer **x** before erasing these ops if we make `emitc.variable` obsolete, while still pushing it immediately).

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