External func/call lowering in FuncToLLVM with BarePtrCallConv


Here, the conversion only adds wrapper when it’s using BarePtrCallConv.
It doesn’t need to modify the arguments but I suppose it still need to add “mlir_ciface” prefix?

I tried to add wrappers and it seems working but not sure if we need the wrapper or just to rename the func/call with the prefix.

Gentle ping to the previous commitor/reviewer @MaheshRavishankar , @ftynse
Could you please review this PR?

I don’t think this is desired. The name mangling is intended to differentiate functions that use “memref C-compatible API”. The wrappers are only necessary to convert the expanded-argument function signatures to signatures with memref as a struct. There doesn’t seem to be any value in adding wrappers that merely forward to the actual function.

Right, I’ve reached at your original commit introducing the c wrapper and clearly understand its main purpose.
However, there’s an issue in execution engine where invoke always expects the presence of c wrapper. c++ , python

Just found in llvm-project/mlir/docs/TargetLLVMIR.md at main · llvm/llvm-project (github.com)

The conversion to the LLVM dialect provides an option to generate wrapper functions that take memref descriptors as pointers-to-struct compatible with data types produced by Clang when compiling C sources. The generation of such wrapper functions can additionally be controlled at a function granularity by setting the `llvm.emit_c_interface` unit attribute.

So, technically, it looks that the wrapper should be always generated with llvm.emit_c_interface attribute.

Remaining question could be, is it correct to set llvm.emit_c_interface attribute to the functions only having bare ptr argument? (i.e., assumed behaviour in invoke())

I don’t have any strong opinion on the design pov, just want to fix the issue that execution engine can’t invoke the host function with -use-bare-pointers-for-host flag.

I have always considered bare pointer calling convention a big hack, mostly utilized by flows that are unwilling or for some reason incapable of handling memref properly, because memref is not a pointer. Execution engine was likely written in the same mindset, so the utility functions like “invoke” are only designed for the default, non-hacky case. It is still possible to use invokePacked directly, or even to get a function pointer from lookup / lookupPacked and call it, in C++. Not sure about Python, we would probably need to bind some more API there (cc @mehdi_amini who worked on that part).

We could consider having some way of supporting the bare pointer convention better in the execution engine, though I am hopeful that it will just go away eventually.

So, technically, it looks that the wrapper should be always generated with llvm.emit_c_interface attribute.

The attribute is an instruction to MLIR to generate the wrapper for the function that has the attribute. The wrapper itself wouldn’t have the attribute AFAIU.

Yep, that’s my bad the sentence is misleading. I meant wrapper needs to be generated whenever the original func has the attribute

Thanks for the explanation, that clarifies more missing pieces to me.
Also, another recent discussion [RFC] `address` dialect might give us a new option to consider. I’m going to retrieve my patch.