search

lcompilers / lpython

Python compiler
https://lpython.org/
Other
1.5k stars 158 forks source link

`pass_wrap_global_stmts` always creates function of `abiType::BindC` #2700

Closed Vipul-Cariappa closed 4 months ago

Vipul-Cariappa commented 4 months ago

Reference https://github.com/lcompilers/lpython/pull/2698#discussion_r1597650250

Vipul-Cariappa commented 4 months ago

cc @Shaikh-Ubaid

Shaikh-Ubaid commented 4 months ago

This is fine. Can you submit this change to LFortran? So that we know it works with LFortran.

Shaikh-Ubaid commented 4 months ago

When you make a PR there, just post a link to it here.

Vipul-Cariappa commented 4 months ago

I initially set the ABI to Source. LFortran does not like Source and tests fail there. So, I instead changed it to BindC. All tests should pass in both LPython and LFortran according to me. It does not matter what ABI the pass_wrap_global_stmts uses, but I want it to be of the same ABI irrespective of other factors. Because at the LLVM backend, it changes the function name according to the ABI used as explained earlier.

Vipul-Cariappa commented 4 months ago

Now, WASM related tests are failing. I will need to look into it.

Shaikh-Ubaid commented 4 months ago

I initially set the ABI to Source. LFortran does not like Source and tests fail there. So, I instead changed it to BindC

I think changing to Source is fine. But changing to BindC might not be. When we say BindC, it means that the function is implemented in C. For this function, we only know the prototype of the function and the function body is empty.

I think in our case, we have the function body so marking it as BindC only to avoid name mangle does not look right. @Vipul-Cariappa Since the name issue for you arises during interactive compilation, can you trying not mangling name when compiler_options.interactive = true in the llvm backend?

Vipul-Cariappa commented 4 months ago

That might be possible. Changing ASRToLLVMVisitor.instantiate_function to avoid name mangling while compiler_options.interactive = true is set.

Vipul-Cariappa commented 4 months ago

I guess we can close this as we have merged #2701.