Drop null problem-cache sentinels on load, keep in-run dedup - #5204
Open
danieyan-amd wants to merge 7 commits into
Open
Drop null problem-cache sentinels on load, keep in-run dedup#5204danieyan-amd wants to merge 7 commits into
danieyan-amd wants to merge 7 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a GPU problem-cache edge case where null sentinel entries (from mark()) were previously treated as valid solutions, causing compilation to be skipped and leading to crashes or missing tuned solutions. The fix centralizes sentinel handling in problem_cache::get() and adds regression coverage to prevent future regressions.
Changes:
- Update
problem_cache::get()to treatnullentries as cache misses (and continue searching lower-priority layers). - Remove now-dead null-skip logic in MLIR
compile_opsand hardenis_module_fusibleagainst non-string/nullsolutions. - Add GPU regression tests covering null sentinels and cache-layer precedence.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/targets/gpu/problem_cache.cpp |
Treat null cache entries as misses during lookup, preventing sentinel values from being used as solutions. |
src/targets/gpu/compile_ops.cpp |
Remove redundant null-sentinel handling now covered by problem_cache::get(). |
src/targets/gpu/mlir.cpp |
Guard is_module_fusible against non-string/null solution values to avoid null dereferences. |
test/gpu/problem_cache_path_override.cpp |
Add regression tests ensuring null sentinels behave as misses and don’t hide lower-layer solutions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
154
to
159
| const auto search = [&](const std::vector<problem_cache_backend>& backends) -> optional<value> { | ||
| const auto it = std::find_if(backends.begin(), backends.end(), [&](const auto& b) { | ||
| return b.get(device_key, key).has_value(); | ||
| }); | ||
| const auto it = std::find_if( | ||
| backends.begin(), backends.end(), [&](const auto& b) { return bool(usable(b)); }); | ||
| if(it != backends.end()) | ||
| return it->get(device_key, key); | ||
| return usable(*it); | ||
| return {}; |
pfultz2
requested changes
Aug 27, 2026
danieyan-amd
pushed a commit
that referenced
this pull request
Aug 27, 2026
A null mark() sentinel is a transient in-run 'benchmark in progress' signal that compile_ops relies on to benchmark a repeated problem only once. Treating it as a cache miss (the prior approach) removed that dedup and re-benchmarked the same problem once per instruction (pfultz2 review on #5204). Keep the in-run dedup (get() returns the in-memory mark; compile_ops skips on a null) and instead drop null sentinels at the load boundary in json and sqlite backends. A persisted or shipped null can no longer come back, so it cannot cause the original skip-and-crash (AIRADSW-871), and repeated problems are still benchmarked once. Updates the backend round-trip tests (a sentinel no longer survives save/load) and adds a load-skips-nulls test covering both the read-only and writable load paths. Signed-off-by: danieyan-amd <daniel.anieyan@amd.com>
danieyan-amd
changed the title
A null mark() sentinel is a transient in-run 'benchmark in progress' signal that compile_ops relies on to benchmark a repeated problem only once. Treating it as a cache miss (the prior approach) removed that dedup and re-benchmarked the same problem once per instruction (pfultz2 review on #5204). Keep the in-run dedup (get() returns the in-memory mark; compile_ops skips on a null) and drop null sentinels when serializing, so the engine never writes a broken cache in the first place: json save() prunes nulls into a copy of the map (save() is const) and sqlite save() skips null rows in the insert loop. load() no longer filters, keeping the read path simple. A persisted null can no longer be produced, so it cannot cause the original skip-and-crash (AIRADSW-871), and repeated problems are still benchmarked once. Updates the backend round-trip tests (a sentinel no longer survives save/load) and renames the sentinel test to cover the save path. Signed-off-by: danieyan-amd <daniel.anieyan@amd.com>
danieyan-amd
force-pushed
the
problem-cache-null-miss
branch
from
August 31, 2026 20:18
ce71875 to
4e9f02e
Compare
pfultz2
reviewed
Sep 1, 2026
A null/non-string solution has no tuning config; the MLIR backend pipeline rejects it downstream (compile() throws the tuning-solution error), so detect it here and fail fast with a clear error instead of treating the module as fusible (pfultz2 review on #5204). Signed-off-by: danieyan-amd <daniel.anieyan@amd.com>
Drop the inaccurate claim that a null can only be a fresh in-memory mark: load() does not filter, so a pre-existing cache file can still supply one. Signed-off-by: danieyan-amd <daniel.anieyan@amd.com>
Contributor
Author
|
Shortened comments on the last commit |
pfultz2
approved these changes
Sep 4, 2026
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
A null
mark()entry in the problem cache is a transient, in-run "benchmark in progress" sentinel. Within one compile,compile_opsbenchmarks a repeated problem once: the first instance marks it, and duplicate instances see the null viaget()and skip, reusing the winner. The null was stored in the same map that is persisted to disk, so a null that never got overwritten (a failed or interrupted benchmark) could be persisted, shipped, and re-loaded. On the next runget()returned that stale null and the op was skipped forever, leaving it unresolved →0xC0000005inis_module_fusible/ "No valid tuned compilation" (AIRADSW-871, BERT pooler on Navi48/gfx1201). Deletingproblem_cache.jsoncleared the nulls and worked around it.Fix
Keep the null purely in memory for its one legitimate job (in-run dedup); never let it cross to or from disk.
compile_ops(if(solution.is_null()) return;).get()returns the first hit (a null it returns is only ever a fresh in-memory mark, since load now drops persisted nulls).jsonandsqliteload()drop null entries, so a persisted or shipped null (read-only or writable) is a plain miss and the op is compiled and tuned fresh. This is the crash fix.is_module_fusiblenull/non-string guard is kept (separate no-config path).This supersedes the earlier revision of this PR, which treated a null as a cache miss — that removed the in-run dedup and re-benchmarked a repeated problem once per instruction (raised in review). Filtering on load (not save) covers caches that were already written badly and avoids copying the whole cache to strip nulls.
Tests
problem_cache_load_skips_null_sentinels— a persisted null is dropped on load (read-only and writable paths).problem_cache_writable_mark_is_sentinel— an in-run mark is still returned for dedup._round_triptests updated: a null sentinel no longer survives save/load (real solutions still do).Verified on a reduced MLIR-off gfx1100 build:
migraphx_gpubuilds and all four problem-cache test binaries pass.