Drop binding layer typed pointer support. #1160
Conversation
|
Note from triage meeting: items pinned for opaque pointer fixes may also be required here. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
At the LLVM level, typed pointers are no longer supported going forward. This patch removes typed pointer support at the LLVM binding layer and bumps the LLVM release used to LLVM 16.
rj-jesus
force-pushed
the
rjj/move-to-llvm-16
branch
from
b6fc854 to
603bace
Compare
rj-jesus
changed the title
| #endif | ||
| } | ||
| return nullptr; | ||
| return size.getFixedValue(); |
There was a problem hiding this comment.
Note on this change: getFixedSize() retured getFixedValue() anyway, and getFixedSize() is deprecated / removed in later LLVM versions.
There was a problem hiding this comment.
Yep, I believe these are just API renames.
gmarkall
left a comment
gmarkall
left a comment
There was a problem hiding this comment.
I think this is good, but I have a couple of questions on the diff to complete my understanding of the changes.
| @property | ||
| def intrinsic_name(self): | ||
| return 'p%d%s' % (self.addrspace, self.pointee.intrinsic_name) | ||
| if ir_layer_typed_pointers_enabled: |
There was a problem hiding this comment.
I think this is a fix for an error that was previously un-noticed / un-addressed - is that correct?
There was a problem hiding this comment.
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).
| # 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: |
There was a problem hiding this comment.
I think the changes here pertain to the fix I asked about above. Is that right?
There was a problem hiding this comment.
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.
gmarkall
left a comment
gmarkall
left a comment
There was a problem hiding this comment.
Many thanks for the PR! I think this is good to approve, noting that:
- I've experimented with this locally and run into no issues, including running the Numba test suite.
- This isn't changing behaviour from the default, which has already had a full buildfarm run.
- It only removes the non-default, deprecated path, which needs to be removed to move forward with LLVM versions.
3 participants
At the LLVM level, going forward typed pointers are no longer supported.
This patch removes typed pointer support from the LLVM binding layer.
Initially, it also attempted to bump the LLVM version used to LLVM 16, but due to unrelated reasons with Anaconda, it seems it's easier to do that separately as a follow-up patch.