Skip to content

Upstream sync 13/N: merge 1ab2801dde [ROCm] Remove stale SDPA and skinny GEMM workarounds (#50907) (conflict) - #1260

Merged
roberteg16 merged 4 commits into
rogarcia.merge-upstream-108from
rogarcia.merge-upstream-109
Sep 9, 2026
Merged

roberteg16 merged 4 commits into
rogarcia.merge-upstream-108from
rogarcia.merge-upstream-109

Conversation

@roberteg16

@roberteg16 roberteg16 commented Sep 7, 2026

Copy link
Copy Markdown

Context

Thirteenth step of the batched upstream catch-up. Stacked on #<batch 12 PR>.

Conflict-only step.

Merged upstream changes:

  1. 1ab2801dde [ROCm] Remove stale SDPA and skinny GEMM workarounds vllm-project/vllm#50907 — [ROCm] Remove stale SDPA and skinny GEMM workarounds

One conflict, in vllm/model_executor/layers/utils.py, at the wvSplitKrc skinny-GEMM call. Both sides are AMD-authored, which is exactly why this needed reading the kernel rather than trusting either side.

ours:     x_view = x.reshape(-1, x.size(-1))
          with record_function_or_nullcontext(f'wvSplitKrc {n}x{m}x{k}'):
              out = ops.wvSplitKrc(weight, x_view, cu_count, bias)
          return out.reshape(*x.shape[:-1], weight.shape[0])
theirs:   x_view = x.reshape(-1, x.size(-1)).contiguous()
          return ops.wvSplitKrc(x_view, weight, cu_count, bias)
resolved: x_view = x.reshape(-1, x.size(-1)).contiguous()        <- theirs
          with record_function_or_nullcontext(f'wvSplitKrc {n}x{m}x{k}'):
              out = ops.wvSplitKrc(x_view, weight, cu_count, bias)  <- theirs' arg order
          return out.reshape(*x.shape[:-1], weight.shape[0])     <- ours

Three independent differences, resolved separately — two to theirs, one to ours.

Follow-up fix in this branch (f94abd3f06). The weight.is_contiguous()
gate this commit adds to use_skinny disqualifies every weight that
pad_weights_avoid_cache_cliff_on_gfx11 has padded — that pad changes
stride(0) only, on purpose — dropping those layers onto the fallback GEMM.
Measured cost: −3.3% decode on bench2 (83.1 → 80.2 tok/s), bisected to this
batch with the per-PR CI wheels. Relaxed to weight.stride(-1) == 1, which is
what the kernel actually requires: the host captures Kap/Kbp = stride(0) and
the kernel indexes B_[row * Kbp] rather than assuming K. Recovers the full
83.1 tok/s; correctness checked against torch.nn.functional.linear with
padded weights (max |Δ| 0.0039). Details in the
sweep comment.

Audit

1. Argument order — took theirs. This is a real fork bug being fixed.

The two skinny kernels use opposite conventions, so the fork's ordering cannot be justified by analogy with the wvSplitK(weight, x_view, …) call 60 lines below:

csrc/rocm/skinny_gemms.cu:1261  wvSplitK:   M_in = in_a.size(0)   <- weight 1st
csrc/rocm/skinny_gemms.cu:2286  wvSplitKrc: M_in = in_b.size(0)   <- weight 2nd
                                            N_in = in_a.size(0)
                                            K_in = in_b.size(1)

wvSplitKrc reads M and K off its second argument, so the second argument must be the [m, k] weight and the first the activation. The fork passed (weight, x_view), which transposes the problem: M_in would be the token count and K_in the weight's second dim.

2. .contiguous() — took theirs. Upstream's own comment on the sibling call explains it: a shape-preserving reshape can retain a transposed activation's non-contiguous strides, and the skinny kernels assume contiguous K.

3. Profiler label and output reshape — kept ours. The record_function wrapper is on every other GEMM path in this file (DOT, wvSplitK, LLMM1, BLAS — lines 300–348); dropping it here alone would leave one unlabelled path in the profile. The reshape is not redundant either: the kernel allocates out_c as 2-D {N_in, M_in} (skinny_gemms.cu:2308), so for x with more than two dims upstream's bare return hands back a 2-D tensor where the caller expects [*x.shape[:-1], m].

Reachability, stated plainly: this path is gated on on_gfx950(), so it is dead code on gfx1151 (Strix Halo) and the fix cannot be exercised by this fork's benchmarks. Resolved for correctness on gfx950 rather than left to rot; a gfx950 owner should confirm.

py_compile, ruff check and ruff format pass on the merged file.

Merge commit only — do not squash or rebase.

AI assistance was used to prepare this merge.

Test plan

  • Argument order verified against the kernel source, not by analogy
  • Output-shape contract checked against the kernel's out_c allocation
  • py_compile + ruff check + ruff format
  • Build + 6-model benchmark sweep vs the Strix Halo dashboard — no regression on transformers 5.15.0 (results)

mgoin and others added 3 commits August 11, 2026 14:19
)

