Skip to content

[MHA] Fix -DONLY_FAV3=0 quoting that breaks the backward JIT build - #6616

Merged
zufayu merged 1 commit into
ROCm:mainfrom
menglcai:menglcai/fix_donly_fav3
Oct 18, 2026
Merged

zufayu merged 1 commit into
ROCm:mainfrom
menglcai:menglcai/fix_donly_fav3

Conversation

@menglcai

Copy link
Copy Markdown
Contributor

Motivation

Split out from #4306, with none of the CK backward/PREBUILD changes from that PR.

cmdGenFunc_mha_bwd and cmdGenFunc_mha_varlen_bwd return "flags_extra_cc": ["'-DONLY_FAV3=0'"]. That value reaches the compiler through extra_cflags, which is passed as an argv list, so the quotes are part of the flag rather than shell syntax and the compiler reads ''-DONLY_FAV3=0'' as a filename.

Linux never sees this, because ninja runs each command through /bin/sh -c and the shell strips the quotes. Windows uses CreateProcess with no shell, so they survive and the backward module fails to compile:

clang: error: no such file or directory: ''-DONLY_FAV3=0''

Technical Details

The flag is written into build.ninja the same way on both platforms; only the execution differs, which is why the bug is invisible on Linux.

The change drops the quotes at the two sites that carry this flag:

-        "flags_extra_cc": ["'-DONLY_FAV3=0'"],
+        "flags_extra_cc": ["-DONLY_FAV3=0"],

ONLY_FAV3 only appears on the two backward generators, so nothing else is affected. On Linux the change is a no-op, since the shell was already removing the quotes.

Test Plan

Four machines, covering both operating systems and two GPU architectures:

host OS GPU ROCm / torch
AMD Ryzen™ AI Max+ 395 Ubuntu 24.04 gfx1151 torch 2.13.0+ROCm 7.15
AMD Ryzen™ AI Max+ 395 Windows 11 25H2 gfx1151 torch 2.14.0+ROCm 10.2.0
AMD Radeon™ AI PRO R9700 Windows 11 25H2 gfx1201 torch 2.14.0+ROCm 10.2.0
AMD Radeon™ AI PRO R9700 Ubuntu 24.04 gfx1201 torch 2.14.0+ROCm 10.1

Three checks:

  1. Compiler behaviour, no shell involved. Calls the HIP compiler directly so the quoting can be judged on its own, independently of platform:
import subprocess, shutil, tempfile, os
cc = shutil.which("hipcc") or shutil.which("clang++")
d = tempfile.mkdtemp(); src = os.path.join(d, "t.cpp")
open(src, "w").write("int main(){return ONLY_FAV3;}\n")
for flag in ["-DONLY_FAV3=0", "'-DONLY_FAV3=0'"]:
    r = subprocess.run([cc, flag, "-c", src, "-o", d + "/t.o"], capture_output=True, text=True)
    print(repr(flag), "rc =", r.returncode, r.stderr.strip()[:80])
  1. mha_bwd JIT build, before and after, on Windows and Linux. The op is called directly rather than through test_mha.py. On a non-gfx9 host that test stops at the forward step with invalid argument for fmha_fwd, because ck-tile's generate.py defaults to --targets gfx9,gfx950 and emits no kernel the runtime arch can dispatch, so it never reaches the backward path this change affects.

  2. Linux regression check. Full test_mha.py run with and without the change, to confirm it is a no-op there.

Test Result

  1. Compiler behaviour — identical on all four hosts, regardless of OS, GPU or ROCm version:
'-DONLY_FAV3=0'    rc = 0
"'-DONLY_FAV3=0'"  rc = 1  clang++: error: no such file or directory: ''-DONLY_FAV3=0''
  1. mha_bwd JIT build:
host before after
AMD Ryzen™ AI Max+ 395 (Windows, gfx1151) fails, all 166 translation units succeeds
AMD Ryzen™ AI Max+ 395 (Ubuntu, gfx1151) succeeds succeeds
AMD Radeon™ AI PRO R9700 (Windows, gfx1201) fails, 25 translation units report the error succeeds
AMD Radeon™ AI PRO R9700 (Ubuntu, gfx1201) succeeds succeeds

The split follows the operating system, not the GPU: both Windows hosts fail and both Linux hosts build either way, which is what the shell difference predicts.

  1. Linux regression check: test_mha.py gives identical numerics either way (Output and dQ max diff 0.0009765625 in both).

