[Bug] Don't cache may-aliasing buffers in CacheLoopInvariantGlobalVars (#810)#811
[Bug] Don't cache may-aliasing buffers in CacheLoopInvariantGlobalVars (#810)#811hughperkins wants to merge 1 commit into
Conversation
#810) 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 literal-index read a[0] 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 serial-offload path took an unconditional is_offload_unique()==true short-circuit, bypassing the may-alias exclusion the parallel path already gets via gather_uniquely_accessed_pointers. Exclude from caching any buffer accessed within the offload through two distinct pointers P1, P2 with alias_analysis(P1, P2) == uncertain. Buffers whose accesses are all definitely-same or all definitely-different remain cacheable, so read-modify-write accumulators and disjoint accesses are unaffected. Add a regression test (field + ndarray) reproducing the four ingredients: a serialized outer loop, an inner loop, a store through the outer loop variable index, and a guarded literal-index read of the same buffer.
|
@codex review |
|
thoughts on agent on perf: 
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69ea6b0457
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| auto has_may_alias_pair = [](const std::vector<Stmt *> &ptrs) { | ||
| for (int i = 0; i < (int)ptrs.size(); i++) { | ||
| for (int j = i + 1; j < (int)ptrs.size(); j++) { | ||
| if (irpass::analysis::alias_analysis(ptrs[i], ptrs[j]) == AliasResult::uncertain) { |
There was a problem hiding this comment.
When the aliased access is into a vector/matrix ndarray element, both accesses reach this check as MatrixPtrStmts over ExternalPtrStmt origins such as a[i][0] and a[0][0]. alias_analysis() currently returns different for two MatrixPtrStmts whose origins are only uncertain aliases, so this branch does not add the ndarray to may_alias_unsafe_arr_ids_; the cache pass can still cache the literal element load and defer the loop-indexed store, reproducing the stale-read bug for TensorType ndarray elements even though the scalar ndarray test passes.
Useful? React with 👍 / 👎.
|
Comparison with @duburcqa 's AI's version. Verdict is that his AI's output is better, on this occasion: 
|
|
Some nuance: 
|
|
Moving towards a hybrid of both solutions: 
|
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.
|
closing this in favor of #812 |
1 participant
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 literal-index read a[0] 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 serial-offload path took an unconditional is_offload_unique()==true short-circuit, bypassing the may-alias exclusion the parallel path already gets via gather_uniquely_accessed_pointers.
Exclude from caching any buffer accessed within the offload through two distinct pointers P1, P2 with alias_analysis(P1, P2) == uncertain. Buffers whose accesses are all definitely-same or all definitely-different remain cacheable, so read-modify-write accumulators and disjoint accesses are unaffected.
Add a regression test (field + ndarray) reproducing the four ingredients: a serialized outer loop, an inner loop, a store through the outer loop variable index, and a guarded literal-index read of the same buffer.
Issue: #
Brief Summary
copilot:summary
Walkthrough
copilot:walkthrough