Skip to content

Drop binding layer typed pointer support. #1160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
esc merged 4 commits into from
Mar 17, 2025
Merged

Drop binding layer typed pointer support. #1160

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
603bace
Drop typed pointer support and bump LLVM release to 16.
rj-jesus Feb 19, 2025
95e8fba
No longer bump LLVM release
rj-jesus Mar 11, 2025
b7f160a
Merge branch 'main' into rjj/move-to-llvm-16
rj-jesus Mar 13, 2025
e49ca3f
Fix intrinsic_name
rj-jesus Mar 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions ffi/core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,19 @@ LLVMPY_CreateByteString(const char *buf, size_t len) {
API_EXPORT(void)
LLVMPY_DisposeString(const char *msg) { free(const_cast<char *>(msg)); }

// FIXME: Remove `enableOpaquePointers' once typed pointers are removed.
API_EXPORT(LLVMContextRef)
LLVMPY_GetGlobalContext(bool enableOpaquePointers) {
LLVMPY_GetGlobalContext() {
auto context = LLVMGetGlobalContext();
LLVMContextSetOpaquePointers(context, enableOpaquePointers);
// FIXME: Remove with LLVM >= 17.
LLVMContextSetOpaquePointers(context, true);
return context;
}

// FIXME: Remove `enableOpaquePointers' once typed pointers are removed.
API_EXPORT(LLVMContextRef)
LLVMPY_ContextCreate(bool enableOpaquePointers) {
LLVMPY_ContextCreate() {
LLVMContextRef context = LLVMContextCreate();
LLVMContextSetOpaquePointers(context, enableOpaquePointers);
// FIXME: Remove with LLVM >= 17.
LLVMContextSetOpaquePointers(context, true);
return context;
}

Expand Down
6 changes: 2 additions & 4 deletions ffi/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,11 @@ LLVMPY_CreateByteString(const char *buf, size_t len);
API_EXPORT(void)
LLVMPY_DisposeString(const char *msg);

// FIXME: Remove `enableOpaquePointers' once typed pointers are removed.
API_EXPORT(LLVMContextRef)
LLVMPY_GetGlobalContext(bool enableOpaquePointers);
LLVMPY_GetGlobalContext();

// FIXME: Remove `enableOpaquePointers' once typed pointers are removed.
API_EXPORT(LLVMContextRef)
LLVMPY_ContextCreate(bool enableOpaquePointers);
LLVMPY_ContextCreate();

} /* end extern "C" */

Expand Down
20 changes: 0 additions & 20 deletions ffi/targets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,26 +121,6 @@ LLVMPY_ABIAlignmentOfType(LLVMTargetDataRef TD, LLVMTypeRef Ty) {
return (long long)LLVMABIAlignmentOfType(TD, Ty);
}

// FIXME: Remove me once typed pointers are no longer supported.
API_EXPORT(long long)
LLVMPY_ABISizeOfElementType(LLVMTargetDataRef TD, LLVMTypeRef Ty) {
llvm::Type *tp = llvm::unwrap(Ty);
if (!tp->isPointerTy())
return -1;
tp = tp->getPointerElementType();
return (long long)LLVMABISizeOfType(TD, llvm::wrap(tp));
}

// FIXME: Remove me once typed pointers are no longer supported.
API_EXPORT(long long)
LLVMPY_ABIAlignmentOfElementType(LLVMTargetDataRef TD, LLVMTypeRef Ty) {
llvm::Type *tp = llvm::unwrap(Ty);
if (!tp->isPointerTy())
return -1;
tp = tp->getPointerElementType();
return (long long)LLVMABIAlignmentOfType(TD, llvm::wrap(tp));
}

API_EXPORT(LLVMTargetRef)
LLVMPY_GetTargetFromTriple(const char *Triple, const char **ErrOut) {
char *ErrorMessage;
Expand Down
17 changes: 1 addition & 16 deletions ffi/type.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,22 +144,7 @@ API_EXPORT(uint64_t)
LLVMPY_GetTypeBitWidth(LLVMTypeRef type) {
llvm::Type *unwrapped = llvm::unwrap(type);
auto size = unwrapped->getPrimitiveSizeInBits();
return size.getFixedSize();
}

// FIXME: Remove me once typed pointers support is removed.
API_EXPORT(LLVMTypeRef)
LLVMPY_GetElementType(LLVMTypeRef type) {
llvm::Type *unwrapped = llvm::unwrap(type);
llvm::PointerType *ty = llvm::dyn_cast<llvm::PointerType>(unwrapped);
if (ty != nullptr) {
#if LLVM_VERSION_MAJOR < 14
return llvm::wrap(ty->getElementType());
#else
return llvm::wrap(ty->getPointerElementType());
#endif
}
return nullptr;
return size.getFixedValue();
Copy link
Member

@gmarkall gmarkall Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note on this change: getFixedSize() retured getFixedValue() anyway, and getFixedSize() is deprecated / removed in later LLVM versions.

Copy link
Contributor Author

@rj-jesus rj-jesus Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I believe these are just API renames.

}

API_EXPORT(LLVMTypeRef)
Expand Down
3 changes: 0 additions & 3 deletions llvmlite/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@
__version__ = get_versions()['version']
del get_versions

# FIXME: Remove me once typed pointers are no longer supported.
# Opaque pointers unconditionally required for later LLVM versions
opaque_pointers_enabled = True
# We default to IR layer typed pointers being enabled, since they're needed in
# the most common usage scenarios with later LLVMs.
def _ir_layer_typed_pointers_enabled():
Expand Down
12 changes: 2 additions & 10 deletions llvmlite/binding/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,14 @@
from llvmlite.binding import ffi

# FIXME: Remove me once typed pointers are no longer supported.
from llvmlite import opaque_pointers_enabled
from ctypes import c_bool


def create_context():
return ContextRef(
ffi.lib.LLVMPY_ContextCreate(opaque_pointers_enabled))
ffi.lib.LLVMPY_ContextCreate())


def get_global_context():
return GlobalContextRef(
ffi.lib.LLVMPY_GetGlobalContext(opaque_pointers_enabled))
ffi.lib.LLVMPY_GetGlobalContext())


class ContextRef(ffi.ObjectRef):
Expand All @@ -28,12 +24,8 @@ def _dispose(self):
pass


# FIXME: Remove argtypes once typed pointers are no longer supported.
ffi.lib.LLVMPY_GetGlobalContext.argtypes = [c_bool]
ffi.lib.LLVMPY_GetGlobalContext.restype = ffi.LLVMContextRef

# FIXME: Remove argtypes once typed pointers are no longer supported.
ffi.lib.LLVMPY_ContextCreate.argtypes = [c_bool]
ffi.lib.LLVMPY_ContextCreate.restype = ffi.LLVMContextRef

ffi.lib.LLVMPY_ContextDispose.argtypes = [ffi.LLVMContextRef]
38 changes: 0 additions & 38 deletions llvmlite/binding/targets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@
from llvmlite.binding.common import _decode_string, _encode_string
from collections import namedtuple

# FIXME: Remove `opaque_pointers_enabled` once typed pointers are no longer
# supported.
from llvmlite import opaque_pointers_enabled

Triple = namedtuple('Triple', ['Arch', 'SubArch', 'Vendor',
'OS', 'Env', 'ObjectFormat'])

Expand Down Expand Up @@ -202,30 +198,6 @@ def get_abi_alignment(self, ty):
"""
return ffi.lib.LLVMPY_ABIAlignmentOfType(self, ty)

def get_pointee_abi_size(self, ty):
"""
Get ABI size of pointee type of LLVM pointer type *ty*.
"""
if opaque_pointers_enabled:
raise RuntimeError("Cannot get pointee type in opaque pointer "
"mode.")
size = ffi.lib.LLVMPY_ABISizeOfElementType(self, ty)
if size == -1:
raise RuntimeError("Not a pointer type: %s" % (ty,))
return size

def get_pointee_abi_alignment(self, ty):
"""
Get minimum ABI alignment of pointee type of LLVM pointer type *ty*.
"""
if opaque_pointers_enabled:
raise RuntimeError("Cannot get pointee type in opaque pointer "
"mode.")
size = ffi.lib.LLVMPY_ABIAlignmentOfElementType(self, ty)
if size == -1:
raise RuntimeError("Not a pointer type: %s" % (ty,))
return size


RELOC = frozenset(['default', 'static', 'pic', 'dynamicnopic'])
CODEMODEL = frozenset(['default', 'jitdefault', 'small', 'kernel', 'medium',
Expand Down Expand Up @@ -440,16 +412,6 @@ def has_svml():
ffi.LLVMTypeRef]
ffi.lib.LLVMPY_ABIAlignmentOfType.restype = c_longlong

# FIXME: Remove me once typed pointers are no longer supported.
ffi.lib.LLVMPY_ABISizeOfElementType.argtypes = [ffi.LLVMTargetDataRef,
ffi.LLVMTypeRef]
ffi.lib.LLVMPY_ABISizeOfElementType.restype = c_longlong

# FIXME: Remove me once typed pointers are no longer supported.
ffi.lib.LLVMPY_ABIAlignmentOfElementType.argtypes = [ffi.LLVMTargetDataRef,
ffi.LLVMTypeRef]
ffi.lib.LLVMPY_ABIAlignmentOfElementType.restype = c_longlong

ffi.lib.LLVMPY_GetTargetFromTriple.argtypes = [c_char_p, POINTER(c_char_p)]
ffi.lib.LLVMPY_GetTargetFromTriple.restype = ffi.LLVMTargetRef

Expand Down
9 changes: 1 addition & 8 deletions llvmlite/binding/typeref.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,6 @@
from llvmlite import ir
from llvmlite.binding import ffi

# FIXME: Remove `opaque_pointers_enabled' when TP's are removed.
from llvmlite import opaque_pointers_enabled


class TypeKind(enum.IntEnum):
# The LLVMTypeKind enum from llvm-c/Core.h
Expand Down Expand Up @@ -108,7 +105,7 @@ def elements(self):
"""
Returns iterator over enclosing types
"""
if self.is_pointer and opaque_pointers_enabled:
if self.is_pointer:
raise ValueError("Type {} doesn't contain elements.".format(self))
return _TypeListIterator(ffi.lib.LLVMPY_ElementIter(self))

Expand Down Expand Up @@ -226,10 +223,6 @@ def _next(self):
ffi.lib.LLVMPY_PrintType.argtypes = [ffi.LLVMTypeRef]
ffi.lib.LLVMPY_PrintType.restype = c_void_p

# FIXME: Remove me once typed pointers support is removed.
ffi.lib.LLVMPY_GetElementType.argtypes = [ffi.LLVMTypeRef]
ffi.lib.LLVMPY_GetElementType.restype = ffi.LLVMTypeRef

ffi.lib.LLVMPY_TypeIsPointer.argtypes = [ffi.LLVMTypeRef]
ffi.lib.LLVMPY_TypeIsPointer.restype = c_bool

Expand Down
4 changes: 3 additions & 1 deletion llvmlite/ir/types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ def gep(self, i):

@property
def intrinsic_name(self):
return 'p%d%s' % (self.addrspace, self.pointee.intrinsic_name)
if ir_layer_typed_pointers_enabled:
Copy link
Member

@gmarkall gmarkall Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a fix for an error that was previously un-noticed / un-addressed - is that correct?

Copy link
Contributor Author

@rj-jesus rj-jesus Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it fixes an inconsistent behaviour in how pointer intrinsic names are printed.

Previously, even if we enabled IR layer typed pointers, the pointer's intrinsic name would still reflect its "true" underlying type. This is probably not the desired behaviour, so I've changed this to follow the same logic we have for the "_to_string" method.

I should have done this when we added IR layer typed pointers, but I think this got lost with the opaque pointer fixes (which in practice came after IR layer typed pointers, but I had initially thought them to come before).

return 'p%d%s' % (self.addrspace, self.pointee.intrinsic_name)
return super(_TypedPointerType, self).intrinsic_name


class VoidType(Type):
Expand Down
23 changes: 3 additions & 20 deletions llvmlite/tests/test_binding.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,6 @@
from llvmlite.binding import ffi
from llvmlite.tests import TestCase

# FIXME: Remove me once typed pointers are no longer supported.
from llvmlite import opaque_pointers_enabled

# arvm7l needs extra ABI symbols to link successfully
if platform.machine() == 'armv7l':
llvm.load_library_permanently('libgcc_s.so.1')
Expand Down Expand Up @@ -1655,12 +1652,7 @@ def test_type_printing_struct(self):
mod = self.module()
st = mod.get_global_variable("glob_struct")
self.assertTrue(st.type.is_pointer)
# FIXME: Remove `else' once TP are no longer supported.
if opaque_pointers_enabled:
self.assertIsNotNone(re.match(r'ptr', str(st.type)))
else:
self.assertIsNotNone(re.match(r'%struct\.glob_type(\.[\d]+)?\*',
str(st.type)))
self.assertIsNotNone(re.match(r'ptr', str(st.type)))
self.assertIsNotNone(re.match(
r"%struct\.glob_type(\.[\d]+)? = type { i64, \[2 x i64\] }",
str(st.global_value_type)))
Expand Down Expand Up @@ -1849,11 +1841,7 @@ def test_constant_as_string(self):
inst = list(list(func.blocks)[0].instructions)[0]
arg = list(inst.operands)[0]
self.assertTrue(arg.is_constant)
# FIXME: Remove `else' once TP are no longer supported.
if opaque_pointers_enabled:
self.assertEqual(arg.get_constant_value(), 'ptr null')
else:
self.assertEqual(arg.get_constant_value(), 'i64* null')
self.assertEqual(arg.get_constant_value(), 'ptr null')

def test_incoming_phi_blocks(self):
mod = self.module(asm_phi_blocks)
Expand Down Expand Up @@ -1958,12 +1946,7 @@ def test_vararg_function(self):
self.assertTrue(func.type.is_pointer)
with self.assertRaises(ValueError) as raises:
func.type.is_function_vararg
# FIXME: Remove `else' once TP are no longer supported.
if opaque_pointers_enabled:
self.assertIn("Type ptr is not a function", str(raises.exception))
else:
self.assertIn("Type i32 (i32, i32)* is not a function",
str(raises.exception))
self.assertIn("Type ptr is not a function", str(raises.exception))

def test_function_typeref_as_ir(self):
mod = self.module()
Expand Down
13 changes: 8 additions & 5 deletions llvmlite/tests/test_ir.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,9 +281,9 @@ def test_declare_intrinsics(self):
declare double @"llvm.powi.f64"(double %".1", i32 %".2")""")
if not ir_layer_typed_pointers_enabled:
self.check_descr(self.descr(memset).strip(), """\
declare void @"llvm.memset.p0i8.i32"(ptr %".1", i8 %".2", i32 %".3", i1 %".4")""") # noqa E501
declare void @"llvm.memset.p0.i32"(ptr %".1", i8 %".2", i32 %".3", i1 %".4")""") # noqa E501
self.check_descr(self.descr(memcpy).strip(), """\
declare void @"llvm.memcpy.p0i8.p0i8.i32"(ptr %".1", ptr %".2", i32 %".3", i1 %".4")""") # noqa E501
declare void @"llvm.memcpy.p0.p0.i32"(ptr %".1", ptr %".2", i32 %".3", i1 %".4")""") # noqa E501
else:
self.check_descr(self.descr(memset).strip(), """\
declare void @"llvm.memset.p0i8.i32"(i8* %".1", i8 %".2", i32 %".3", i1 %".4")""") # noqa E501
Expand Down Expand Up @@ -2451,9 +2451,12 @@ def assert_ne(ptr1, ptr2):
def test_ptr_intrinsic_name(self):
self.assertEqual(ir.PointerType().intrinsic_name, 'p0')
self.assertEqual(ir.PointerType(addrspace=1).intrinsic_name, 'p1')
# Note: Should this be adjusted based on the pointer mode?
self.assertEqual(ir.PointerType(int1).intrinsic_name, 'p0i1')
self.assertEqual(ir.PointerType(int1, 1).intrinsic_name, 'p1i1')
if not ir_layer_typed_pointers_enabled:
Copy link
Member

@gmarkall gmarkall Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the changes here pertain to the fix I asked about above. Is that right?

Copy link
Contributor Author

@rj-jesus rj-jesus Mar 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I wasn't sure if we should remove intrinsic names from typed pointers entirely (such that we would always print them in opaque pointer format), but then I thought we should probably keep consistency with _to_string, and so I've adjusted this accordingly.

self.assertEqual(ir.PointerType(int1).intrinsic_name, 'p0')
self.assertEqual(ir.PointerType(int1, 1).intrinsic_name, 'p1')
else:
self.assertEqual(ir.PointerType(int1).intrinsic_name, 'p0i1')
self.assertEqual(ir.PointerType(int1, 1).intrinsic_name, 'p1i1')

def test_str(self):
"""
Expand Down
Loading
Loading