[Bug] Exclude may-aliasing buffers from loop-invariant caching (#810)#812
Open
hughperkins wants to merge 3 commits into
Open
[Bug] Exclude may-aliasing buffers from loop-invariant caching (#810)#812hughperkins wants to merge 3 commits into
hughperkins wants to merge 3 commits into
Conversation
CacheLoopInvariantGlobalVars could scalarize a loop-invariant global access into a local slot and defer its write-back until after the loop, while a distinct may-aliasing access to the same buffer read/wrote global memory directly. When the two addresses coincide at runtime (e.g. a store through a loop-variable index a[i] and a guarded read a[0] or a[t] in a serialized loop) the direct access observed a stale value, so the in-kernel read returned the pre-write-back value even though post-kernel memory was correct. The pass consults gather_dynamically_indexed_pointers as its aliasing guard, but that analysis only flagged pointers with a non-const/non-loop index, and treated a const-indexed and a loop-index access as independently safe, so their mutual aliasing was never recorded. The serial-offload path relies on this guard because is_offload_unique() short-circuits to true for serial tasks, bypassing the uniqueness analysis the parallel path uses. Flag a buffer's access pointer as non-cacheable when it may alias another access to the same buffer, decided by alias_analysis(P1, P2) == uncertain. Accesses that are all definitely-same or all definitely-different remain cacheable, so read-modify-write accumulators and disjoint accesses (e.g. a[0, i] and a[1, i]) are unaffected. Using the alias oracle rather than an index-kind heuristic also catches two distinct loop indices (a[i] vs a[t]) that a structural const-vs-loop guard would miss. Add regression tests (field + ndarray) for both the literal-index and the loop-index aliasing patterns.
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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". |
Covers the PR #811 review case: aliasing accesses through vector/matrix elements (a[i_c][0] store vs guarded a[0][0] read) reach the caching pass as MatrixPtrStmts, where alias_analysis returns 'different' for uncertain origins. The origin-keyed guard in gather_dynamically_indexed_pointers still excludes them because the cache pass resolves each MatrixPtr to its ExternalPtr/GlobalPtr origin.
Empirical check on x64 (fix reverted vs applied) shows the vector/matrix element pattern from the PR #811 review is not miscompiled on baseline main: across ndarray-vector, field-vector, matrix-field, and two-loop-index variants the matrix-element caching path never defers the write-back the scalar path does. The test passes with or without the fix, so rename it and rewrite the docstring to describe it as a forward-looking guard rather than a regression.
Collaborator
Author
|
@duburcqa Note: 2-3% regression on CPU tests, it looks like. Will dig 
|
Collaborator
Author
|
Genesis unit-tests pass though: 
|
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.
CacheLoopInvariantGlobalVars could scalarize a loop-invariant global access into a local slot and defer its write-back until after the loop, while a distinct may-aliasing access to the same buffer read/wrote global memory directly. When the two addresses coincide at runtime (e.g. a store through a loop-variable index a[i] and a guarded read a[0] or a[t] in a serialized loop) the direct access observed a stale value, so the in-kernel read returned the pre-write-back value even though post-kernel memory was correct.
The pass consults gather_dynamically_indexed_pointers as its aliasing guard, but that analysis only flagged pointers with a non-const/non-loop index, and treated a const-indexed and a loop-index access as independently safe, so their mutual aliasing was never recorded. The serial-offload path relies on this guard because is_offload_unique() short-circuits to true for serial tasks, bypassing the uniqueness analysis the parallel path uses.
Flag a buffer's access pointer as non-cacheable when it may alias another access to the same buffer, decided by alias_analysis(P1, P2) == uncertain. Accesses that are all definitely-same or all definitely-different remain cacheable, so read-modify-write accumulators and disjoint accesses (e.g. a[0, i] and a[1, i]) are unaffected. Using the alias oracle rather than an index-kind heuristic also catches two distinct loop indices (a[i] vs a[t]) that a structural const-vs-loop guard would miss.
Add regression tests (field + ndarray) for both the literal-index and the loop-index aliasing patterns.
Note: this version of the code is strongly based on code from @duburcqa
Issue: #
Brief Summary
copilot:summary
Walkthrough
copilot:walkthrough