fix(gnovm): correct softfloat add/sub for normal operands cancelling to subnormal#5818
Open
omarsy wants to merge 1 commit into
Open
fix(gnovm): correct softfloat add/sub for normal operands cancelling to subnormal#5818omarsy wants to merge 1 commit into
omarsy wants to merge 1 commit into
Conversation
Collaborator
🛠 PR Checks Summary🔴 Pending initial approval by a review team member, or review from tech-staff Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
…to subnormal gno's softfloat (a verbatim copy of Go's runtime/softfloat64.go) returns a wrongly-scaled subnormal when two near-equal, opposite-sign normal float64 values cancel into a subnormal result (|x+y| < 2.2e-308). fadd64/fsub64 hand fpack64 a heavily-cancelled mantissa (mant0 << 1<<mantbits64) while exp0 is still a normal-range exponent; the denormal branch resets to (mant0, exp0) and only right-shifts, which is the wrong direction in that case. Fix: re-normalize the mantissa (left-shift) before aligning to the subnormal exponent. This is a no-op for already-normalized callers (mul/div/conversions), so it cannot regress them. The fix is applied through the generator so it survives `go generate`, with an anchor guard that fails loudly if upstream Go changes or fixes the code. fpack32 carries the identical latent bug and is patched for parity. Verified: the reported add/sub operands now return 847895691526144 (matching `go run`); 0 mismatches vs hardware across 30M cancellation pairs (17M subnormal) and 20M random pairs over +,-,*,/; the same fix corrects a real GOMIPS=softfloat go1.22.3 build under qemu-mips. A regression case is added to the softfloat64_test.go all-pairs base slice. Fixes gnolang#5806 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
e554fa0 to
cd7b76c
Compare
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.
Description
Fixes #5806.
gno runandgo rundisagree onfloat64+/-for certain operands: when two near-equal, opposite-sign normal float64 values cancel into a subnormal result (|x+y| < 2.2e-308), gno returns a wrongly-scaled value (off by ~6 orders of magnitude), e.g. the issue's MRE returns202154086instead of847895691526144.Root cause
gno's softfloat (
runtime_softfloat64.go) is a verbatim copy of Go'sruntime/softfloat64.go, and the bug is in upstream'sfpack64. Under heavy cancellation,fadd64handsfpack64a mantissa far below1<<mantbits64while the exponent is still normal-range. The denormal branch resets to the un-normalized(mant0, exp0)and only right-shifts to align to the subnormal exponent — the wrong direction in this case — so it returns the un-normalized cancellation mantissa at the wrong scale instead of the correctly-rounded subnormal.This is latent in upstream Go too (it only executes on software-float targets like
GOMIPS=softfloat); gno hits it because it always uses softfloat for deterministic, hardware-independent results.Fix
Re-normalize the mantissa (left-shift) before the subnormal alignment loop — restoring the normalization invariant
fpack64already applies on its normal path (the identical loop runs at the top of the function). It is a no-op for already-normalized callers (fmul64/fdiv64/conversions), so they are unaffected; only thefadd64/fsub64cancellation case changes.fpack32carries the identical latent bug and is fixed for parity.Because the file is generated (
DO NOT EDIT, copied from$GOROOT), the fix is applied through the generator so it survivesgo generate, with an anchor guard that fails loudly if a future Go toolchain changes or fixes the upstream code.Changes
gnovm/pkg/gnolang/internal/softfloat/runtime_softfloat64.go: left-normalization loop added to the denormal branch offpack64andfpack32.gnovm/pkg/gnolang/internal/softfloat/gen/main.go: applies the fix duringgo generate(with astrings.Count==1anchor guard) plus injects the regression case into the generated test.gnovm/pkg/gnolang/internal/softfloat/runtime_softfloat64_test.go: two cancellation operands added toTestFloat64's all-pairs (soft-vs-hardware)baseslice.Verification
847895691526144, matchinggo run.+ - * /; the previous code was wrong on >50% of cancellation-to-subnormal pairs.fpack32path (f64->f32 narrowing) and full conversion sweeps match hardware; the new loop is a verified no-op for non-add/sub callers.GOMIPS=softfloatgo1.22.3build under qemu-mips (202154086->847895691526144).This bug has also been reported upstream to Go; once fixed there, the generator's anchor guard will trip on the next toolchain bump so the local workaround can be removed.