ggml-hip: make -funsafe-math-optimizations opt-in (GGML_HIP_UNSAFE_MATH, default OFF) - #26696
ggml-hip: make -funsafe-math-optimizations opt-in (GGML_HIP_UNSAFE_MATH, default OFF)#26696jimw567 wants to merge 1 commit into
Conversation
ggerganov
left a comment
There was a problem hiding this comment.
Avoid writing overly-verbose PR descriptions and comments with AI. This is against the AI policy of the project and does not leave a good impression.
Regarding the discussion about unsafe math - in theory, it is correct that spec decoding should give identical results to non-spec decoding. However, in practice this is never the case, regardless if unsafe math is used or not. The main source of discrepancy in this case is due to the tokens being processed in different batch sizes when spec decoding is on/off. This leads to different kernels being used, or different FP accumulation depending on how the data gets arranged in the memory.
The bottom line is that this test will sometimes fail even if unsafe math is disabled.
| # mtp-greedy | ||
| # | ||
| # Speculative (MTP) decoding is lossless: at temperature 0 its output must be | ||
| # byte-identical to the non-speculative baseline. On RDNA3.5 (gfx1151) the HIP | ||
| # -funsafe-math-optimizations flag reassociates FP reductions and flips greedy | ||
| # argmax, breaking that identity (AIESW-40114). This asserts identity so a future | ||
| # change that re-enables fast math (or otherwise perturbs the numerics) fails CI. | ||
| # Uses a model with the NextN/MTP head built in: baseline ignores it, draft-mtp | ||
| # uses it. Driven via llama-server (the only tool that takes --spec-type on a | ||
| # single model), matching the downstream regression harness. | ||
|
|
There was a problem hiding this comment.
Avoid writing such long comments - they are not maintainable. Take a look at the rest of the script to see how comments should look like.
There was a problem hiding this comment.
Thanks for pointing this out. I will rewrite.
There was a problem hiding this comment.
I moved the test to test_speculative.py and shorten the comments.
| function gg_run_mtp_greedy { | ||
| cd ${SRC} | ||
|
|
||
| gg_wget models-mnt/qwen3.5-4b-mtp/ https://huggingface.co/unsloth/Qwen3.5-4B-MTP-GGUF/resolve/main/Qwen3.5-4B-Q4_0.gguf |
There was a problem hiding this comment.
This is a very big model - it is not suitable for CI.
There was a problem hiding this comment.
Ok. Let me see if I can find a smaller unit test for CI.
There was a problem hiding this comment.
This 4B model is the smallest MTP model that I can find to reliably guard this contract that my end users care about. I have put measures into the CI pipeline to help the CI runtime.
- Model download is cached. The first test on a fresh rocm runner will add 105s overhead with 78s to download the model. The subsequent PR runs will only incur 27s overhead.
- This test is only enabled for gpu-rocm. If the additional 27s overhead is too big, I can help add additional ROCm runners.
| # $1=label, $2..=extra server args | ||
| function mtp_start_server { | ||
| local label=$1; shift | ||
| ./bin/llama-server --model ${model} --port ${port} --host 127.0.0.1 \ | ||
| -ngl 99 -c 4096 -fa on --poll 50 "$@" > $OUT/${ci}-srv-${label}.log 2>&1 & | ||
| server_pid=$! | ||
| local i=0 | ||
| while [ $i -lt 180 ]; do | ||
| if curl -s http://127.0.0.1:${port}/health 2>/dev/null | grep -q '"status":"ok"'; then | ||
| return 0 | ||
| fi | ||
| if ! kill -0 ${server_pid} 2>/dev/null; then | ||
| echo "server (${label}) died during startup"; tail -20 $OUT/${ci}-srv-${label}.log; return 1 | ||
| fi | ||
| i=$((i+1)); sleep 1 | ||
| done | ||
| echo "server (${label}) failed to become ready"; return 1 | ||
| } | ||
|
|
||
| function mtp_stop_server { | ||
| [ -n "${server_pid}" ] && kill ${server_pid} 2>/dev/null | ||
| wait ${server_pid} 2>/dev/null | ||
| server_pid="" | ||
| } | ||
|
|
||
| # $1=prompt -> assistant content on stdout | ||
| function mtp_ask { | ||
| curl -s http://127.0.0.1:${port}/v1/chat/completions \ | ||
| -H 'Content-Type: application/json' \ | ||
| -d "{\"messages\":[{\"role\":\"user\",\"content\":$(printf '%s' "$1" | python3 -c 'import json,sys; print(json.dumps(sys.stdin.read()))')}],\"stream\":false,\"temperature\":0,\"max_tokens\":200,\"cache_prompt\":false,\"chat_template_kwargs\":{\"enable_thinking\":false}}" \ | ||
| | python3 -c 'import json,sys; print(json.load(sys.stdin)["choices"][0]["message"]["content"])' | ||
| } |
There was a problem hiding this comment.
The ci/run.sh script does not contain server tests. The server tests should go in tools/server/tests.
There was a problem hiding this comment.
I moved the test to test_speculative.py
Points on AI usage are well taken. I will follow the project AI policy going forward. You are right that "the main source of discrepancy in this case is due to the tokens being processed in different batch sizes when spec decoding is on/off.". However, for the models that we have been tracking (Qwen3.5 4B/9B Qwen3.6 27B/35B) in different modes, we hadn't seen any divergence in our daily regression for 2 months until #23962 and now #25495. I will keep a close eye on this new test and will deal with failures as they show up. |
|
This greedy sampling test failing is indeed a bit suspicious. Normally, for this case I would expect the top token to be quite ahead in terms of probability, so I don't see how unsafe math optimizations can make such a big perturbation to shift that and select another token. There might be something else going on beyond FP reduction order. It would be nice to have a similar test in the server test suite, but the main issue that I am not sure how to address is that this is a big model. The current models that we use are intentionally tiny in order to keep the CI resources low: llama.cpp/tools/server/tests/utils.py Lines 505 to 637 in 9a31e1d In that category, a 4B model (even if quantized) is quite large. So we have to figure out some alternative. |
|
Correction - we already have a speculative test here: llama.cpp/tools/server/tests/unit/test_speculative.py Lines 26 to 54 in 6de1b63 So you can try to start from it. If I am not mistaken, it's quite similar to the case that you want to add. |
|
Since its causing correctness issues we should not offer the option at all until its fixed |
|
On my machine the effects of -funsafe-math-optmizations means that many Q8 model degenerate almost immediately. I agree that this option should be default Off and/or removed until stability can be proved. |
abf89bc to
a5023a1
Compare
gpu-rocm CI failed:
|
|
@ggml-org/ci Can someone please help review this PR? Thanks! |
| option(GGML_HIP_NO_VMM "ggml: do not try to use HIP VMM" ON) | ||
| option(GGML_HIP_MMQ_MFMA "ggml: enable MFMA MMA for CDNA in MMQ" ON) | ||
| option(GGML_HIP_EXPORT_METRICS "ggml: enable kernel perf metrics output" OFF) | ||
| option(GGML_HIP_UNSAFE_MATH "ggml: compile HIP with -funsafe-math-optimizations" OFF) |
There was a problem hiding this comment.
Lets just remove the option and -funsafe-math-optimizations entirely until we are sure there is no real issue her, otherwise we will have users turning it back on and filing issues.
|
lets just remove the option for now (if we want the to spend the ci resources for the test is something @ggerganov or @CISC must decide) I assigned myself #26982 to check what kernel is causing the difference. |
@IMbackK Thanks for reviewing! Sure. Let me just remove the option entirely so the accuracy is preserved. I will roll the CI changes into a different PR. |
It enables -fassociative-math, which reassociates FP reductions and can flip greedy argmax on RDNA3.5 (e.g. MTP speculative decode diverging from the non-speculative baseline). Drop it so HIP builds are IEEE-conformant.
34361df to
29f696f
Compare
|
@IMbackK @ggerganov I reduced the PR to just one line removal. Please help review again. |
Overview
-funsafe-math-optimizationscompile flag behind a new CMake optionGGML_HIP_UNSAFE_MATH, defaulting OFF, so HIP builds are IEEE-conformant bydefault. Opt back into the fast-math speedup with
-DGGML_HIP_UNSAFE_MATH=ON.speculative decoding stays byte-identical to the non-speculative baseline at temp 0.
Motivation
Speculative decoding (incl. MTP) is a lossless technique: at temperature 0 its
non-associativity."
-funsafe-math-optimizationsenables-fassociative-math, whichreassociates FP reductions. On RDNA3.5 (gfx1151) this widens the numerical noise
enough to flip near-tie greedy argmax decisions, so MTP output diverges from a
baseline that was previously bit-identical (AIESW-40114).
For determinism-sensitive paths (MTP identity, reproducible greedy output) the
correct default is not to reorder FP ops. The flag stays available for users who want
the speed and accept reduced determinism. Note upstream #25495 already removed the
stronger
-ffast-math; this addresses the remaining-funsafe-math-optimizations.Evidence (gfx1151 / Qwen3.5-4B-Q4_0 MTP, temp 0)
Baseline
--spec-type nonevs--spec-type draft-mtp --spec-draft-n-max 3, comparedbyte-for-byte:
-DGGML_HIP_UNSAFE_MATH=ONBoth the fix and the test's negative control were validated on hardware: the test
goes green on the fixed build and red the moment fast math is re-enabled.
Changes
ggml/CMakeLists.txt: addoption(GGML_HIP_UNSAFE_MATH ... OFF).ggml/src/ggml-hip/CMakeLists.txt: wrap-funsafe-math-optimizationsinif (GGML_HIP_UNSAFE_MATH).ci/run.sh: addgg_run_mtp_greedy, gated toGG_BUILD_ROCM. Downloadsunsloth/Qwen3.5-4B-MTP (NextN head built in), drives baseline vs draft-mtp via
llama-serverat temp 0, and fails if the greedy outputs differ.Test plan
-funsafe-math-optimizationsabsent; MTP greedy == baseline (IDENTICAL)-DGGML_HIP_UNSAFE_MATH=ON: flag present; MTP greedy != baseline (DIVERGED) -> CI check exits nonzerogg_run_mtp_greedyruns and passes on the gfx1151 runnerAdditional information
#26611
Requirements