Skip to content

Fix 32-bit fixed-point multiply to use a correct 64-bit intermediate#48

Open
RichardHoekstra wants to merge 1 commit into
tsotchke:mainfrom
RichardHoekstra:fix/fixed-multiply-32bit
Open

Fix 32-bit fixed-point multiply to use a correct 64-bit intermediate#48
RichardHoekstra wants to merge 1 commit into
tsotchke:mainfrom
RichardHoekstra:fix/fixed-multiply-32bit

Conversation

@RichardHoekstra

Copy link
Copy Markdown

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:

result_hi = a_hi * b_hi                  ' missing the << 16
result_lo = (a_lo * b_lo) SHR 16
result = result_hi + result_lo + ((a_hi * b_lo) SHR 16) + ((a_lo * b_hi) SHR 16)

The correct expansion of (a*b) >> 16 is a_hi·b_hi·2^16 + (a_hi·b_lo + a_lo·b_hi) + (a_lo·b_lo)>>16. Two terms are misplaced: result_hi is 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):

product expected (Q16.16) fallback returned
0.25 × 0.25 4096 4096 ✓
2.0 × 0.5 65536 1
3.0 × 3.0 589824 9
1.5 × 2.0 196608 3
10.0 × 0.1 65536 1

Where it bites

  • __ASM_AVAILABLE__ is never defined, so FixedMulAsm's real inline-asm block never compiles — its simulated #ELSE body is the same buggy formula as FixedMultiplyFallback. So HasAssemblySupport() doesn't gate it: both 32-bit branches are wrong.
  • It's masked on 64-bit builds, which take the correct #IFDEF __FB_64BIT__ branch — which is why it isn't visible in development.
  • On 32-bit DOS it affects the lab / reference transformer path (attention scaling, LayerNorm, GELU, softmax) and InitQuantizationTables.
  • The shipped preview is unaffected: main_prodreal_gpt.bas uses its own correct TinyGPTFixedMul ((CLNGINT(a)*CLNGINT(b)) \ 4096).

Fix

Both 32-bit branches now use a 64-bit LONGINT intermediate (emulated on 32-bit targets), matching the existing __FB_64BIT__ branch and TinyGPTFixedMul:

DIM result AS LONGINT
result = CLNGINT(a) * CLNGINT(b)
result = result SHR FIXED_POINT_SHIFT
RETURN CLNG(result)

Verification

Re-run of the same DOS repro against the patched functions — every case now matches a 64-bit reference exactly, including negatives:

product ref fallback asm-sim
0.25 × 0.25 4096 4096 4096
2.0 × 0.5 65536 65536 65536
3.0 × 3.0 589824 589824 589824
1.5 × 2.0 196608 196608 196608
10.0 × 0.1 65540 65540 65540
−2.0 × 3.0 −393216 −393216 −393216
  • Cross-compiled with DJGPP FreeBASIC, run under DPMI (dosbox-x).
  • src/main_prod.bas cross-compiles to DOS clean with this change.
  • python3 -m unittest discover tests → 108 passed; git diff --check clean; verify_workspace_tracking.py passes.
  • Diff is one file, two functions in src/asm_optimizations.bas (+16 / −31).

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant