Repository navigation
[HIP] fix(custom_all_reduce): uniform trip count for the block barriers - #5694
Conversation
Three kernels in this header stage their operands through shared memory and
bracket that with two block-wide __syncthreads(), inside a grid-stride loop
whose trip count is not uniform across the block:
cross_device_reduce_2stage stage 1
cross_device_reduce_2stage_write_mode stage 2
reduce_scatter_cross_device_store (fused allreduce+rmsnorm, first step)
The span each loop walks -- the rank's own partition, size/ngpus or
m*valid_pack_count/ngpus -- is not generally a multiple of the stride, so the
lanes covering the tail run one iteration more than the rest. The barriers then
pair up across different iterations, and against the ones in end_sync. Two
things follow: the reduction can read tmp_smem before the other wavefronts have
written their operands into it, and a wavefront can publish "stage 1 done" in
end_sync while another is still producing its output. The result is a wrong
reduction at a fixed relative position inside each rank partition -- observed as
four bad rows per batch, spaced by size/ngpus.
Rounding the loop bound up to a multiple of the stride makes the trip count
exactly uniform (tid < stride by construction); the work is predicated instead.
No added synchronisation and no change to the reduction order.
Repro on 4x MI355X, bf16 [bs, 2880], 32 shapes x 600 reductions each, against
RCCL all_reduce (bf16 rounding on a 4-way sum is ~0.05). Each reduction is also
run twice and compared bitwise, which needs no tolerance:
before 64 of 19200 wrong and non-deterministic, at bs = 120, 121, 127, 157,
160, 161; worst |diff| = 12.4
after 0 of 19200, worst |diff| = 0.0625
chained, 48 reductions back to back at bs=160:
before 300 of 11520 wrong, worst 13.1; after 0 of 11520
world_size=2 (1-stage path): 0 of 5120 either way, i.e. no regression
It is a race, so the counts move between runs -- an earlier sweep gave 43/19200
and 173/11520, over a slightly different set of shapes. Most shapes pass either
way: the race needs the partition remainder to land mid-block, which is why this
has survived.
reduce_scatter_cross_device_store is fixed by inspection -- same defect, same
file -- but the reproducer above does not exercise the fused-allreduce path.
Timing, 7 reps x 500 calls, median us/call taken as the max over ranks, whole
measurement repeated twice: within run-to-run noise at bs=120..161, and 3-6%
slower at bs=256 (19.2 -> 20.3) and bs=512 (32.2 -> 33.1).
🏷️ CI GuideRuns automatically on every PR:
Extended tests (opt-in via labels):
PR title tags & labels: |
6f55afa to
6fb1e0e
Compare
There was a problem hiding this comment.
- The change of device function start_sync and end_sync are unnecessary. detail see #4294 (comment)
- The change on kernels are truely useful, I have tested on main and this pr without change start_sync and end_sync(commit 6110f00), results as bellow. this can also prove origin start_sync and end_sync have no influence on accuracy
commit 6110f00

3368bc0 to
bf8bc55
Compare
hi @TennyWang1223 , thanks for the careful review, I've trimmed the corresponding section per requested. |
Comments only; every changed line starts with `//`, so the generated code is identical to 6110f00.
|
The new commit is syntax-only, only trim the comments and the code should function well; should be good to merge @TennyWang1223 |



What is wrong
Three kernels in
csrc/include/custom_all_reduce.cuhstage their operands through shared memoryand bracket that with two block-wide
__syncthreads()— inside a grid-stride loop whose tripcount is not uniform across the block:
cross_device_reduce_2stagecross_device_reduce_2stage_write_modereduce_scatter_cross_device_storeTaking the first as the example:
end - startis this rank's partition,size / ngpus, which is not generally a multiple ofstride = gridDim.x * (THREAD_NUM / ngpus). The lanes that cover the tail therefore run oneiteration more than the rest, so the barriers pair up across different iterations — and against
the ones in
end_sync. Two consequences:warp_id == 0reduction can readtmp_smembefore the other wavefronts have writtentheir operands into it;
end_syncand publish "stage 1 done" while another is still producingtmp_out, so the stage-2 all-gather can read a partition that is not finished.Either way the output is wrong at a fixed relative position inside each rank's partition.
How it shows up
sizeis flat, so each rank owns a contiguoussize / ngpusslice — one quarter of the rows ofa
[bs, hidden]tensor atngpus == 4. The damage lands at the same offset in every quarter:Found in an RL workload; reproduced standalone below.
Standalone reproduction
Four ranks on MI355X, bf16
[bs, 2880], 32 shapes × 600 reductions each. Every reduction is runtwice and compared bitwise (no tolerance needed), and also compared against RCCL's
all_reduce(bf16 rounding on a 4-way sum is ~0.05, so anything above that is a wrong answer rather than a
different summation order):
It is a race, so the counts move between runs; an earlier sweep over a slightly different set of
shapes gave 43/19200 and 173/11520. Most shapes pass either way — the race needs the partition
remainder to land mid-block, which is why this has survived.
The numbers above were measured on a build based on
d9e5ef7cethat also carried a secondcommit, since dropped, which widened two sync scopes. @TennyWang1223 re-measured
6110f00fon its own and reproduced the fix, which attributes it to the barrier change alone. The
three patched hunks are unchanged on
main, and the rebased diff is byte-identical to theone that was tested.
The fix
Round the loop bound up to a multiple of the stride, which makes the trip count exactly uniform
(
tid < strideby construction), and predicate the work. No added synchronisation and no changeto the reduction order.
reduce_scatter_cross_device_storeis fixed by inspection — same defect, same file — but thereproducer above does not exercise the fused-allreduce path.
Timing, 7 reps × 500 calls, median us/call taken as the max over ranks, whole measurement
repeated twice: within run-to-run noise at bs=120..161, and 3-6% slower at bs=256
(19.2 → 20.3) and bs=512 (32.2 → 33.1).