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.

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

[clang-linker-wrapper] Dump any device code format #17546

Open
wants to merge 4 commits into
base: sycl
Choose a base branch
from

Conversation

bader
Copy link
Contributor

@bader bader commented Mar 20, 2025

Despite the option name sycl-dump-device-code it enables dumping of only SPIR-V files emitted by the translator tool. This change dumps device code right before embedding it into the fat object. The tool dumps offloading image of any format.

Dispate of the option name sycl-dump-device-code it enables dumping of
SPIR-V files emitted by the translator tool. This change dumps device
code right before embedding it into the fat object. The tool dumps
offloading image of any format.
@bader bader requested a review from a team as a code owner March 20, 2025 01:42
@bader bader temporarily deployed to WindowsCILock March 20, 2025 02:05 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 02:52 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 02:52 — with GitHub Actions Inactive
@bader bader requested a review from a team as a code owner March 20, 2025 04:14
@bader bader temporarily deployed to WindowsCILock March 20, 2025 04:15 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 04:36 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 04:36 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 15:04 — with GitHub Actions Inactive
@bader bader requested a review from maksimsab March 20, 2025 15:07
@bader bader temporarily deployed to WindowsCILock March 20, 2025 16:03 — with GitHub Actions Inactive
@bader bader temporarily deployed to WindowsCILock March 20, 2025 16:03 — with GitHub Actions Inactive
@bader
Copy link
Contributor Author

bader commented Mar 21, 2025

@maksimsab, ping.

@bader
Copy link
Contributor Author

bader commented Mar 24, 2025

@maksimsab, ping^2.

@@ -242,7 +242,7 @@ Flags<[WrapperOnlyOption]>, HelpText<"Embed LLVM IR for runtime kernel fusion">

def sycl_dump_device_code_EQ : Joined<["--", "-"], "sycl-dump-device-code=">,
Flags<[WrapperOnlyOption]>,
HelpText<"Path to the folder where the tool dumps SPIR-V device code. Other formats aren't dumped.">;
HelpText<"Directory to dump offloading images to.">;
Copy link
Contributor

Choose a reason for hiding this comment

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

This option mirrors the user facing option 'sycl-dump-device-code' which was introduced for the old offloading model here: #11132. In that PR, I see that we use this only for dumping SPIR-V code. If the idea is to extend this option to cover all possible device outputs, may be we should do this for the old offloading model as well?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there is any interest in supporting additional formats for "old offloading model".
The old offloading model is supposed to be replaced with the new offloading model soon. Right?

FYI: non-SPIR-V targets seems to be happy with -fsycl-device-only output. See #17390.

If we have customers for this use case, please, file a feature request to handle it separately.


OffloadImageDumpDir = Dir;

if (!DryRun) {
Copy link
Contributor

Choose a reason for hiding this comment

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

May be we can avoid the use of dry-run with -sycl-dump-device-code and emit an error if user does that?

Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why would we want to restrict clang-linker-wrapper this way?

Copy link
Contributor

@asudarsa asudarsa left a comment

Choose a reason for hiding this comment

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

LGTM. Suggested a nit and a possible followup. I am trying to think if we can add some testing for this.
We can add in a separate PR as well.

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants