Skip to content

Expose ObjectCode as public API + prune unnecessary input arguments #435

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 11 commits into from
Feb 19, 2025

Conversation

ksimpson-work
Copy link
Contributor

@ksimpson-work ksimpson-work commented Feb 4, 2025

close #194
close #448
close #450

Copy link
Contributor

copy-pr-bot bot commented Feb 4, 2025

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.

@ksimpson-work ksimpson-work self-assigned this Feb 4, 2025
@ksimpson-work ksimpson-work added P0 High priority - Must do! cuda.core Everything related to the cuda.core module labels Feb 4, 2025
@ksimpson-work ksimpson-work added this to the cuda.core beta 3 milestone Feb 4, 2025
@ksimpson-work
Copy link
Contributor Author

/ok to test

@ksimpson-work
Copy link
Contributor Author

/ok to test

@leofang leofang self-assigned this Feb 16, 2025
@leofang
Copy link
Member

leofang commented Feb 16, 2025

/ok to test

@leofang leofang added the feature New feature or request label Feb 16, 2025
@leofang leofang changed the title Remove jit options argument from ObjectCode Expose ObjectCode as public API + prune unnecessary input arguments Feb 16, 2025
Copy link

@ksimpson-work
Copy link
Contributor Author

I think we should invest the time to change the design so that ObjectCode only represents cubins, but I think this change can be standalone thanks to the comment about not passing anything but a cubin when directly constructing an ObjectCode.

My thought for ptx / ltoir / other IRs is that we add two things:

Program.code -> returns the code string (for user and needed by linker )
Program.transpile(target_type) -> for advanced users who want to see the IR. give them a method to get their ptx, but isolate it from compile() which will return an ObjectCode that is necessarily wrapping a cubin

Linker will need some modifications. Something like:

Linker.add_code_object becomes Linker.add_program() - this is to pass IR program
Linker.add_object_code(ObjectCodes*)

@ksimpson-work ksimpson-work marked this pull request as ready for review February 18, 2025 18:34
@leofang leofang requested a review from vzhurba01 February 18, 2025 21:13
@leofang
Copy link
Member

leofang commented Feb 18, 2025

We discussed the above comment in an offline meeting. Capturing some thoughts:

  • transpile() would still have multiple possible output code types, same as compile() today
  • We might want to subclass ObjectCode based on the supported code types in the future (ex: PTXObjectCode, CUBINObjectCode, ...). The offline suggestion to add a new from_cubin() constructor, however, makes it possible to defer this discussion to the future and it can be addressed later in a non-breaking fashion. It's also aligned with other parts of cuda.core design and so it is added in this PR now.
  • Whatever API that we come up for wrapping cubins as an ObjectCode, it can also take (in-memory) PTX as valid input due to limitations in driver APIs. There is no programmatic way for us to guard against "misuse" (since the driver considers it a legit use case).

FWIW I think adding ObjectCode.code (instead of Program.code) for retrieving the wrapped content makes a lot of sense. I think not having it was an overlook. Let us add it as part of #370. (ObjectCode does not have .handle, but this is close enough.)

@vzhurba01 since both Keenan and I already touched this PR significantly, please kindly review it.

@leofang
Copy link
Member

leofang commented Feb 18, 2025

/ok to test

@vzhurba01
Copy link
Collaborator

Small test update needed but all else looks good to me

Co-authored-by: Vladislav Zhurba <[email protected]>
@leofang
Copy link
Member

leofang commented Feb 18, 2025

/ok to test

@leofang
Copy link
Member

leofang commented Feb 19, 2025

CI is green!

@ksimpson-work ksimpson-work enabled auto-merge (squash) February 19, 2025 17:12
@ksimpson-work ksimpson-work merged commit 1ac7d2c into NVIDIA:main Feb 19, 2025
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cuda.core Everything related to the cuda.core module feature New feature or request P0 High priority - Must do!
Projects
None yet
3 participants