numba / llvmlite

A lightweight LLVM python binding for writing JIT compilers
https://llvmlite.pydata.org/
BSD 2-Clause "Simplified" License
1.94k stars 322 forks source link

Add support for opaque pointers #1064

Closed rj-jesus closed 2 months ago

rj-jesus commented 5 months ago

This patch supports the transition of llvmlite from typed to opaque pointers, making the latter the default.

It also includes the following significant changes:

gmarkall commented 5 months ago

I note that this works with LLVM 15 but not 14. Given the imminent move to LLVM 15, I think it's not worth fixing for 14.

rj-jesus commented 5 months ago

Hi @gmarkall thanks, sounds good! I'll push a fix for the flake8 tests failing soon as well.

rj-jesus commented 5 months ago

@sklam I believe this affects #1051. Do you have any thoughts on the changes in llvmlite/ir/types.py to PointerType to accommodatefrom_llvm on opaque pointers?

gmarkall commented 5 months ago

Note from triage discussion - for function types, round tripping the type of arguments should still be able to work (correct me if this is wrong, @sklam)

sklam commented 5 months ago

@sklam I believe this affects #1051. Do you have any thoughts on the changes in llvmlite/ir/types.py to PointerType to accommodatefrom_llvm on opaque pointers?

Looks good. The usecase is solely for getting function type back into a ir.Type and all of that is still working as expected.

I happen to notice that on LLVM14 parse_assembly do not like the opaque pointer despite it is enabled. Is that a LLVM bug?

% python -m unittest  llvmlite.tests.test_binding.TestTypeRef.test_typeref_as_ir_for_wrappers_of_cpp_vector_struct
<string>:5:55: warning: ptr type is only supported in -opaque-pointers mode
declare void @"_Z3foo8Vector2DPS_"(<2 x float> %".1", ptr %".2")
                                                      ^
E
======================================================================
ERROR: test_typeref_as_ir_for_wrappers_of_cpp_vector_struct (llvmlite.tests.test_binding.TestTypeRef)
Exercise extracting C++ struct types that are passed as vectors.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/path/to/llvmlite/tests/test_binding.py", line 2091, in test_typeref_as_ir_for_wrappers_of_cpp_vector_struct
    self._check_typeref_as_ir_for_wrappers(
  File "/path/to/llvmlite/tests/test_binding.py", line 2062, in _check_typeref_as_ir_for_wrappers
    new_mod = llvm.parse_assembly(str(wrapper_mod))
  File "/path/to/llvmlite/binding/module.py", line 25, in parse_assembly
    raise RuntimeError("LLVM IR parsing error\n{0}".format(errmsg))
RuntimeError: LLVM IR parsing error
<string>:5:55: error: expected type
declare void @"_Z3foo8Vector2DPS_"(<2 x float> %".1", ptr %".2")
rj-jesus commented 4 months ago

Looks good. The usecase is solely for getting function type back into a ir.Type and all of that is still working as expected.

Perfect, thanks for having a look!

I happen to notice that on LLVM14 parse_assembly do not like the opaque pointer despite it is enabled. Is that a LLVM bug?

I'm not sure, most of CI seems to be failing for the same reason though. When I spoke with @gmarkall we concluded it was probably not worth fixing given llvmlite will be moving to LLVM 15 soon. I can look into it though if you think that'd be useful.

sklam commented 4 months ago

Let's wait for LLVM15 becoming default

rj-jesus commented 4 months ago

That sounds great, do you have an estimate of when that will be?

rj-jesus commented 4 months ago

Hi @gmarkall, thanks for having had a look! I'll work on the proposed changes and update the review soon. In the meantime, I've also left a few comments in reply to yours. If you can, please let me know what you think and I'll include any further changes in the next update.

rj-jesus commented 4 months ago

Hi @gmarkall, I believe I've addressed most of your comments in the last commit, in particular:

Please let me know if you have any other comments!

rj-jesus commented 4 months ago

Hi @gmarkall, thanks very much for your comments, I believe I've addressed them in the latest revision, in particular:

Please let me know if you have any other comments.

gmarkall commented 3 months ago

@rj-jesus Thanks for this, I think it looks good - I'll update the CI configuration and docs.

rj-jesus commented 3 months ago

@gmarkall Thank you very much for the feedback! Please let me know if you need help with anything.

esc commented 2 months ago

thanks for the patch