[BugFix][Argreduce] Fix MACA tiled argmax/argmin always returning index 0 - #53
Open
Lfan-ke wants to merge 3 commits into
Open
[BugFix][Argreduce] Fix MACA tiled argmax/argmin always returning index 0#53Lfan-ke wants to merge 3 commits into
Lfan-ke wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request corrects the execution order in the argreduce MACA kernel to ensure that out_idx is updated before row_extreme is overwritten, resolving a bug where the index was evaluated against the updated extreme value instead of the previous one. As there are no review comments, I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Author
|
@codex review please. read code at pr and repositories. |
Signed-off-by: 林晨 (Leo Cheng) <chengkelfan@qq.com>
Signed-off-by: 林晨 (Leo Cheng) <chengkelfan@qq.com>
Lfan-ke
force-pushed
the
fix/argreduce-maca-tile-merge
branch
from
August 6, 2026 13:36
dcb32bf to
b04651d
Compare
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.
Problem
The MACA tiled argmax/argmin always returns
0.argreduce_maca.pyexists because MACA gives a block 64 KiB of shared memory, so wide rows have to be reduced tile by tile. The per-tile extremum and its index are computed correctly; the bug is in the merge step that folds each tile into the running row extremum:row_extreme[i]is written first, so by the timeout_idxis decided,row_extreme[i] >= tile_extreme[i]holds by construction and the predicate is always false.out_idxis therefore never written and keeps the0it was filled with. Both the argmax branch and the argmin branch have it.Only the tiled path is affected, and only MACA reaches it — the single-tile kernel scans the whole row in one shared buffer and is fine, and on an NVIDIA card the larger shared-memory budget keeps almost everything single-tile. So this is a silent wrong answer confined to MetaX, on the wide-row argmax the manifest declares (lm-head).
Fix
Decide
out_idxagainst the previous row extremum, i.e. write it beforerow_extremeis overwritten. Two statements swapped, in each of the two branches. The comparison stays strict (>/<), which is what keeps the leftmost-index semantics thattorch.argmax/torch.argminalso have.Test node delta
The four new cells are regression coverage for this defect. They are the only cases in the file that reach the tiled path at all: with a 64 KiB budget and a per-column cost of
elem_bytes + 4,compute_tile_nkeeps everything belowN ≈ 11008(fp16 / bf16) on the single-tile path, and the widest existing fixture isN = 4096. So the patched lines had no coverage whatsoever — which is how "always returns index 0" shipped in the first place. The test assertskernel.config["tile_n"] > 0so it cannot silently regress back to the single-tile path, and it puts the extremum in the last tile so a merge that never writesout_idxis visible rather than accidentally right. It isskipif(not is_maca()), since the tiled kernel is MACA-only.Test Result
MetaX C500, MACA 3.5.3.20.
On
dev, the new cases fail:With this patch:
Whole file, with this patch:
ruff check --config pyproject.tomlis clean on both files.One thing this PR does not fix
float32atN = 16384pickstile_n = 8192, i.e.8192 × (4 + 4) = 65536bytes, exactlydevice_smem_budget(), and the launch is rejected:The same kernel launches at
tile_n = 8064(64512 bytes), andsoftmaxatN = 32768requests exactly 65536 bytes (2 × 16384 × 2) yet launches and passes today. Survival at exactlydevice_smem_budget()thus depends on the kernel, and the budget sits on the edge by construction:compute_tile_nreturnstile_n_max, consuming the budget to the byte, wheneverN_paddedis a multiple of it, as every power-of-two hidden size is. Sweeping 256-alignedN_paddedup to 40960 for argreduce fp32, 71 of 128 tiled cases land exactly on 65536.I have not isolated the cause, so I am not moving the budget here. The device reports
sharedMemPerBlock = 65536with no reserve, and a baremxcckernel launches at 65536, as does the driver API, so the shortfall is not a hardware limit. This is separate from the merge bug and predates it; the new cases use fp16 / bf16 to stay clear. Happy to follow up once the cause is known.