fix(bufid_sync): make optimizeSamePipeMerge deterministic - #1304
Open
Adamkh329 wants to merge 1 commit into
Open
fix(bufid_sync): make optimizeSamePipeMerge deterministic#1304Adamkh329 wants to merge 1 commit into
Adamkh329 wants to merge 1 commit into
Conversation
`--enable-bufid_sync` emitted DIFFERENT CODE on repeated runs of the same
binary over the same input.
`optimizeSamePipeMerge` iterated `op2BufSync_`, a `DenseMap<Operation *, ...>`,
so visit order followed the ops' POINTER VALUES. The survivor of a merge group
is `*std::min_element(ids)` over whichever ids the first-visited op happens to
group, recorded first-writer-wins, so a different visit order picks a different
survivor and emits different code.
MEASURED before, on Qwen3DecodeA5/rope_kv_cache (a5, level3), six runs each
pinned with `taskset -c 0`:
get_buf count: 24 24 24 23 23 23
The exact sequence is NOT itself reproducible -- it cannot be, since that is
the defect. A later eight-run repeat of the same command gave
24 24 24 24 23 24 24 24. What reproduces is the VARIANCE, not the pattern.
`mergeMap` SIZE was stable at 4 every run; the TARGETS drifted -- 9->2 / 9->6 /
9->8 and 10->3 / 10->2 / 10->6, with only 3->0 and 8->3 stable. Pinning to one
CPU rules out a thread race: it is bucket order.
Ordering the ops by their earliest bracket `syncIRIndex`, tie-broken by smallest
`logicId`, is stable across runs because both come from the SyncIR rather than
from the heap. Only the two loops that read `op2BufSync_` needed it; the
int-keyed maps inside were already deterministic given a deterministic outer
order, and the final rewrite loop is order-independent because it writes a fresh
map.
This restores what docs/bufid_sync_a5_design.md:300 requires, and it matters
beyond reproducible builds: every A/B measurement previously taken through this
pass carried an unattributed run-to-run variance of about one get_buf per
kernel.
The unified allocator never calls this pass (it is the standalone
BufidSyncPass's only caller), so that path was already deterministic and is
unaffected.
Verified: rope_kv_cache emits a byte-identical kernel across 8 pinned runs; the
full corpus -- 101 kernels that compile with --enable-bufid_sync, 3 reps each
with the CPU pin VARIED between reps to be harsher than the original repro --
reports 0 non-deterministic kernels; lit 1664 passed / 0 failed; ctest 50/50.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
--enable-bufid_syncemitted different code on repeated runs of the same binaryover the same input.
Cause
optimizeSamePipeMergeiteratedop2BufSync_, aDenseMap<Operation *, ...>, sovisit order followed pointer values. The merge survivor is
*std::min_element(ids)over whichever ids the first-visited op groups, recorded first-writer-wins — so a
different visit order picks a different survivor and emits different code.
Evidence
Eight runs of the same command, each pinned with
taskset -c 0to rule out athread race:
get_bufmergeMapsize was stable while its targets drifted (9→2 / 9→6 / 9→8, 10→3 /10→2 / 10→6). Pinning rules out concurrency: it is bucket order.
The exact sequence is deliberately not presented as reproducible — it cannot be,
since that is the defect. What reproduces is the variance, not the pattern.
Fix
Order ops by earliest bracket
syncIRIndex, tie-broken by smallestlogicId—both come from the SyncIR rather than the heap, so the order is stable across
runs. Only the two loops reading
op2BufSync_needed it: the int-keyed mapsinside were already deterministic given a deterministic outer order, and the
final rewrite loop is order-independent because it writes a fresh map.
Why it matters beyond reproducible builds
Every A/B measurement previously taken through this pass carried roughly one
get_bufper kernel of unattributed run-to-run variance. Any buffer-idcomparison predating this fix should be treated as carrying that noise.
This also restores what
docs/bufid_sync_a5_design.md:300already requires:"The traversal order should be deterministic so generated IDs are stable."
Verification
BufidSyncPass.cpp:78is the onlycaller), so that path was already deterministic and is unaffected