Fix/rocmfpx encoder review - #31
Open
LaurentZuijdwijk wants to merge 5 commits into
Open
Conversation
Every check in test_rocmfpx.c is a plain assert(), including the rocmfpx_validate_row_data_*() calls and the mse ordering comparisons. The Release configuration the PR builds defines NDEBUG, so all of them compiled away and the test still exited 0 - it validated nothing. Undefine NDEBUG before including <assert.h>, the way test-quantize-fns.cpp and test-quantize-perf.cpp already do. test_rocmfp2_reference.c does not need this: it uses its own always-active check machinery rather than assert(). Assisted-by: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VcXid9TGNfxqxxNWFQGES8
rocmfpx.c is already a source of ggml-base, and everything test_rocmfpx.c calls is declared GGML_API. Compiling it into the test as well breaks MSVC shared-library builds: with BUILD_SHARED_LIBS=ON and GGML_BACKEND_DL=OFF the test inherits GGML_SHARED through llama -> ggml -> ggml-base but not GGML_BUILD, so the codec declarations become __declspec(dllimport) in the same translation unit that defines them, which MSVC rejects. Drop the extra source and let the test link the library implementation. test-rocmfp2-reference keeps compiling rocmfp2_reference.c directly: it is a standalone reference that is not part of ggml-base and carries no GGML_API. Assisted-by: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VcXid9TGNfxqxxNWFQGES8
rocmfpx_choose_scale_fp2_mse() only searched scale bytes 1..126, returning 0
solely when the block was all zeros. ROCmFP2's S40 codebook is
{-4, -1, +1, +4}, which has no zero code, so at any nonzero scale the smallest
magnitude it can emit is one scale step. A block that sits below half of the
smallest UE4M3 scale is then amplified rather than approximated.
A block filled with 2^-14 reconstructed as 2^-10, 16x too large, for a squared
error 225x greater than encoding it as zero. rocmfp2_p1_quantize_block_ref(),
which searches every legal byte 0x00..0x7e, picked scale byte 0 for the same
input - so the two encoders in this tree disagreed.
Seed the search with the zero-scale candidate so bytes 1..126 have to beat it.
Ties keep byte 0, matching both the lower-byte rule the rest of the search uses
and the standalone reference.
before: e=(1,1) sse=2.68e-05
after: e=(0,0) sse=1.19e-07 (reference: 5.96e-08 per 16-weight group)
FP3, FP6 and FP8 all have a zero code and so can already match the zero scale
by rounding; only FP2 needs this.
Assisted-by: Claude Opus 5 (1M context)
Claude-Session: https://claude.ai/code/session_01VcXid9TGNfxqxxNWFQGES8
The FP6 scale search stops descending once the unavoidable clipping error at a scale already exceeds the best error found so far. That bound was computed as max_abs - 31*scale, but rocmfpx_decode_fp6_code() maps code 32 to -32: the representable range is the asymmetric [-32, 31]. For a block whose largest magnitude is negative the bound therefore overestimates the error that cannot be avoided, and the search gives up before reaching the scale that wins. Sweeping 20000 half-blocks with a single dominant outlier: before: 324 suboptimal with a negative outlier, 0 with a positive one after: 0 suboptimal in either case worst observed case: byte 12 at sse 1.783e-04 where byte 11 gives 1.367e-04. The asymmetry between the two columns is exactly what the wrong bound predicts. The Vulkan encoder carried the same bound, so fix both together - the two encoders have to agree on the scale they choose. Assisted-by: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VcXid9TGNfxqxxNWFQGES8
rocmfpx_quantize_fp6_code(), rocmfpx_fp6_decoded_value() and rocmfpx_quantize_fp8_code() rounded to int first and clamped afterwards. The isfinite() guard keeps NaN and inf out, but a large finite weight still overflows the conversion, so the value that gets clamped is already wrong: 1e12 -> fp6 -0.03125 sign flipped 1e30 -> fp6 0, fp8 0 magnitude lost entirely Clamp the float to the representable range first, then convert. For in-range values this is identical to the old order - rounding cannot push a value past a bound it was not already past - so ordinary weights encode bit for bit as before, and the extremes now saturate: 1e12 -> fp6 31*scale, fp8 127*scale 1e30 -> fp6 31*scale, fp8 127*scale The Vulkan encoder had the same shape, and int() of an out-of-range float is undefined in GLSL, so fix it in the same commit to keep the two encoders in agreement. This is extreme-input robustness, not a defect seen on ordinary weights. Assisted-by: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VcXid9TGNfxqxxNWFQGES8
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
1 participant
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.
Overview
Measurements
Baseline:
After:
Correctness:
Additional information
Requirements
belong in halo-box/llama.cpp instead