-
Notifications
You must be signed in to change notification settings - Fork 71
Support consuming NRT functions from external libraries #167
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.
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
Merged
Changes from 5 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
2f62cb1
initial
brandon-b-miller ec6bf1e
pass compute sanitizer on the python side
brandon-b-miller f52a15c
clean
brandon-b-miller 4df687b
merge/resolve
brandon-b-miller 01be5c2
reset files
brandon-b-miller 0b23a09
check all ptx files in maybe_link_nrt
brandon-b-miller dd568e7
detect nrt in all linked ptx files
brandon-b-miller af9045d
read_file
brandon-b-miller f2e9b29
tests, fill out headerA
brandon-b-miller e4008d0
merge/resolve
brandon-b-miller e65088f
temp file
brandon-b-miller e1e6cc1
Merge branch 'main' into support-cudf-strings
brandon-b-miller 5bd9b7a
move and rename read_file
brandon-b-miller e3042a6
Update numba_cuda/numba/cuda/tests/nrt/test_nrt.py
brandon-b-miller 501b4da
get_include() in tests
brandon-b-miller ff478f4
handle LinkableCode
brandon-b-miller 43b647b
convert to LinkableCode
brandon-b-miller 297c43b
Merge branch 'main' into support-cudf-strings
brandon-b-miller eab1d9e
Revert "convert to LinkableCode"
brandon-b-miller 5c0d87d
merge/resolve/pacify ruff
brandon-b-miller 0dff0b6
style
brandon-b-miller 11a014a
LinkableCode accepts nrt parameter
brandon-b-miller 02ae604
merge/resolve
brandon-b-miller 03cfd92
reorder steps in pynvjitlink wheel test job
brandon-b-miller 613db4a
Update numba_cuda/numba/cuda/dispatcher.py
brandon-b-miller b05fbe8
dont leak memory, cleanup
brandon-b-miller 5134e6c
cache nrt source
brandon-b-miller 4e1329d
Merge branch 'main' into support-cudf-strings
brandon-b-miller a2aa31e
small fix
brandon-b-miller 2edbe6a
reorder test build steps
brandon-b-miller 47a45b4
Merge branch 'main' into support-cudf-strings
brandon-b-miller 34b710d
partially address reviews
brandon-b-miller bd3da22
go back to regex check
brandon-b-miller dc9680b
small fix
brandon-b-miller 71c280b
preserve nrt library through serialization
brandon-b-miller eaebb9a
pass full test suite with nrt and nvidia binding
brandon-b-miller d8469de
merge/resolve
brandon-b-miller dd35975
Merge remote-tracking branch 'NVIDIA/main' into support-cudf-strings
gmarkall ff12e16
drop autodetection of NRT in user supplied raw file paths
brandon-b-miller d893d2b
Apply suggestions from code review
brandon-b-miller e8659aa
Merge branch 'main' into support-cudf-strings
brandon-b-miller ad0a0a7
simplify and update docstring
brandon-b-miller 19c6a56
small bugfix
brandon-b-miller 50b61ac
reoder steps in final CI script
brandon-b-miller File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| #include <cuda/atomic> | ||
|
|
||
| typedef void (*NRT_dtor_function)(void* ptr, size_t size, void* info); | ||
| typedef void (*NRT_dealloc_func)(void* ptr, void* dealloc_info); | ||
|
|
||
| extern "C" | ||
| struct MemInfo { | ||
| cuda::atomic<size_t, cuda::thread_scope_device> refct; | ||
| NRT_dtor_function dtor; | ||
| void* dtor_info; | ||
| void* data; | ||
| size_t size; | ||
| }; | ||
| typedef struct MemInfo NRT_MemInfo; | ||
|
|
||
| extern "C" __device__ void* NRT_Allocate(size_t size); | ||
| extern "C" __device__ void NRT_MemInfo_init(NRT_MemInfo* mi, | ||
| void* data, | ||
| size_t size, | ||
| NRT_dtor_function dtor, | ||
| void* dtor_info); | ||
| extern "C" __device__ void NRT_decref(NRT_MemInfo* mi); | ||
| extern "C" __device__ void NRT_print_refct(NRT_MemInfo* mi); |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want the
nrtkwarg for the JIT decorator to take precedence over the global NRT setting for enabling it? I think this will never link the NRT if it is globally disabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good observation. I think the best behavior might be
But I'm not sure if numba's code generation will actually provide increfs/decrefs without
enable_nrtset to true in the target context, is that correct? In that case we might be adding the refcounting functions to the link but not actually calling them.The motivation for this kwarg is twofold: Where it came up was an edge case I observed in cuDF where an NRT managed object is created and immediately returned, such as in
lambda string: return string. In this case it works out that NRT functions are used, but they only appear in the separately provided PTX file. The ASM that numba inspects when deciding to link NRT or not contains only the declarations for the parent functions that eventually call the NRT ones, so this defeats numba's NRT detection.Overall I feel that unless the NRT detection is absolutely definitive somehow it might be more reliable to let the user decide when to link NRT. This has the drawback of creating two switches, one to enable the possiblity of NRT, and then a second one to actually add it to the link. But even that I think its a little better than somewhat shaky auto detection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that numba-cuda doesn't link the NRT when it should? If so, then I would have seen that as a bug in numba-cuda?
Or, if it adds NRT to the asm when it's not necessary, we should be running the refcount pruning pass to elide those unnecessary NRT calls.
Is there a third situation I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is the case. It is the PTX file that is provided by cuDF that is not checked. The PTX from the numba side is checked, but doesn't contain the actual NRT calls - it only contains a call to a shim function that uses NRT functions within its implementation.
Initially I felt that since cuDF was somewhat burying the NRT calls maybe it should also be the one to signal to numba that the NRT implementations are also needed (or the same for any consuming library that does the same thing).
But I also see that it could be considered a bug in numba-cuda that all the input PTX files are not checked for NRT functions. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we support using the NRT functions from external code, then ideally we should be checking for calls to NRT functions in external code rather than asking the user to do additional work - particularly if it requires signalling in the
@cuda.jitdecorator, because the person writing the jitted function can often be someone different to the person who implemented another library.We've seen this problem with the
linkandextensionskwargs already, where libraries want to use these features, but then have to ask the users to remember to use them with no explicit connection between use of the library and the requirement to add the kwargs.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of the changes in 0b23a09 and dd568e7 ? Here we drop the kwarg and scan the ptx files at the expense of a double read of the same file, something theres probably an easy solution for.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for being late to the discussion.
If it's the library dev duty to specify
nrtarg, how about adding it to theLinkableCodeinterface? The downside is that this cannot be specified if user would want to directly use a path to thelinkargument. Perhaps we can make that an unsupported case or simply deprecate passing in a raw path (and reintroduce it as aPathsubclass toLinkableCode).What let me to think of this is that 0b23a09 NRT discovery mechanism may not be able to handle binary format input such as LTOIR (disassembly could work but I think that's a too much price to pay).
I increasingly see that Numba as a compiler need to separate the duty from lib dev and lib user (kernel writer). And
LinkableCodeinterface can be the perfect vehicle to separate those needs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this sounds like a good idea.
I think that's not too significant a downside. I see the provision of a path to link as a bit of a legacy anyway, because it's not very convenient - it's just what happened to be implemented first, a long time ago, perhaps before the API design had a lot of consideration.
I think introducing a
Pathsubclass might not fit exactly right, because the linker needs to know what type of object is being passed in. A helper function that determines the type based on the extension, reads it in, and then returns the appropriateLinkableCodesubclass instance would probably be OK though. (Perhaps a class methodLinkableCode.from_path(), maybe?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good design.