Drop null problem-cache sentinels on load, keep in-run dedup - #5204
Drop null problem-cache sentinels on load, keep in-run dedup#5204danieyan-amd wants to merge 3 commits into
Conversation
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.
| 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 {}; |
| if(solution.is_null()) | ||
| return; | ||
| // get() never returns a null sentinel (a null cache entry is | ||
| // treated as a miss), so a value here is always a real solution. |
There was a problem hiding this comment.
It should not be treated as a miss. This will cause us to benchmark the problem multiple times instead of just once.
There was a problem hiding this comment.
Agreed, I reverted the null-as-miss change so we keep the original in-run dedup: compile_ops still treats a fresh mark() null as “already being benchmarked,” and get() is back to the minimal first-hit lookup.
It seem like the actual bug was the persistence boundary. A mark() null is only valid during the current run, but it could be saved and loaded later as stale state, causing the op to be skipped even though no benchmark was actually in progress. I fixed that by dropping null entries in the JSON/SQLite load() paths. That way fresh in-memory nulls still dedup correctly, while any persisted null becomes a normal miss and gets re-benchmarked.
I did this on load rather than save so it also cleans up already-existing shipped caches, without adding a full-cache copy on every save. The gfx1100 MLIR-off build/tests are green, including coverage for both writable and read-only persisted nulls.
I think that longer term a possible better design would be to separate the transient mark() state from the persistent solution cache entirely, so keep an in-memory marked/in_progress set and only ever persist completed solutions. That removes the transient/durable state overlap by construction, but it’s a larger refactor than we need for this fix.
There was a problem hiding this comment.
A mark() null is only valid during the current run, but it could be saved and loaded later as stale state
How could it be saved? It shouldnt be saved at the end because we benchmark it and then save the actual solution.
I did this on load rather than save
It shouldnt be fixed on load, we should be fixing it on save so we arent creating broken caches.
so it also cleans up already-existing shipped caches
There is no "shipped" caches yet. And a python script could clean up the corrupted ones that are destined to be shipped.
However, I am not sure fixing it on save actually fixes the problem either because we should have benchmarked everything and saved the actual solutions.
I think that longer term a possible better design would be to separate the transient mark() state from the persistent solution cache entirely, so keep an in-memory marked/in_progress set and only ever persist completed solutions. That removes the transient/durable state overlap by construction, but it’s a larger refactor than we need for this fix.
This sounds like I am not talking to a human here. The mark() method already writes to memory and not to the persistent solution cache, and thats the code you wrote.
There was a problem hiding this comment.
Moved the fix to save instead of load like we discussed. Json now does all of the nulls filtering into a copy before writing (save is const), sqlite skips them in the insert loop, load is untouched, and the in-run deduplication stays. The nulls come from skipped/failed benchmark configs so a mark never gets its solution (separate follow-up other repo).
There was a problem hiding this comment.
Ok ping me when you push out the changes.
There was a problem hiding this comment.
I dont see the changes. Its still skipping on load instead of during save.
There was a problem hiding this comment.
the changes are now pushed
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 <[email protected]>
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 <[email protected]>
ce71875 to
4e9f02e
Compare
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.