Fix incorrect NaN payload values in isCanonicalNaN and isArithmeticNaN#8350
Open
sumleo wants to merge 2 commits intoWebAssembly:mainfrom
Open
Fix incorrect NaN payload values in isCanonicalNaN and isArithmeticNaN#8350sumleo wants to merge 2 commits intoWebAssembly:mainfrom
sumleo wants to merge 2 commits intoWebAssembly:mainfrom
Conversation
The wasm spec defines a canonical NaN as having only the quiet bit set in the significand (bit 22 for f32, bit 51 for f64), giving payloads of 0x400000 and 0x8000000000000 respectively. The previous code checked for the maximum possible payload ((1 << 23) - 1 = 0x7FFFFF for f32), which is not what the spec requires. Similarly, isArithmeticNaN checked payload > max_payload, which could never be true. Per the spec, an arithmetic NaN is any NaN with the quiet bit set, so the correct check is whether the quiet bit is set in the payload. These fixes allow f32.wast, f64.wast, and float_exprs.wast spec tests to pass, which were previously skipped due to this bug.
Member
|
Please un-skip those tests, now that they pass. That will also serve as testing for the PR. |
Contributor
Author
|
Done — un-skipped |
sumleo
force-pushed
the
fix-nan-payload-values
branch
from
8efe63e to
4228af0
Compare
The original skip reasons cited incorrect NaN canonicality checks, which are now fixed by the isCanonicalNaN/isArithmeticNaN changes. However, these tests still fail on Linux due to platform-dependent NaN propagation where arithmetic operations can produce signaling NaNs instead of quiet NaNs. Update the skip comments accordingly.
sumleo
force-pushed
the
fix-nan-payload-values
branch
from
4228af0 to
344b47f
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
2 participants
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.
Summary
isCanonicalNaNto check for the correct canonical NaN payload per the wasm spec:1 << 22(0x400000) for f32 and1ull << 51(0x8000000000000) for f64, instead of the incorrect(1 << 23) - 1(0x7FFFFF) and(1ull << 52) - 1.isArithmeticNaNto check whether the quiet bit is set in the payload (bit 22 for f32, bit 51 for f64), instead of the impossiblepayload > max_payloadcomparison that could never be true.Details
The wasm spec defines:
nan:0x400000, for f64 this isnan:0x8000000000000.The previous
isCanonicalNaNchecked if the payload equaled the maximum 23-bit value (all bits set), which is not what the spec requires. The previousisArithmeticNaNcomparedpayload > (1u << 23) - 1, but since 0x7FFFFF is already the maximum possible 23-bit payload, this could never return true.This fix should enable un-skipping the
f32.wast,f64.wast, andfloat_exprs.wastspec tests, which are currently skipped inscripts/test/shared.pybecause of this bug.Test plan
binaryen-unittests)wasm-shell test/spec/testsuite/f32.wast- all checks passedwasm-shell test/spec/testsuite/f64.wast- all checks passedwasm-shell test/spec/testsuite/float_exprs.wast- all checks passed