[SYCL] Augment SYCL_DUMP_IMAGES to dump only used device images - #22996
Open
uditagarwal97 wants to merge 5 commits into
Open
[SYCL] Augment SYCL_DUMP_IMAGES to dump only used device images#22996uditagarwal97 wants to merge 5 commits into
SYCL_DUMP_IMAGES to dump only used device images#22996uditagarwal97 wants to merge 5 commits into
Conversation
Handle SYCL_DUMP_IMAGES like the other SYCL RT environment variables: add it to config.def and give it a SYCLConfig specialization instead of reading it with std::getenv in ProgramManager. When images are dumped upon use, print the name of the file each image was dumped to, and report the file name again when an already dumped image is used by a subsequent build. Also document that any positive value other than 2 dumps all device images and that dumping is off by default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Check that level 2 dumps a device image on first use, reports the file it was dumped to, reports a second kernel from the same image as already dumped, and does not dump anything at image load time, while level 1 dumps all loaded images without reporting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Drop the single-use DumpImages local in ProgramManager::addImage, tighten blank lines in SYCLConfig<SYCL_DUMP_IMAGES>, and remove the dump-file existence RUN lines from the E2E test since the FileCheck patterns already cover which images get dumped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends SYCL_DUMP_IMAGES to support a new mode that dumps only the device images actually used at runtime (and reports the dump filename), while preserving the existing “dump all loaded images” behavior and documenting the updated semantics.
Changes:
- Add
SYCL_DUMP_IMAGES=2(“dump used images only”) behavior inProgramManager::getBuiltURProgram, including reporting and de-duplication of dumps. - Refactor
dumpImage()to return the written filename and introduce a typedSYCLConfig<SYCL_DUMP_IMAGES>config parser. - Add an end-to-end test for dump levels and update user documentation for the new
SYCL_DUMP_IMAGESbehavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| sycl/test-e2e/SPVDumpUse/dump_levels.cpp | New e2e coverage for “dump used only” vs “dump all” behavior. |
| sycl/source/detail/program_manager/program_manager.hpp | dumpImage() now returns filename; adds state to track images dumped-on-use. |
| sycl/source/detail/program_manager/program_manager.cpp | Implements “dump used only” and adjusts “dump all” gating via SYCLConfig. |
| sycl/source/detail/config.hpp | Adds SYCLConfig<SYCL_DUMP_IMAGES> parsing helpers (dumpAll, dumpUsedOnly). |
| sycl/source/detail/config.def | Registers SYCL_DUMP_IMAGES as a config key. |
| sycl/doc/EnvironmentVariables.md | Documents the integer semantics, including level 2 used-only dumping. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Treat an explicit SYCL_DUMP_IMAGES=0 as "off" instead of "dump all", so the value matches the documentation. A non-numeric value still dumps all images, preserving the historical behavior of the variable. - Replace the function-local static sequence ID used by the dump-on-use path with a ProgramManager member guarded by m_DumpedImagesMutex. - Erase the image from m_DumpedImages in removeImages(), so the map does not grow unboundedly and a reused image address cannot be mistaken for an already dumped one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
YuriPlyakhin
approved these changes
Aug 24, 2026
YuriPlyakhin
left a comment
YuriPlyakhin
left a comment
Contributor
There was a problem hiding this comment.
sycl/doc/EnvironmentVariables.md LGTM
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
SYCL_DUMP_IMAGESdumps every device image the runtime loads, at load time. For an application that links many device images but only runs a few kernels (like any PyTorch test), the output is dominated by images that are never used, and there is no way to tell from the dump which of them the runtime actually built and ran.Changes proposed in this PR
SYCL_DUMP_IMAGESis now read as an integer level:2stderr.