Fix MIGraphX EP decode nondeterminism from unsynchronized output reads - #86
Open
aditya-dl wants to merge 3 commits into
Open
Fix MIGraphX EP decode nondeterminism from unsynchronized output reads#86aditya-dl wants to merge 3 commits into
aditya-dl wants to merge 3 commits into
Conversation
2f6c1d6 ("MIGraphX ROCm 7.14 feature port") removed the per-Compute hipStreamSynchronize(hip_stream) calls in all three Compute() paths (staging, and both eager paths), reasoning that ORT's own DeviceStreamCollection::CleanUp -> SyncStream::Flush already synchronizes the compute stream at run end. That flush does happen, but too late: ORT's CopyOutputsAcrossDevices (the D2H copy that fetches decode outputs/logits) runs via a blocking hipMemcpy on the null/legacy stream, before OnRunEnd/CleanUp are called. Our compute stream is created with hipStreamNonBlocking (shared/hip/stream_support.h), which is explicitly exempt from the null stream's implicit ordering guarantees. So the blocking hipMemcpy can read the output buffer while the GPU is still writing it. This reproduced as OGA token-id nondeterminism under greedy decode via the MIGraphX EP - present on this branch's parent f0ffe5e and absent on ep.9D (gpuep-rel-2609), which still had these sync calls. Verified after the fix (5 iterations each, warm cache, greedy): DeepSeek-1L 8 tokens -> byte-identical to ep.9D, 0 divergence DeepSeek-1.5B 16 tokens -> byte-identical to ep.9D, 0 divergence Throughput cost is within run-to-run noise, so the ~10% the removal appeared to buy was skipped correctness, not real throughput: 1L 9D 407.3 tk/s (mean of 3) vs fixed 402.5 (mean of 4) 1.5B 9D 124.9 tk/s vs fixed 128.5 The env-gated event-based deferred sync on main (e5ad9cb) is therefore not needed to recover performance here. Note for anyone reproducing: this file builds into migraphx-backend.dll (src/migraphx/CMakeLists.txt:78 renames the migraphx-ep target), NOT amdgpu-ep.dll. Deploying only amdgpu-ep.dll runs the old code and makes this fix look ineffective. Independent of the DataTransfer backend-routing fix in 1062ace - this is a stream-ordering bug, not a backend-resolution bug.
CopyTensors computes stream_handle from the OrtSyncStream ORT passes in (data_transfer.cc:43-45) and honors it on the D2D path, but the D2H branch discarded it and issued a plain hipMemcpy on the legacy/null stream. The EP's compute stream is created with hipStreamNonBlocking (src/shared/hip/stream_support.h:15), which is explicitly exempt from the null stream's implicit cross-stream ordering. So a D2H read of an output buffer on the null stream is unordered against kernels still writing it on the compute stream. For fused-node output fetches ORT does hand us the producer stream (utils.cc -> data_transfer_manager.cc -> plugin_data_transfer.cc), so this is a live gap, not a theoretical one. Use hipMemcpyWithStream (== hipMemcpyAsync + hipStreamSynchronize on that stream) when a stream is supplied; keep the plain blocking hipMemcpy when it is null. Mirrors the in-tree MIGraphX EP's gpu_data_transfer.cc. This is hardening, NOT a replacement for the per-Compute sync in 5ce17e7. ORT hardcodes a null stream on the single-pair plugin path (plugin_data_transfer.cc:61), and the EP's staging D2H (CopyStagingOutputsToOrt) bypasses DataTransfer entirely, so this alone does not cover every output path. It is, however, the prerequisite for ever removing the per-Compute syncs to reclaim CPU/GPU overlap. Verified (both dlls deployed -- see 5ce17e7 on the two-dll gotcha): - DeepSeek-1L, 8 tok, 5 iters: [69492, 79, 59417, 84094, 94258, 241, 80551, 3145] -- byte-identical to 9D, zero divergence warnings. 402.8 tk/s p50 (unchanged vs 402.5 pre-change). - DeepSeek-R1-Distill-Qwen-1.5B_fp16, 16 tok, 5 iters: byte-identical to 9D. 128.4 tk/s p50. - Concurrent two-backend repro: both sessions run, no data_transfer.cc:73 failure, both predict class 446 @ logit 7.16016.
Collaborator
Author
|
@tperry-amd could you take a look at this here? This is a gating issue for 10D since it affects all LLM workloads through OGA. I would also like your input in terms of is there a better solution that I am missing. |
Zhaeong
approved these changes
Aug 13, 2026
tperry-amd
approved these changes
Aug 13, 2026
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
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.
Greedy decode through the MIGraphX EP returns different token ids on every iteration — same model, same params, same process.
ep.9Dis stable on the same model and script.2f6c1d6 removed the per-
ComputehipStreamSynchronizecalls, on the basis that ORT flushes the compute stream at run end viaDeviceStreamCollection::CleanUp -> SyncStream::Flush.That flush does happen, but after the outputs have already been read —
CopyOutputsAcrossDevicesruns insideExecuteGraphImpl(inference_session.cc:3231), whileOnRunEndandCleanUpare at:3244and:3260. And because our compute stream is createdhipStreamNonBlocking(shared/hip/stream_support.h), it is exempt from the null stream's implicit ordering — so nothing prevents the fetch from reading logits while kernels are still writing them.Two commits:
hipStreamSynchronize(hip_stream)at the end of the threeComputepaths. Only that hunk of 2f6c1d6 is undone — the staging and coalesce-IO work stays, and theOnRunEndsync it dropped is correctly left out, sincestream_never carried compute work.hip::DataTransfer::CopyTensorshonour the stream ORT supplies (hipMemcpyWithStream), matching the D2D branch and the in-tree MIGraphX EP. Hardening, not a replacement: ORT passes a null stream on the single-pair path, and the staging path bypassesDataTransferentirely.Verified, 5 iterations each, warm cache, greedy:
ep.9D, zero divergenceep.9D, zero divergenceAlso re-ran the two-session concurrent repro: both sessions run, no failure at
data_transfer.cc:73.Throughput is within run-to-run noise on both models, so the apparent gain from removing the syncs was skipped correctness. The env-gated deferred-sync experiment on main (e5ad9cb) is not needed here, and would not fix this in any case: it waits in
OnRunEnd, which is on the wrong side of the output read.Note
src/migraphx/*.ccbuilds intomigraphx-backend.dll, notamdgpu-ep.dll— both need deploying to test this.