Scope

This is a build-correctness fix and does not enable anything on its own: backward on a non-gfx9 host still fails to dispatch afterwards, for the same CK target reason noted above. That is out of scope here per the decision on #4306.

Submission Checklist

`flags_extra_cc` reaches the compiler through `extra_cflags`, which is passed as an argv list, so quoting an element makes the quotes part of the flag rather than shell syntax. ninja hides this on Linux because it runs each command through `/bin/sh -c`, which strips them, but on Windows it uses CreateProcess with no shell, so clang receives them literally and every translation unit of the backward module fails:

  clang: error: no such file or directory: ''-DONLY_FAV3=0''

The flag only appears on cmdGenFunc_mha_bwd and cmdGenFunc_mha_varlen_bwd, so the breakage is confined to the backward JIT build.

Verified on gfx1151. Windows (25H2, TheRock SDK, torch 2.14.0+rocm10.2.0): the mha_bwd JIT build fails on all 166 translation units before the change and completes in 132.6s after it. Linux (Ubuntu 24.04, ROCm 7.15, torch 2.13.0): the module builds either way and test_mha.py reports identical numerics, as expected given the shell. A direct hipcc call reproduces the failure without a shell on Linux too: argv `-DONLY_FAV3=0` returns 0, argv `'-DONLY_FAV3=0'` returns 1 with the same error.

This is a build-correctness fix and does not by itself enable anything new.

Signed-off-by: menglcai <menglcai@amd.com>
@menglcai
menglcai requested a review from a team October 16, 2026 08:40
@github-actions

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every PR:

  • ✅ Pre-checks (submodule verification, code formatting)
  • ✅ Aiter op tests (gfx942 + gfx950)
  • ✅ Triton tests on MI35X (only when aiter/ops/triton/** or related paths are changed)

Extended tests (opt-in via labels):

Label Tests
ci:gfx1250-ffm-triton Run the five-shard gfx1250 FFM Triton test suite
ci:triton-300x Run an additional Triton test job on MI300X in PRs (added automatically when gfx942 configs change); main branch always runs both MI35X and MI300X
ci:triton-355 Run the full Triton test suite on MI35X, not only the tests the change affects
ci:windows Windows JIT op smoke test on the gfx1201 ROCm runner
ci:multi-gpu Aiter multi-GPU tests on the 8-GPU runner
ci:sglang SGLang integration tests: DeepSeek-R1-MXFP4 accuracy, Qwen 3.5 accuracy
ci:atom ATOM benchmark: DeepSeek-R1-0528, GPT-OSS-120B
ci:atom_full ATOM accuracy suite for PR and main models from ATOM models_accuracy.json
ci:vllm vLLM benchmark: DeepSeek-V4-Pro, MiniMax-M3-MXFP4
ci:all All standard extended tests (excludes ci:atom_full)

Only add ci:atom_full for FlyDSL or Triton upgrades.
Add labels via the sidebar or gh pr edit 6616 --add-label <label>

One backend per PR:
A PR changes one kernel backend: [Triton/Gluon] (Triton and Gluon count as one), [HIP], [ASM], [CK], [OPUS] or [FlyDSL]. If the title ends up with two backend tags, split the PR -- as stacked pull requests when one part cannot merge without the other.

PR title tags & labels:
Component tags ([Triton/Gluon], [HIP], [CK], [ASM], ...) are added to the PR title and as PR labels automatically from the changed files and re-synced on every push — change-type tags like [fix]/[Perf], op tags like [MLA], and human labels (ci:*) are left untouched. Add the no-auto-title label to stop the title rewrites; labels stay in sync either way.

@zufayu zufayu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The quoting fix is correctly isolated from the Windows CK support work, and the full CI is green. LGTM.

@zufayu
zufayu merged commit 79634b1 into ROCm:main Oct 18, 2026
yuychang added a commit to yuychang/aiter that referenced this pull request Oct 19, 2026
…m#6355 and ROCm#6616

- fmha_kernels.py: keep main's new _check_out and this branch's _fp8_gfx950_reject_reason rename.
- mha.py: keep the try_flydsl descale routing and gate the FlyDSL route on ck_only_feature is None.
- That keeps main's exclusion of padded cu_seqlens and logits soft-cap for every varlen call, not only descaled ones.
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