Fix 32-bit fixed-point multiply to use a correct 64-bit intermediate#48
Open
RichardHoekstra wants to merge 1 commit into
Open
Fix 32-bit fixed-point multiply to use a correct 64-bit intermediate#48RichardHoekstra wants to merge 1 commit into
RichardHoekstra wants to merge 1 commit into
Conversation
The 32-bit path of FixedMul was wrong for any operand with a nonzero integer part. The hi/lo decomposition left result_hi = a_hi*b_hi without its << 16 and shifted the cross terms right instead of keeping them at weight, so e.g. 3.0*3.0 returned 9 (raw) instead of 9<<16, and 2.0*0.5 returned 1 instead of 65536 -- off by ~2^16. This affected both FixedMultiplyFallback and FixedMulAsm's simulated body (the real inline-asm block is gated behind __ASM_AVAILABLE__, which is never defined, so the simulated path is what actually compiles). It is masked on 64-bit builds, which take the correct __FB_64BIT__ branch, so it only bites 32-bit DOS targets -- exercised by the lab/reference transformer (attention scaling, LayerNorm, GELU) and by InitQuantizationTables. The production TinyGPT path (real_gpt.bas) is unaffected: it uses its own correct TinyGPTFixedMul. Both branches now use a 64-bit LONGINT intermediate (emulated on 32-bit targets), matching the existing __FB_64BIT__ branch and TinyGPTFixedMul. Verified in the real DOS runtime (DJGPP fbc + dosbox-x): 0.25*0.25, 2.0*0.5, 3.0*3.0, 1.5*2.0, 10.0*0.1 and -2.0*3.0 all now match a 64-bit reference exactly.
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.
Problem
The 32-bit path of
FixedMul(Q16.16) is wrong for any operand with a nonzero integer part. It splits each operand into hi/lo halves, but:The correct expansion of
(a*b) >> 16isa_hi·b_hi·2^16 + (a_hi·b_lo + a_lo·b_hi) + (a_lo·b_lo)>>16. Two terms are misplaced:result_hiis missing its<< 16, and the cross terms are shifted right instead of kept at weight. Only the pure-fraction term lands correctly, so the result is right only when both operands are< 1.0.Measured in the real DOS runtime (before the fix):
Where it bites
__ASM_AVAILABLE__is never defined, soFixedMulAsm's real inline-asm block never compiles — its simulated#ELSEbody is the same buggy formula asFixedMultiplyFallback. SoHasAssemblySupport()doesn't gate it: both 32-bit branches are wrong.#IFDEF __FB_64BIT__branch — which is why it isn't visible in development.InitQuantizationTables.main_prod→real_gpt.basuses its own correctTinyGPTFixedMul((CLNGINT(a)*CLNGINT(b)) \ 4096).Fix
Both 32-bit branches now use a 64-bit
LONGINTintermediate (emulated on 32-bit targets), matching the existing__FB_64BIT__branch andTinyGPTFixedMul:Verification
Re-run of the same DOS repro against the patched functions — every case now matches a 64-bit reference exactly, including negatives:
src/main_prod.bascross-compiles to DOS clean with this change.python3 -m unittest discover tests→ 108 passed;git diff --checkclean;verify_workspace_tracking.pypasses.src/asm_optimizations.bas(+16 / −31).