[Mlir-commits] [mlir] [mlir][emitc] Refactor emitc.apply op (PR #72569)

[Gil Rapaport]

aniragil wrote:

> This breaks the use of EmitC in IREE without providing a suitable alternative. **Therefore, this cannot be merged as-is!** In IREE an address-of operator is applied to the arguments passed to a function (see [ConvertVMToEmitC.cpp#L2155](https://github.com/openxla/iree/blob/7963ca781b6f3782ad21d20ac24b7adca150e36f/compiler/src/iree/compiler/Dialect/VM/Conversion/VMToEmitC/ConvertVMToEmitC.cpp#L2155)) and the &-operator is also used in conjunction with IREE's `!vm.ref` type.
> 

Sorry, should have described it in the commit message: The alternative to taking the address of a value is to define a variable using emitc::variable, use an emitc::assign to assign the value to that variable and then take that variable's address using emitc::address_of. In the mentioned iree code this seems to require a rather minor change in [the addressOf function](https://github.com/openxla/iree/blob/7963ca781b6f3782ad21d20ac24b7adca150e36f/compiler/src/iree/compiler/Dialect/VM/Conversion/VMToEmitC/EmitCBuilders.cpp#L132), but I'm not familiar with iree's code so I may be completely off here.

> Furthermore, I don't see a strong argument so far to split up the `emitc.apply` op. In the initial proposal, presented at the ODM at 2021-04-22 (EmitC: Generating C/C++ from MLIR [slides](https://drive.google.com/file/d/1p2AM7B1beb5oVf_tlTyinnVQoWuodoxl/view?usp=sharing)), we had an `emitc.getaddressof` op, but it was later on agreed to rather go with the `emitc.apply` op.
> 

The current state is that emitc:apply as a single op is modelling two distinct opcodes and selecting between them using an attribute, which complicates any code handling this op. I don't know how often this approach is used in MLIR, but I remember  @joker-eph explaining its downsides during the [2021-05-27 MLIR Open Meeting](https://youtu.be/_qXCzP5YAZY?t=3237). However the emitc.apply case might be different. Could you elaborate on why the multi-op approach was eventually chosen and why the same approach wasn't applied to emitc's binary ops?

> From my point of view it is much more important to answer the question, whether it should be ensured in the conversion that the &-operator is not used incorrectly or if an incorrect usage could be prevented with a verification pass instead of coupling the &-operator to only EmitC variables.

This patch suggests a possible way to address the specific concern of breaking IREE by limiting emitc.apply **assuming** the suggestion to separate SSA values from variables is accepted (I'll mark it as draft to avoid confusion), which is a broader discussion yet to be done on discourse. I'll open a topic for it, but in a nutshell - I think limiting address-taking to variables goes beyond verification issues. I agree with @jpienaar that "... separating SSA IDs from variables, variables as symbols and expressions may all be related", as identifying SSA values with locations whose addresses can be taken seems to induce complicated (if not inconsistent) semantics, e.g. does the result of emitc.literal have a location? If the result of emitc.add does, what happens to it when it's part of an expression?


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