Fix broadcast wrong output for src_partition != 0 and add single-tile guard#7
Open
RuifengHua wants to merge 1 commit into
Open
Conversation
Commit c38d790 ("NKI Lib 2026-04-13") added Broadcast.execute() with a shuffle mask built as [src_partition] * 32, while the source is also sliced to that row via slice(dim=0, start=src_partition, end=src_partition+1). This resulted in wrong output for any src_partition != 0: within the sliced view the target row is already at index 0, but nc_stream_shuffle's shuffle_mask[i] selects which source partition (of the sliced view) output row i copies from, so passing src_partition double-applies the offset. (src_partition == 0 happened to work, which is why this stayed latent.) Fix by building the mask as [0] * 32. Also add a single-tile guard: broadcast spreads one per-feature row across <= pmax rows, so a multi-tile stream is rejected with an actionable message rather than silently doing an unintended per-tile broadcast. Add an integration test suite covering non-zero src_partition and the guard.
|
Thanks for your contribution! I've reached out to our team to review your commit and we will get back to you shortly with any follow-up. |
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.
Summary
Broadcastreplicates one source partition row to every row of the destination;src_partitionselects which row of the source tile is broadcast.Broadcast.execute()slices the source down to the chosen row:so within that sliced view the target row is at index
0. It then built the shuffle mask as:nc_stream_shuffle'sshuffle_mask[i]selects which source partition (within the quadrant of the already-sliced view) output rowicopies from. Because the slice has already applied thesrc_partitionoffset, the mask must be0— usingsrc_partitiondouble-applies the offset and produces wrong output for anysrc_partition != 0. (src_partition == 0happens to work, which is why this stayed latent — no in-tree caller uses a non-zero value.)This PR:
[0] * 32;<= pmaxrows, so a multi-tile stream is rejected with an actionable message instead of silently doing an unintended per-tile broadcast (the correct pattern for a larger destination is to broadcast once into a<= pmaxbuffer and reuse it across the tile loop, as the in-tree callers do);Repro (before fix)
Broadcasting source row
2to all rows should copysrc[2]into every output row. Before the fix, a non-zerosrc_partitionreads out of the sliced view and yields wrong output (nanhere):Test plan
Added
test/integration/nkilib/experimental/primitives/blas/test_broadcast.py, verified in simulation mode (--test-mode simulation):P, free dimF, dtype (float32,bfloat16), source tile row countSRC_ROWS, and broadcast rowSP(filtered toSP < SRC_ROWS), including non-zerosrc_partitionsrc_partitioncases fail against the pre-fix mask and pass after the fix;src_partition == 0passes both before and afterblas.Broadcast): a valid single-tile broadcast passes at the edges (notablyP = pmax, the boundary just below where the guard rejects), and a multi-tile stream (P > pmax) is rejected with the guard messageNote: validated in simulation only (correctness fix, no performance claim). Broadcast is an exact copy, so tolerances are
0.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.