Signed-off-by: Andreas Karatzas <Andreas.Karatzas@amd.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
Co-authored-by: OpenAI Codex <codex@openai.com>
… GEMM workarounds (vllm-project#50907) (conflict)

Conflict-only step. One conflict, in vllm/model_executor/layers/utils.py, in the
wvSplitKrc skinny-GEMM call. Both sides are AMD-authored, which is why this
needed reading the kernel rather than trusting either.

  ours:   x_view = x.reshape(-1, x.size(-1))
          with record_function_or_nullcontext(f'wvSplitKrc {n}x{m}x{k}'):
              out = ops.wvSplitKrc(weight, x_view, cu_count, bias)
          return out.reshape(*x.shape[:-1], weight.shape[0])
  theirs: x_view = x.reshape(-1, x.size(-1)).contiguous()
          return ops.wvSplitKrc(x_view, weight, cu_count, bias)

Three independent differences, resolved separately by content:

1. ARGUMENT ORDER - took theirs, and this is a real fork bug being fixed.
   The two skinny kernels use OPPOSITE conventions, so the fork's ordering
   cannot be justified by analogy with the wvSplitK call 60 lines below:
     csrc/rocm/skinny_gemms.cu:1261  wvSplitK:   M_in = in_a.size(0)  <- weight 1st
     csrc/rocm/skinny_gemms.cu:2286  wvSplitKrc: M_in = in_b.size(0)  <- weight 2nd
                                                 N_in = in_a.size(0)
                                                 K_in = in_b.size(1)
   wvSplitKrc reads M and K off its *second* argument, so the second argument
   must be the [m, k] weight and the first the activation. The fork passed
   (weight, x_view), which transposes the problem: M_in would be the token
   count and K_in the weight's second dim. Upstream's (x_view, weight) is
   correct against the kernel as it exists in this tree.

2. .contiguous() - took theirs. Upstream's own comment on the sibling call
   explains it: a shape-preserving reshape can retain a transposed
   activation's non-contiguous strides, and the skinny kernels assume
   contiguous K. The fork lacked it.

3. record_function label and the output reshape - kept ours. The profiler
   wrapper is used on every other GEMM path in this file (DOT, wvSplitK,
   LLMM1, BLAS - lines 300..348), so dropping it here alone would leave one
   unlabelled path in the profile. The reshape is not redundant either: the
   kernel allocates out_c as 2D {N_in, M_in} (skinny_gemms.cu:2308), so for
   x with more than two dims upstream's bare return would hand back a 2D
   tensor where the caller expects [*x.shape[:-1], m]. Keeping both is the
   union, not a preference.

Note on reachability: this path is gated on on_gfx950(), so it is dead code on
gfx1151 (Strix Halo) and the fix cannot be exercised by this fork's benchmarks.
Resolved for correctness on gfx950 rather than left to rot.

py_compile, ruff check and ruff format pass on the merged file.
Batch 13 (upstream 1ab2801) added weight.is_contiguous() to the use_skinny
gate. That disqualifies every weight pad_weights_avoid_cache_cliff_on_gfx11 has
padded -- the pad changes stride(0) only, to dodge the gfx11x L2/MALL
channel-hash cliff -- and drops those layers onto the fallback GEMM.

Measured on bench2 (Qwen3.6-35B-A3B W4A16, int8:g32 lm-head), the benchmark that
exercises this path end to end:

  wheel -107 (pre-batch-13)        83.1 tok/s
  wheel -117 (batch 13 present)    80.2 tok/s   -3.3%
  wheel -117 + this change         83.1 tok/s   recovered

Runtime trace: 1760 of 4150 calls had weight.is_contiguous() == False, and every
one of them has stride(-1) == 1, so all of them are eligible again.

What the kernel actually needs is K contiguous, not the whole tensor. The host
captures Kap/Kbp = stride(0) (skinny_gemms.cu:1264) and the kernel indexes
B_[row * Kbp] and s[k_ + Kap * n] rather than assuming K; the LDS-fit check in
the launcher is sized with Kbp too. Correctness verified against
torch.nn.functional.linear with padded weights across four shapes: max |delta|
0.0039, i.e. fp16 accumulation noise.
@roberteg16
roberteg16 marked this pull request as ready for review September 8, 2026 08:10
@roberteg16
roberteg16 requested review from eble-amd and removed request for AndreasKaratzas September 8, 2026 08:10
@roberteg16
roberteg16 added this pull request to stack #1286 September 9, 2026 15:30
@roberteg16
roberteg16 merged commit 0d01387 into gfx11 Sep 9, 2026
8 of 9 checks passed
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