fix(BACKEND-ROCM): narrow silu(gate) to the gate dtype before multiply - #2957
Conversation
|
GPU gate run on this box (gfx1100, RX 7900 XTX, rocm-dev:10.0.0 container): test_ops_rocm_silu_rounding — 2/2 cases, 7058 assertions, all passed. One test revision was required by that run: the f32 legs asserted bit-equality between the device libdevice expf and the host expf, which differ by a few ULPs on arbitrary inputs; the f32 arms now compare within a documented 4-ULP band (UlpsApart helper, reason in a comment above the helper), while bf16/f16 keep the exact narrow-dtype equality the repair's contract is about. The narrowing itself is proven by the bf16 bit-exact legs. Head now 1230af2. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true |
970e871 to
1230af2
Compare
|
Device gate run on The fix, proved. Build clean under
Lines 185, 251 and 317 are That is the red-before evidence the change needed, on an AMD device, which no My gelu commit shipped a bad assertion, and the same run caught it. The It could not have been right in principle either — Removed in Re-queued on the corrected head; I will merge once that comes back. Sorry for the |
acd314d to
09df71a
Compare
Scope the independent BACKEND-ROCM repair extracted from PR mudler#2894: the ROCm SiluAndMul/MoeSiluMul kernels compute silu(gate) in f32 and multiply by up without first narrowing to the gate tensor's dtype, while the CPU oracle (cpu_ops.cpp:669,733) narrows via RoundThrough and upstream vLLM's silu_kernel (csrc/libtorch_stable/activation_kernels.cu:158) casts the intermediate to T before compute multiplies. On bf16 exact-equality checks the ROCm arm diverges. The spec records: both affected variants (SiluMulK in rocm_dense_basic.hip:99 and MoeSiluMulK in rocm_moe_router.hip:32), the upstream anchors with file:line, the NarrowTo<T> design, the self-skipping oracle-parity test plan, the gate and docker-compile evidence requirements, and the mudler#2889/mudler#1954 issue linkage. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:OMEN-ALPHA [OMP]
The ROCm SiluAndMul (dense, packed [T,2D]) and MoeSiluMul (MoE, separate gate/up) kernels computed silu(gate) in f32 and multiplied by up without first narrowing the silu result to the gate tensor's dtype. The CPU oracle (cpu_ops.cpp:669 SiluAndMulKernel, :733 MoeSiluMulKernel) narrows via RoundThrough(in_dt, ...) before the multiply, and upstream vLLM's silu_kernel (csrc/libtorch_stable/activation_kernels.cu:158 at e126687a9) casts the intermediate to T before compute (:36) and packed_compute (:72) multiply. On bf16 exact-equality checks the ROCm arm diverges -- issue mudler#1954 records the ROCm exactness failure, mudler#2889 names this as one of three pre-existing bugs. Fix: add a NarrowTo<T> device helper that round-trips an f32 value through the gate dtype (identity for f32, __float2bfloat16/__bfloat162float for bf16, __float2half/__half2float for f16), and insert it between silu and the multiply in both kernels: SiluMulK (rocm_dense_basic.hip:122): NarrowTo<Tin>(g / (1+expf(-g))) * up MoeSiluMulK (rocm_moe_router.hip:49): NarrowTo<Tg>(Silu(Ld(gate,i))) * Ld(up,i) The f32 path is NarrowTo<float> = identity, so f32-in/f32-out paths are bit-identical to before. The bf16 path now matches the CPU oracle exactly: both sides round silu(gate) to bf16 precision, then multiply by up in f32, then store. Also adds a focused self-skipping test (test_ops_rocm_silu_rounding.cpp) that runs both ops on the ROCm backend and compares against the CPU oracle with exact equality on every dtype arm (raw uint16 bits for bf16, exact f32 for f32) across multiple shapes. Registered in tests/CMakeLists.txt inside the if(VLLM_CPP_HIP) block, mirroring test_rocm_backend.cpp's guard pattern. Refs mudler#2889, mudler#1954. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:OMEN-ALPHA [OMP]
… the same file The silu fix here is right and it is the same defect twice more, three functions away. GeluMulK (packed, serving OpId::kGeluAndMul) and GeluMulSepK (separate gate/up) both compute 0.5f * g * (1 + tanhf(inner)) in f32 and multiply by up without narrowing, while the CPU oracle rounds the gelu intermediate through in_dt before its multiply exactly as it does the silu one (cpu_ops.cpp:644). So on bf16 the ROCm gelu-mul multiplies an f32 gelu where the oracle multiplies a bf16-rounded one, and the two disagree on exact equality for the reason this pull request already fixes for silu. GeluMulK is reachable through the public seam: GeluAndMulKernelRocm is registered for OpId::kGeluAndMul on kROCM, so vt::GeluAndMul on a bf16 tensor reaches it on a default configuration. Applies the NarrowTo<Tin> helper this change already introduces at both sites, and adds a GeluAndMul case to the new test on the same bounds as its SiluAndMul sibling: exact bytes on bf16, which is what the narrowing buys, and a 4-ULP band on f32 for the device-versus-host transcendental. Reverting either narrowing reds the bf16 arm of its case. Closes mudler#2966. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
…rted nothing and failed The device gate caught my own bad assertion. On gfx1151, with the narrowing fix correctly in place, the GeluAndMul case's f32 arm failed 14 assertions -- about 0.6% of elements -- against the 4-ULP band I copied from the SiluAndMul case without justifying it for gelu. The gelu polynomial amplifies the device tanhf versus host std::tanh divergence further than silu's expf does, so the bound was simply wrong. It also could not have been right in principle: NarrowTo<float> is the identity, so an f32 arm cannot distinguish this fix from its absence. It was measuring device-versus-host transcendental agreement, which this change does not touch, and the honest options were to widen a band until it passed -- picking a number to green an assertion that discriminates nothing -- or to remove it. Removed, with the measurement recorded in the case so nobody re-adds it. The bf16 arm, which is the one the narrowing actually buys, stays and is verified: same gfx1151 run, all three cases' bf16 exactness green with the fix, and all three red without it. Mutating NarrowTo back to the identity reds line 185 (SiluAndMul), 251 (GeluAndMul) and 317 (MoeSiluMul), two assertions each, then a byte-for-byte restore. So the gelu case discriminates on the arm that matters. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
09df71a to
efa3959
Compare
This PR is a candidate for the unscoped ROCm term that blocks every gfx1151 speed numberFlagging a connection that is not in this PR's body and that raises its priority considerably. A hypothesis, not a claim — I have not measured it.
And the part that matters here:
That last sentence is the open question, and this PR is in exactly the right class to be part of the answer: a ROCm-only rounding difference in I am not asserting it IS the cause. There could be several such terms, and #2966 alleges the same defect in the gelu-mul kernels. But it makes this PR worth more than a tidy-up: Why that matters beyond correctness. What I would like to see happen with it
The harness for step 2 already exists and does not need rewriting: |
Measured: RULED OUT as the cause of the gfx1151 token-gate divergenceI raised the hypothesis that this PR's narrowing might be the "term of its own that nobody has scoped" behind rc job Against
Not narrowed, not moved, not even shifted by an index. This PR changes nothing about the token gate, and the ROCm-only term remains unscoped. What this does NOT sayThis is not a criticism of the change. It builds and it is correct, and that was measured on the board in the same job — 3 cases, 7061 of 7061 assertions, 0 skipped, matching the count derived from the test source before the run. The discrimination arm is what makes it trustworthy: without the fix, 6 CHECKs go red and every one is a It is a real correctness fix that happens not to be this bug. Two things worth carrying forward1. The failure is reproducible, which is new information. This is an independent second run, four days later, on a different build, and it reproduces the divergence set exactly — same three prompts, same first-differing index. So the 3-of-6 is a stable property of the ROCm arm and not run-to-run noise. That makes it a tractable target: a bisect over candidate terms will give clean signal. 2. #2966 is the next candidate, and it alleges the same missing narrowing in the gelu-mul kernels. Applying the same method to it costs one lease and returns implicated-or-ruled-out either way. Why this matters beyond correctness
|
…2957 is ruled out as its cause Second independent run of the Qwen3.8-27B Q4_K_M ROCm token gate, four days after the first and on a different build, on a tree carrying #2957 (`ac77de7b4`). rc job `74a5a81c` on `strix:gpu0`. TOKENIZER_DIVERGENCES=0/6 GENERATION_DIVERGENCES=3/6 TOKEN_GATE=FAIL GEN[1] first differing index 45 GEN[3] first differing index 45 GEN[5] first differing index 32 RULED OUT, BY A RULE FIXED BEFORE THE RUN. The task briefing committed the interpretation in advance: a divergence set that MOVES implicates the term, a set that is UNCHANGED rules it out. The predecessor recorded ROCm losing prompts 1, 3 and 5 with prompt 1 diverging at step 45. This run loses prompts 1, 3 and 5 with prompt 1 diverging at step 45. Not narrowed, not shifted, not by one index. The hypothesis was worth testing and is simply wrong. #2957 narrows `silu(gate)` to the gate dtype on the MoE path, which is the right CLASS of ROCm-only rounding difference to move a rank-1/rank-2 boundary, and three flips in 288 decode steps is the size of effect a missing narrowing produces. It is not the cause here. THIS DOES NOT ARGUE AGAINST #2957, which the same job measured on the board: 3 cases, 7061 of 7061 assertions, 0 skipped, matching the count derived from the test source BEFORE the run. Its discrimination arm is what makes it trustworthy -- without the fix 6 CHECKs go red and every one is a `bf16 := true` case, the 3 that still pass are exactly the smallest shapes with too few elements to witness a rounding difference, and the ~7052 f32 element-checks are untouched, as `NarrowTo<float>` being the identity requires. It is a correct fix that happens not to be this bug. THE FAILURE IS REPRODUCIBLE, and that is the new information. The predecessor established the set once; this reproduces it exactly on a different build four days later. So the 3-of-6 is a STABLE property of the ROCm arm and not noise, which makes it a tractable bisect target: one lease per candidate term, each returning implicated-or-ruled-out under the same pre-committed rule. #2966 is next. The predecessor's core finding stands untouched: ROCm loses 1/3/5 while the CPU tier loses 1/2/4, disjoint, so our two own tiers emit different tokens on the same prefix and the term is ROCm-specific and internal rather than oracle non-determinism. ONE BOARD FAULT IS RECORDED RATHER THAN AVERAGED AWAY. `LEGS_OK_fix=2 of 3`, and a `Memory access fault by GPU node-1 ... Page not present` on the probe arm. The predecessor read 6 of 6 clean with zero faults, so the fault rate on this board is less settled than that suggested, and it bears on how many repetitions a future gfx1151 measurement needs. No speed, latency or memory figure appears anywhere in this change. While the gate reads FAIL, §Gates admits no throughput number on gfx1151 -- against llama.cpp or against vLLM, which #2740 established does run there. The whole Strix speed axis sits behind these three prompts. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: AGENT:claude-opus-5-1m [claude-code]
Row:
BACKEND-ROCMRefs: #2889 (silu item), #1954, split out of #2894 per its review.
Spec: committed first at 365b7fa (.agents/specs/backend-rocm-moe-silu-rounding.md).
Summary
The ROCm silu-mul kernels computed silu(gate) in f32 and multiplied by up WITHOUT narrowing to the gate tensor's dtype first, diverging from the CPU oracle on exact-equality checks. Fixed in BOTH variants:
SiluMulK(dense) — src/vt/rocm/rocm_dense_basic.hipMoeSiluMulK— src/vt/rocm/rocm_moe_router.hipThe narrowing mirrors upstream vLLM
e126687a9csrc/libtorch_stable/activation_kernels.cu::silu_kernel(intermediate cast to T, then the multiply in compute/packed_compute) and the CPU oracle'sRoundThrough(in_dt, ...)(cpu_ops.cpp:669/733).Verification
test_ops_rocm_silu_roundingasserts exact equality (raw uint16 bits for bf16) vs the CPU oracle across shapes; operator GPU run pending on this box's queue.FOLLOWING_AGENTS_PROTOCOL
Following-Agents-Protocol: true
AI-Assisted: true
Assisted-by: AGENT:OMEN-ALPHA [OMP]