-
Notifications
You must be signed in to change notification settings - Fork 306
Make ProgramOptions._as_bytes() public with backend support #1351
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
Closed
Closed
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
da9c077
Initial plan
Copilot 8e24725
Make ProgramOptions._as_bytes() public with type annotation and docst…
Copilot d2f144a
Address code review comment - improve test comment clarity
Copilot 523a246
Update docstring to refer to underlying compiler library instead of N…
Copilot fa2ce29
Add backend parameter to as_bytes() method with support for NVRTC, NV…
Copilot 377707d
Improve backend option filtering - remove backend checks from as_byte…
Copilot 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
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.
@copilot it occurs to me that we should take an argument
backendand transforms this accordingly. The reason is that which a subset of options is shared by all compiler libraries (nvrtc/nvjitlink/nvvm), the accepted names are slightly different and unfortunately there exists no unique option. So we need to handle this difference for users.Please inspect the codebase and consolidate a treatment that we can reuse across the
ProgramandLinkerclasses. The handling of thebackendargument can be done in a standalone helper function instead of a class method, if deemed appropriate.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.
@copilot keep #1341 in mind when exploring a plan.
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.
Implemented in fa2ce29. The
as_bytes()method now accepts abackendparameter ("NVRTC", "NVVM", or "nvJitLink") and formats options appropriately for each backend. Created a reusable helper function_format_options_for_backend()that handles the different option naming conventions and value formats across backends:--prefix, "true"/"false" booleans-prefix, "1"/"0" booleans,compute_arch prefix-prefix, "true"/"false" booleansThe method defaults to "NVRTC" for backward compatibility. Added comprehensive tests for all backends.
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'm unable to access issue #1341 details from this environment. Could you provide the key considerations from that issue that should be incorporated into this implementation? If there are specific changes needed to align with #1341, I can make those adjustments.