Repository navigation
docs(cuda): correct Marlin characterization in gate/up SwiGLU ULP-sweep comments (follow-up to #1404) - #1410
Merged
Conversation
…ep comments Follow-up to #1404. Two landed doc comments in the plain `fp16_gate_up_swiglu_two_op_ulp_bound_sweep` mischaracterized the default plain-M>1 path: - They said plain M>1 "dispatches Marlin split-K (default-on)" and that split-K is "possibly non-deterministic". Both are wrong. Verified against `marlin_gemm.rs`: * Split-K is gated by `ONNX_GENAI_MARLIN_SPLITK`, which is **default OFF** (`marlin_splitk_enabled`), and is documented **deterministic** (fixed-order fp32 partial reduction, within f64-oracle tolerance, argmax/greedy tokens byte-identical, validated e2e on glm-4-9b and qwen2.5-14b). * The default plain M>1 fused gate/up path runs the **direct** Marlin int4 tensor-core GEMM (`launch_marlin_gemm` via `maybe_launch_marlin_splitk` when split-K is off), which is documented non-byte-identical to the tiled two-op reference by a different accumulation order (parity tests assert a `2e-2 * max_out` tolerance, not equality). The corrected comments state the accurate path (direct Marlin, not split-K), drop "possibly non-deterministic", and record that `main` does not hold "M>1 is bit-exact to the two-op reference" under the default config (`fp16_gate_up_swiglu_is_bit_exact_to_two_op_path` reds at M=5 with Marlin default-on; passes only under `ONNX_GENAI_MARLIN_M_GT_1=0`). Comment-only change; no behavior change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
🔴 Benchmark Regression DetectedComparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).
Visual flags: Host infoWhat this cannot catch
|
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.
What this does
Comment-only follow-up to #1404. Corrects two landed doc comments in
matmul_nbits.rs(the plainfp16_gate_up_swiglu_two_op_ulp_bound_sweep) that mischaracterized the default plain-M>1 path.What was wrong
The comments said plain M>1 "dispatches Marlin split-K (default-on), which is documented non-byte-identical and possibly non-deterministic." Both halves are wrong. Verified against
crates/onnx-runtime-ep-cuda/src/kernels/marlin_gemm.rs:ONNX_GENAI_MARLIN_SPLITK(marlin_splitk_enabled()), separate fromONNX_GENAI_MARLIN_M_GT_1.maybe_launch_marlin_splitkelects it only when that flag is set andchoose_split_k > 1; otherwise it calls the direct kernel. So split-K does not run under the default config.launch_marlin_gemm), documented non-byte-identical to the tiled two-op reference by a different accumulation order (parity tests assert a2e-2 * max_outtolerance, not equality).What the corrected comments say
They now name the correct path (direct Marlin, not split-K), drop "possibly non-deterministic", and record that
maindoes not hold "M>1 is bit-exact to the two-op reference" under the default config —fp16_gate_up_swiglu_is_bit_exact_to_two_op_pathreds at M=5 with Marlin default-on and passes only underONNX_GENAI_MARLIN_M_GT_1=0.This aligns the code comments with the corrected framing already applied to PR #1404's body, issue #1405 (now titled "direct tensor-core GEMM, not split-K"), and the #1305 correction comment.
Why it's safe
Pure comment change — no code, no behavior change, no test change. The
//////edits do not touch any executable line.Follows-up #1404. Marlin write-up: #1405.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com