Add support for opaque pointers

[rj-jesus]

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

It also includes the following significant changes:

• Adds the option to fallback to typed pointers; in particular, if LLVMLITE_DISABLE_OPAQUE_POINTERS=1, typed pointers are used.
• Decouples pointee data from llvmlite pointers, moving it to a new class _TypedPointerType; objects of this class are created automatically from PointerType's when pointee types are known in a way that's transparent to users.
• Adds explicit types to certain instructions (e.g., loads, gep, etc.) to enable phasing out pointee types on a case-by-case basis.

[gmarkall]

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]

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

[rj-jesus]

@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]

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]

@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]

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]

Let's wait for LLVM15 becoming default

[rj-jesus]

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

[gmarkall]

I've had a look over this and experimented locally with it (not encountering any issues so far in doing that). I've left a couple of comments on the diff, and will have another pass following discussing them.

[rj-jesus]

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]

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

• Change logic _disable_opaque_pointers → opaque_pointers_enabled
• Remove leading underscore from opaque_pointers_enabled
• Move LLVMPY_GlobalGetValueType back to value.cpp
• Reinstate LLVMPY_ABISizeOfElementType and LLVMPY_ABIAlignmentOfElementType with error if used in opaque pointer mode

Please let me know if you have any other comments!

[gmarkall]

Thanks for the updates! I think this is getting there, but I have left a few comments on the diff.

To summarise a few points from our discussions relevant to the continuation of this PR:

1. I will push changes to update the CI setup to test with opaque pointers enabled and disabled.
2. I will push changes to add some documentation (this won't need to be a long section, I think)
3. This PR enables a transitional state where we're supporting a lot of the typed pointer APIs even if we're using opaque pointers in the underlying LLVM binding. This means we can enable opaque pointer usage in Numba without making a large set of changes to it. Once we have confidence in the use of opaque pointers, we can then modify all its API usage to only use the opaque pointer variant of the APIs, allowing the eventual removal of the typed mode in llvmlite. This gives us a simple, gradual, and safe transition path for Numba.

Many thanks for your efforts so far!

[gmarkall]

This results in the _TypedPointerType.__init__() method getting called twice, because:

If new() is invoked during object construction and it returns an instance of cls, then the new instance’s init() method will be invoked like init(self[, ...]), where self is the new instance and the remaining arguments are the same as were passed to the object constructor.

(from https://docs.python.org/3/reference/datamodel.html#object.__new__) - it is called once because the _TypedPointerType() form of the call on this line results in a call to __new__() and __init__(), then after the return of this __new__(), the __init__() method is called again.

I think it needs to be:

Suggested change

	return _TypedPointerType.from_llvm(typeref, ir_ctx)
	return return super().__new__(_TypedPointerType)

instead.

[gmarkall]

This suggestion was a mistake - see https://github.com/numba/llvmlite/pull/1064/files#r1699988799

[rj-jesus]

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

• We now default to opaque pointers disabled
• Moved a few imports to the top-level
• Refactored if logic in a few constructors
• Fixed double-init issue you pointed out

Please let me know if you have any other comments.

[gmarkall]

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

[rj-jesus]

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

[gmarkall]

I think the code changes here are good. I've marked as "waiting on second reviewer" because I've added:

• CI setup that runs with opaque pointers enabled
• Documentation

that could do with a check from someone else.

[rj-jesus]

Hi @gmarkall, Thanks for these commits, I've had a look at the documentation and everything checks out, I've just left a minor comment. Please let me know if you need anything else.

[rj-jesus]

Maybe it's worth noting this is only valid for alloca instructions since they have a clear "allocated type" (https://llvm.org/docs/doxygen/Instructions_8h_source.html#l00115).

[sklam]

Docs and CI looks good

[esc]

thanks for the patch