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

[Simon Camphausen]

simon-camp wrote:

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

I've looked through the uses of the address-of operator in IREE and it is only used on emitc.variables or on function parameters. So the explicit indirection through variables would of course work, but would make the generated code less readable.

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

I agree that the address-of Op should be constrained to C variables (however these may be modeled in the end). However I would argue that function parameters should also be allowed, as by the C standard.
I will work through your RFC in the next days and add my thoughts.

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