Conversation
fbusato
left a comment
There was a problem hiding this comment.
I'm not sure if it is worth introducing a new class only to express the broadcast semantic.
The algorithm enum BLOCK_REDUCE_WARP_REDUCTIONS_NONDETERMINISTIC already ensures broadcast logic.
I would try to extend the logic in warp-based block reduction instead of relying on synchronization.
cccl/cub/cub/block/specializations/block_reduce_warp_reductions.cuh
Lines 158 to 170 in 2fcc3f0
The optimization for block_size=32 is nice but a bit narrow as it is more an edge case.
I also want to mention that a previous tentative for the same issue was made in #9322
okay |
|
Done, and the direction was right — no new primitive was needed.
One constraint worth flagging: the fold order has to be fixed (all threads start from Measured on L20 (sm89), driver 595.91.07, counting threads whose value differs from thread 0:
The total is correct in every configuration ( Also checked #9322 as suggested: it proposed Added the realistic measurement: a block-normalise kernel (every thread divides its own value by
That is parity — the workload is bandwidth bound (32 MB per iteration at roughly 730 GB/s), so the Regression on the same machine, against the refactored header: 120/120 shared-agent cases and 15/15 Cross-architecture check. Same measurement on an RTX 5090 (sm120, driver 580.82.07) against the
The counts depend only on the block/grid geometry, which is why two architectures and two |
…ions path Per review: no new primitive for the broadcast semantic, and no reliance on a synchronization pass. The algorithm already carries the contract, so the cross-warp step is extended instead. ApplyWarpAggregates used to fold the per-warp aggregates in thread 0 only, which is why callers that need the value everywhere had to publish to shared memory, barrier and reload. Folding in every thread, walking the published aggregates in one fixed order, leaves the block aggregate identical in all threads as a by-product: no broadcast pass, no separate class, and no block_size == 32 special case. The order has to be fixed rather than per-thread starting from the thread's own warp aggregate, otherwise different warps would accumulate floating point partials in different orders and the result would no longer be bit-identical across the block, which is the property BlockReduce guarantees. Every thread reads the same shared address per step, which the hardware broadcasts. Evidence, L20 (sm89) under driver 595.91.07, block sizes 32/64/128/256/512/1024, counting the threads whose value differs from thread 0: upstream 240/744/1760/3800/7888/16072, this change 0 across the board, with the total correct in every configuration. A launch-bound microbenchmark shows parity at BT=128/256 and 0.018-0.024 ms against 0.022-0.023 ms at BT=1024, so this is a correctness-of-contract change rather than a measured speedup.
5741b34 to
719e58a
Compare
The broadcast value shared ownership of a union slot with the reduction scratch (issue #3917), so
every participating thread had to re-read it once the reduction completed. Giving the broadcast its
own slot removes that second read, and a single-warp block can skip the cross-warp reduction
entirely, reducing through
WarpReduceplusShuffleIndex<32>.Multi-warp blocks keep the existing raking path, so only the single-warp configuration changes
behaviour. Measured on an L20 (sm89, CUDA 12.8), ABBA-paired against the unpatched block primitive:
1.80x for a single-warp block, parity for multi-warp blocks. Correctness: 15/15
BlockAllReducecases plus the shared-agent regression suite.
Rework in progress (per review)
@fbusato's review asks for a different shape than the one currently pushed, and the rework is the
next commit rather than a tweak:
BLOCK_REDUCE_WARP_REDUCTIONS_NONDETERMINISTICalready carries the broadcast contract, so nothing new is needed to express it;
block/specializations/block_reduce_warp_reductions.cuhso everythread folds in
temp_storage.warp_aggregates[warp_idx]as it is produced, instead of thread 0assembling the total and every thread re-reading it afterwards;
block_size == 32case.Related: #9322 proposed
BlockReduceBroadcast, i.e. a new primitive for the same semantics, and wasclosed without prejudice after passing CI. This branch deliberately does not re-land that shape.
Measurement environment
The numbers above were taken on an L20 (sm89) with driver 570.86.10 / CUDA 12.8. The machine has
since been upgraded to driver 595.91.07, so the figures are being re-measured and this section
will carry the updated table; treat the current numbers as driver-570 references.