External func/call lowering in FuncToLLVM with BarePtrCallConv

MLIR
1

Hi,

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.

2

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

3

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.

4

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

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

5

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.

6

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.