Add IR layer typed pointers mode #1159
Conversation
| @@ -190,17 +186,6 @@ def gep(self, i): | |||
| def intrinsic_name(self): | |||
| return 'p%d%s' % (self.addrspace, self.pointee.intrinsic_name) | |||
|
|
|||
| @classmethod | |||
There was a problem hiding this comment.
The opaque pointer version of this shouldn't be affected by this patch (which is why the tests aren't affected either).
I think the typed version can't survive, but my understanding from #1064 (comment) was that this was okay.
There was a problem hiding this comment.
Thanks for this @rj-jesus !
I think the general idea is good and I'd be keen to press on with it. I'd hope our strategy would be something like:
- Merge this PR
- Get comfortable that nothing Numba-related is broken by it
- Bump the minimum LLVM version to 16 (because it already builds with it anyway, this should be a simple bump)
- Remove the references to typed pointers in the binding layer to clean things up a bit
- Move to later LLVM versions at our convenience
The only idea I'd like to float is WRT naming - we have "opaque pointers" and "lazy opaque pointers" which on the face of it don't have much specific meaning. I'd like to propose that:
- What we're adding here is called "IR layer typed pointers" (or "IR layer opaque pointers", with the required inversion of boolean conditions)
- What is enabled by the
_opaque_pointers_enabled()function is called "Binding layer opaque pointers"
I think this naming makes it a bit clearer where the effects and differences lie in terms of the abstractions presented by llvmlite. I would be keen to hear the perspective of @sklam and @stuartarchibald here too though.
gmarkall
added
4 - Waiting on author
4 - Waiting on second reviewer
and removed
3 - Ready for Review
labels
|
Hi @gmarkall, thanks very much for the feedback! Your strategy sounds very good to me. On the naming issue, I'm more than happy to rename "lazy opaque pointers" to "IR layer opaque pointers" or something else entirely, "lazy opaque pointers" was just a work-in-progress name in any case! As far as the normal "opaque pointer mode" goes, while I'm also very happy to rename it, I think maybe we should just remove it entirely. It won't be an option per se going forward as there will be no alternative, and it was only introduced in the first place as a stop-gap solution to enable testing and porting. We've had it for a release cycle, which I believe was the initial plan. What do you think? |
Co-authored-by: Graham Markall <[email protected]>
Co-authored-by: Graham Markall <[email protected]>
I think this is a good idea, and it would save an iteration compared to my original plan - I prefer what you suggest instead. |
Thanks... Will wait and see what others think of the naming. |
At the LLVM level, typed pointers are no longer supported going forward. This ticket removes typed pointer support at the LLVM binding layer and bumps the LLVM release used to LLVM 16. This builds on top of numba#1159.
rj-jesus
changed the title
|
Hi @gmarkall, as discussed I've raised #1160 to drop the old "binding layer" opaque pointers mode (and bump the LLVM release supported to 16), and I've also just committed the rename "lazy opaque pointers" to "IR layer typed pointers" you had suggested. Can you think of anything else we should be doing? |
Not at this point - thanks for the updates! |
|
From triage meeting - I'll continue to review this until it's RTM. |
gmarkall
added
4 - Waiting on second reviewer
4 - Waiting on CI
and removed
4 - Waiting on reviewer
4 - Waiting on CI
labels
There was a problem hiding this comment.
Thank you very much for these changes @gmarkall! Everything looks good, I just left a few comments about some of the phrasing, but nothing special.
I can't approve this explicitly, but this generally looks good to me.
|
Thanks for the review @rj-jesus - I think your comments all make sense, so I've aimed to address / follow them - let me know what you think. |
There was a problem hiding this comment.
Thank you very much for the changes, @gmarkall! I think it's looking really good, I think the distinction between Typed and Opaque Pointers is clear now.
I can't formally approve the review, but please consider it approved.
This patch adds preliminary support for a "lazy opaque pointers" option whereby we emit IR with typed pointers if pointee types are known (even if opaque pointers are enabled). This makes numba-cuda work with opaque pointers enabled and should unblock #1082.
I've enabled opaque pointers unconditionally and made lazy opaque pointers on by default to facilitate testing. These changes could (should?) be moved to a separate PR if required.