Mostly CUDA: Replace llvmpy API usage with llvmlite APIs #6813
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The llvmpy API exists only for compatibility; this commit replaces all uses of the llvmpy APIs with llvmlite IR builder API calls in the CUDA target. Notes: - Some of the functionality in the `llvmlite.llvmpy.core.Module` subclass of `llvmlite.ir.Module` has been moved into new functions in cgutils. - Because CUDA modules are now llvmlite.ir.Module instances, some of the core calls to `mod.get_or_insert_function` are replaced with calls to the new `cgutils` functions, as the CUDA modules also pass through these functions in the core.
gmarkall
force-pushed
the
grm-cuda-remove-llvmpy
branch
from
f63cfa2 to
2f0af50
Compare
This required a few changes because llvmlite Modules don't have `get_global_variable_named()` and `add_function()` methods.
gmarkall
added
3 - Ready for Review
Effort - medium
gmarkall
requested review from
DrTodd13,
esc,
sklam and
stuartarchibald
as code owners
Contributor
|
@gmarkall thanks for the patch, please could you resolve the conflict against mainline, many thanks! |
Member
Author
|
@stuartarchibald Conflicts resolved! |
Contributor
Many thanks! |
Contributor
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for doing this refactoring. The addition of the few functions to cgutils helps a lot with reducing usage of methods on the module itself. Couple of minor comments else looks good to go. Thanks again!
|
|
||
| if dynamic_smem: | ||
| gvmem.linkage = lc.LINKAGE_EXTERNAL | ||
| gvmem.linkage = 'external' |
Contributor
There was a problem hiding this comment.
Wonder if the enum in llvmlite ought to be moved out of llvmpy and into the llvmlite API such that reliance on strings is reduced? (Not for this PR!).
Member
Author
There was a problem hiding this comment.
Maybe... I assumed that the use of strings instead of the enum was a deliberate design decision to permit flexibility in the linkage types for the user of llvmlite, but it's probably flexibility that's turned out to be un-needed (unless it's being used by external non-Numba users).
stuartarchibald
added
4 - Waiting on author
Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
gmarkall
added
4 - Waiting on reviewer
stuartarchibald
previously approved these changes
Mar 12, 2021
Contributor
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch and fixes.
stuartarchibald
added
4 - Waiting on CI
Contributor
|
Buildfarm ID: |
Member
|
This one fails on CUDA 9.0 and 10.0 tests with: |
esc
added
BuildFarm Passed
Member
|
@gmarkall thanks for the fixes, |
Member
Author
|
@esc many thanks! |
stuartarchibald
approved these changes
Mar 15, 2021
Contributor
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the fix, looks good.
stuartarchibald
added
5 - Ready to merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR replaces all llvmpy API usage with llvmlite API usage in the CUDA target. I started this replacement work because I often want to do something based on existing code in the CUDA target, but I'm loathe to add new code using the compatibility layer, so I felt it would be better to make the entire CUDA target an exemplar of the right API. This spidered out of just the CUDA target because a lot of code is shared between the core and the CUDA target, so the changes are:
Moduleclass with the llvmliteModuleclass. I started by replacing only CUDA target modules with the llvmliteModuleclass, but since it passes through the core so much, with the possibility of a lack of test coverage for some basic functionality (simple mathematical operators, etc, might not be explicitly tested by the CUDA target, with the assumption that simple nopython mode things generally work) I thought the safest route was to make sure that no target uses an llvmpyModule.cgutilsnow has some extra methods to replace functionality that was in the llvmpyModuleclass - these functions are now called where the oldModulemethod was used. I didn't replaceget_global_variable_named()with anything incgutilsbecause it was used exactly once.I'm marking this as medium effort because although it's quite a long PR, the majority of it is very mechanical substitutions.