[Caching] Fix per-construct split false fallback on ndarray-vs-non-ndarray mixed pointers - #905
Open
hughperkins wants to merge 2 commits into
Open
[Caching] Fix per-construct split false fallback on ndarray-vs-non-ndarray mixed pointers#905hughperkins wants to merge 2 commits into
hughperkins wants to merge 2 commits into
Conversation
…array mixed ptrs The whole-element/component recompute-safety guard (added to close a real alias_analysis blind spot) had a conservative catch-all that rejected ANY mixed ExternalPtr/MatrixPtr pair whose matrix side did not normalize to an external pointer. But a MatrixPtr over a local alloca, a global temp, or a field is a different memory space than an ndarray read, and alias_analysis already reports such a pair `different` (checked before this guard runs). Overriding that to "may overlap" disabled the split for any kernel that recomputes a whole-element ndarray read past a component write to a local/temp/field -- including qipc's giant graph_do_while `_step_kernel`, which regressed warm+1-edit time-to-first- step from ~11s back to ~27s (whole-kernel recompile). Fix: `as_ndarray_ptr` now follows the matrix-ptr chain to the underlying ndarray external pointer (handling higher-rank elements), and the guard only overrides alias_analysis when BOTH sides are ndarray-backed; otherwise it defers to alias_analysis's verdict. The genuine whole-element-vs-component ndarray hazard (same ndarray, mixed access) still falls back. Validated on a Blackwell node: new regression test fails before / passes after; the true-positive test still falls back; full test_per_offload_cache.py suite 78 passed / 3 skipped (cpu+cuda); qipc warm+1-edit 26.8s -> 11.6s; qipc test_abd_freefall.py 4 passed (split output correct).
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Collaborator
Author
|
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
1 participant
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
After #900 merged, qipc's giant
graph_do_while_step_kernelstopped using the per-construct split: warm + single-edit time-to-first-step regressed from ~11s back to ~27s (a whole-kernel recompile), defeating the point of the split for the flagship kernel.Root cause is the whole-element/component recompute-safety guard added in #900's final review round (
whole_element_read_may_overlap_component_write). It correctly closes a realalias_analysisblind spot (a whole-element ndarray reada[i]vs a component writea[j][c]=...to the same ndarray, whichalias_analysiswrongly treats as disjoint). But its conservative catch-all rejected any mixedExternalPtr/MatrixPtrpair whose matrix side did not normalize to an external pointer:Hard evidence from an instrumented build on a Blackwell node — the pair flagged for
_step_kernel:A
MatrixPtrover a localalloca/ global temp / field is a different memory space than an ndarray read;alias_analysisalready reports the pairdifferent(checked before this guard runs). Overriding that to "may overlap" is a false positive.Fix
as_ndarray_ptrnow follows the matrix-ptr chain down to the underlying ndarrayExternalPtrStmt(handles higher-rank elements), returning null only when the base genuinely isn't an ndarray.alias_analysisonly when both sides are ndarray-backed; otherwise it defers toalias_analysis's verdict. The genuine whole-element-vs-component ndarray hazard still falls back.Validation (Blackwell node)
..._whole_element_vs_non_ndarray_component_okfails before / passes after; the true-positive..._whole_element_vs_component_recompute_unsafestill falls back.tests/python/test_per_offload_cache.py: 78 passed, 3 skipped (cpu+cuda)._step_kernelwarm + single-edit time-to-first-step: 26.8s -> 11.6s (compile_kernel 21.7s -> 5.2s); cold 32.5s -> 19.8s.test_abd_freefall.py: 4 passed (split output correct).Made with Cursor