Skip to content
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

Jump to bottom

Add the cuda.core.experimental.Linker class #229

Merged
merged 34 commits into from
Dec 7, 2024
Merged

Add the cuda.core.experimental.Linker class #229

merged 34 commits into from
Dec 7, 2024

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Nov 8, 2024 •
edited
Loading

wrap nvjitlink into the object model.
Depends on #224

@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Nov 8, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@leofang leofang added this to the cuda.core beta 2 milestone Nov 11, 2024
@ksimpson-work ksimpson-work force-pushed the ksimpson/cuda_core_linker_155 branch from 1916359 to 81086e0 Compare November 13, 2024 17:33
@ksimpson-work ksimpson-work marked this pull request as ready for review November 13, 2024 17:34
rwgk
rwgk reviewed Nov 13, 2024
View reviewed changes
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/tests/test_linker.py Outdated Show resolved Hide resolved
cuda_core/tests/test_linker.py Outdated Show resolved Hide resolved
@leofang leofang added P0 High priority - Must do! feature New feature or request cuda.core Everything related to the cuda.core module labels Nov 14, 2024
@ksimpson-work
Copy link
Contributor Author

/ok to test

@leofang leofang requested a review from gmarkall November 26, 2024 20:15
leofang
leofang requested changes Nov 27, 2024
View reviewed changes
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

Thanks, Keenan! I haven't reviewed the tests yet (they seem OK from a quick glance). Left some comments below.

Please also modify docs/.../api.rst to document LinkerOptions/Linker and make sure the docs build without warnings and render OK.

cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated
Comment on lines 184 to 197
if self.xptxas is not None:
for opt in self.xptxas:
self.formatted_options.append(f"-Xptxas={opt}")
Copy link
Member

Choose a reason for hiding this comment

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

Graham can correct me but there's probably a better way to pass multiple options to -Xptxas. Could you check if this is tested?

cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/cuda/core/experimental/_linker.py Outdated Show resolved Hide resolved
cuda_core/tests/test_linker.py Outdated Show resolved Hide resolved
@rwgk rwgk changed the title Add the cuda.core.experiemental.Linker class Add the cuda.core.experimental.Linker class Nov 27, 2024
@ksimpson-work ksimpson-work force-pushed the ksimpson/cuda_core_linker_155 branch from aed4c23 to 9d8ecbc Compare November 27, 2024 21:06
@ksimpson-work
Copy link
Contributor Author

TODO - fix the finalizer. Once Ralf's change is complete I will implement the finalizer to match.

@rwgk
Copy link
Collaborator

rwgk commented Dec 2, 2024

Hi @ksimpson-work, I just merged #246. — The pattern for using weakref.finalize() isn't formalized, but it's very easy to adopt just by looking at the PR. Please let me know any questions. I'd be happy to meet 1:1 to explain.

@ksimpson-work
Copy link
Contributor Author

Thanks for the ping! I will look to implement now

@ksimpson-work ksimpson-work force-pushed the ksimpson/cuda_core_linker_155 branch from 88c5ed2 to 702fbaa Compare December 4, 2024 20:02
@ksimpson-work
Copy link
Contributor Author

Tests are updated to link ptx inputs vs ltoir inputs when running with culink backend. The functions are all device, using a global was consistently causing an error when linking (undefined reference to B, C) although B and C were both declared in A and passed to the linker.

@ksimpson-work
Copy link
Contributor Author

Upon further investigation, the issue with linking ptx inputs was not an issue with the linker, but rather a user error in the compilation step. relocatable device code option was required in order to compile and output unreferenced device functions. This is, of course, a requirement to link them to some declaration in the entrypoint file.

@ksimpson-work
Copy link
Contributor Author

lazy load modules in #268

@ksimpson-work ksimpson-work mentioned this pull request Dec 5, 2024
Open
@leofang
Copy link
Member

leofang commented Dec 7, 2024

/ok to test

@leofang
Copy link
Member

leofang commented Dec 7, 2024

Windows failures are known (#271) and irrelevant. Let's merge. Thanks, Keenan!

cc @gmarkall @isVoid for vis

@leofang leofang merged commit 0ca509e into main Dec 7, 2024
11 of 12 checks passed
@leofang leofang deleted the ksimpson/cuda_core_linker_155 branch December 7, 2024 04:44
@leofang leofang linked an issue Dec 7, 2024 that may be closed by this pull request
Add cuda.core.Linker #155
Closed
@leofang leofang mentioned this pull request Dec 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwgk rwgk rwgk left review comments

@leofang leofang leofang approved these changes

@vzhurba01 vzhurba01 Awaiting requested review from vzhurba01

@gmarkall gmarkall Awaiting requested review from gmarkall

Assignees

@ksimpson-work ksimpson-work

Labels
cuda.core Everything related to the cuda.core module feature New feature or request P0 High priority - Must do!
Projects
None yet
Milestone
cuda.core beta 2
Development

Successfully merging this pull request may close these issues.

Add cuda.core.Linker
3 participants
@ksimpson-work @rwgk @leofang