[Stream] Prevent CSE from aliasing dispatch-allocated buffers#23665
Open
jtuyls wants to merge 1 commit intoiree-org:mainfrom
Open
[Stream] Prevent CSE from aliasing dispatch-allocated buffers#23665jtuyls wants to merge 1 commit intoiree-org:mainfrom
jtuyls wants to merge 1 commit intoiree-org:mainfrom
Conversation
jtuyls
requested review from
MaheshRavishankar,
benvanik,
bjacob and
hanhanW
as code owners
MaheshRavishankar
approved these changes
Mar 5, 2026
| @@ -0,0 +1,143 @@ | |||
| // Tests that two scatter ops consuming the same fill-initialized buffer each | |||
Collaborator
There was a problem hiding this comment.
Can you move this to tests/e2e/regression/ since this is not an op test but rather testing general handling of a program.
hanhanW
reviewed
Mar 6, 2026
jtuyls
force-pushed
the
fix/stream-dispatch-cse-aliasing
branch
from
0e65f7a to
9048997
Compare
AsyncDispatchOp was declared as Stream_PureOp (NoMemoryEffect), which let CSE merge identical dispatches that produce non-tied resource results (fresh allocations). When two consumers each need their own independent buffer (e.g. two ScatterElements writing into separate zero-initialized buffers), merging makes them alias the same buffer and corrupt each other. Fix: implement MemoryEffectsOpInterface on AsyncDispatchOp. The new getEffects() reports MemoryEffects::Allocate when any result is a non-tied resource (i.e. a fresh buffer allocation). This prevents CSE from merging such dispatches while still allowing CSE for dispatches where all results are tied (pure in-place transformations). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Jorn <jorn.tuyls@gmail.com>
jtuyls
force-pushed
the
fix/stream-dispatch-cse-aliasing
branch
from
9048997 to
0199972
Compare
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
3 participants
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.
AsyncDispatchOp was declared as Stream_PureOp (NoMemoryEffect), which let CSE merge identical dispatches that produce non-tied resource results (new allocations). When two consumers each need their own independent buffer (e.g. two ScatterElements writing into separate zero-initialized buffers), merging makes them alias the same buffer and corrupt each other.
This PR fixes it by implementing MemoryEffectsOpInterface on AsyncDispatchOp. The new getEffects() reports MemoryEffects::Allocate when any result is a non-tied resource (i.e. a fresh buffer allocation). This is conservative and prevents CSE from merging such dispatches while still allowing CSE for dispatches where all results are tied.