Skip to content

Fix MIGraphX EP decode nondeterminism from unsynchronized output reads - #86

Open
aditya-dl wants to merge 3 commits into
onnxruntime:mainfrom
aditya-dl:fix/migraphx-decode-stream-ordering
Open

Fix MIGraphX EP decode nondeterminism from unsynchronized output reads#86
aditya-dl wants to merge 3 commits into
onnxruntime:mainfrom
aditya-dl:fix/migraphx-decode-stream-ordering

Conversation

@aditya-dl

@aditya-dl aditya-dl commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator

Greedy decode through the MIGraphX EP returns different token ids on every iteration — same model, same params, same process. ep.9D is stable on the same model and script.

2f6c1d6 removed the per-Compute hipStreamSynchronize calls, on the basis that ORT flushes the compute stream at run end via DeviceStreamCollection::CleanUp -> SyncStream::Flush.

That flush does happen, but after the outputs have already been read — CopyOutputsAcrossDevices runs inside ExecuteGraphImpl (inference_session.cc:3231), while OnRunEnd and CleanUp are at :3244 and :3260. And because our compute stream is created hipStreamNonBlocking (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:

  • Reinstate hipStreamSynchronize(hip_stream) at the end of the three Compute paths. Only that hunk of 2f6c1d6 is undone — the staging and coalesce-IO work stays, and the OnRunEnd sync it dropped is correctly left out, since stream_ never carried compute work.
  • Make the D2H branch of hip::DataTransfer::CopyTensors honour 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 bypasses DataTransfer entirely.

Verified, 5 iterations each, warm cache, greedy:

Model Tokens Result
DeepSeek-1L 8 byte-identical to ep.9D, zero divergence
DeepSeek-R1-Distill-Qwen-1.5B_fp16 16 byte-identical to ep.9D, zero divergence

Also 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/*.cc builds into migraphx-backend.dll, not amdgpu-ep.dll — both need deploying to test this.

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.
@aditya-dl
aditya-dl requested a review from tperry-amd August 13, 2026 00:02
@aditya-dl

Copy link
Copy Markdown
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants