hipRTC cache: capture #include content via preprocessing (#1335) - #1338
Closed
Noerr wants to merge 3 commits into
Closed
hipRTC cache: capture #include content via preprocessing (#1335)#1338Noerr wants to merge 3 commits into
Noerr wants to merge 3 commits into
Conversation
Noerr
force-pushed
the
fix-hiprtc-cache-includes
branch
2 times, most recently
from
July 17, 2026 18:36
067f7a0 to
9deb217
Compare
Noerr
marked this pull request as ready for review
July 17, 2026 19:21
Noerr
force-pushed
the
fix-hiprtc-cache-includes
branch
from
July 17, 2026 19:21
9deb217 to
cf0a497
Compare
computeHiprtcCacheKey() keyed on the raw source, so it never saw the content of headers pulled in by #include — in particular headers resolved from -I filesystem paths (issue CHIP-SPV#1335). Editing such a header did not invalidate the cache and stale SPIR-V was served silently. When the source may include headers, preprocess it (clang -E -P) and key on the preprocessed translation unit, which embeds all #included content. -P suppresses line markers and -ffile-prefix-map normalizes the temp path so the output is deterministic across runs (verified two cold runs produce identical output). If preprocessing fails for a source that may include headers, caching is disabled for that program (no lookup, no new entry) rather than falling back to a raw-source key: the latter would silently ignore header edits and risk serving stale SPIR-V, i.e. reintroduce the very bug being fixed. To avoid adding a preprocess subprocess to every call, a conservative scan skips it for self-contained sources (no #include/#import): the raw source is then a complete key. The scan de-splices backslash-newline continuations and matches an unanchored '#[ \t]*(include|import)', so it does not miss indented or line-split directives; a match in a comment or string only costs a harmless extra preprocess, never a stale hit. Closes CHIP-SPV#1335.
pvelesko
force-pushed
the
fix-hiprtc-cache-includes
branch
from
July 18, 2026 08:03
cf0a497 to
9ab3dd9
Compare
Contributor
Author
|
rebased to main now that #1336 is merged. |
Collaborator
|
could you please add a test @Noerr |
Noerr
force-pushed
the
fix-hiprtc-cache-includes
branch
from
July 20, 2026 16:41
144d2cb to
1563350
Compare
Adds TestHiprtcCacheIncludes: compiles the same source with the same options four times, changing only the content of a header reached via -I, and reads HIPRTC's own cache decision (hit vs. store) from the compile output: header A -> miss ; A again -> hit ; header B -> miss ; A again -> hit The compile-2 hit proves caching is exercised (the test cannot pass vacuously if caching never happens); the compile-3 miss proves the header content is in the key; the compile-4 hit proves the key tracks content, not timestamps. Compile-only, so it needs no device. Registered through a generic run wrapper (tests/run_test_with_fresh_cache.cmake) that provisions a fresh mktemp'd cache directory per run and sets CHIP_MODULE_CACHE_DIR/CHIP_LOGLEVEL before the process starts (libCHIP reads them at static init). A never-before-used cache name cannot hold a prior run's entries, so the first compile is reliably a cold miss. Verified non-vacuous: with the header omitted from the key, compile 3 hits and the test fails.
Noerr
force-pushed
the
fix-hiprtc-cache-includes
branch
from
July 20, 2026 17:12
1563350 to
a53cb5d
Compare
Contributor
Author
When the preprocess-only pass used to build the hipRTC cache key exits non-zero, preprocessForCacheKey returned nullopt and the caller reported only "could not preprocess source for the cache key" -- the compiler's own error, written to the temp log file, was discarded with the temp dir. Read that log back and emit it via logWarn on failure so the actual reason is visible. This is diagnostic-only: behavior on failure is unchanged (caching stays disabled for that program, never stale).
Contributor
Author
|
Status update on the test @pvelesko: The test passes on macOS (native + translator) and on the Linux rusticl and pocl-7.1/llvm-19 jobs. It fails only on the LLVM 22 / x86 Intel GPU jobs (native-release, native-debug, translator-debug). I added diagnostic logging to surface the cause (it was previously swallowed). On those runners the The unused-argument warnings are benign (identical on macOS); the failure is The change fails safe there — on the crash, caching is disabled for that program (never stale) — but the include-in-key behavior is effectively inert on that platform, and each include-bearing RTC compile now spawns a crashing preprocess subprocess before falling back. Do you have any insight from that CI platform into why the preprocess step might be segfaulting there? |
Collaborator
|
Superseded by #1375 (squashed, rebased on main, authorship preserved). |
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
2 participants
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 (issue #1335)
computeHiprtcCacheKeykeys on the raw HIP source, so it never sees the content of headers pulled in by#include— in particular headers resolved from-Ifilesystem paths. Editing such a header does not change the key, so a stale, previously-cached SPIR-V is served silently.Fix
When the source may include headers, preprocess it (
clang -E -P) and key on the preprocessed translation unit, which embeds all#included content (matching what ROCm/Comgr do).-Psuppresses line markers so no absolute temp paths leak into the key — verified two independent cold preprocesses produce byte-identical output.Self-contained fast path. To avoid adding a preprocess subprocess to every call, a conservative scan skips it when the source has no
#include/#import— then the raw source is already a complete key (in-memory headers are hashed separately; toolchain headers are covered by the compiler-version component from #1336). The scan de-splices\-newline continuations and matches an unanchored#[ \t]*(include|import), so it does not miss indented or line-split directives; a match inside a comment/string only triggers a harmless extra preprocess, never a stale hit.Measurements (LLVM 21.1.8 / POCL main / Apple M4)
#include)#include#include,-Iheader editedInvalidation validated end-to-end: a program that
#includes a filesystem header via-Igets a cache hit on rerun, and a new key (miss) after the header is edited — the exact scenario #1335 describes.Open questions for reviewers
hip_runtime.h(~3MB) whose content is already covered by the compiler-version key, plus hipcc-wrapper overhead. Trimming the forced includes / invokingclangdirectly would cut this substantially, but changes which#ifbranches the preprocessor sees — a correctness subtlety I left out of this PR. Worth a follow-up?#andinclude;\+space+newline) could slip through. If you want zero-doubt safety I can switch to de-splice-then-substring ("include"/"import"), which has no realistic false negatives at the cost of more harmless false positives.Closes #1335.