Skip to content

More floating point tests! #282

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 31 commits into from
May 6, 2016
Merged

More floating point tests! #282

merged 31 commits into from
May 6, 2016

Conversation

sunfishcode
Copy link
Member

This adds a bunch more tests focused on floating point.

Note that there are some minor inconsistencies in general with how NaNs are handled; I plan to address those separately as part of the work related to WebAssembly/design#660.

@sunfishcode
Copy link
Member Author

Ping :-).

@kripken
Copy link
Member

kripken commented May 5, 2016

I skimmed the tests and things look good to me, although, most goes above my head ;)

However, the binaryen interpreter fails in float_exprs.wast on

(assert_return (invoke "f32.no_fold_div_0" (f32.const nan:0x200000)) (f32.const nan:0x600000))

I'm not sure I understand what is being tested there?

@jfbastien
Copy link
Member

I skimmed the tests and things look good to me, although, most goes above my head ;)

However, the binaryen interpreter fails in float_exprs.wast on

(assert_return (invoke "f32.no_fold_div_0" (f32.const nan:0x200000)) (f32.const nan:0x600000))

I'm not sure I understand what is being tested there?

Looks like the bug is this last line:

  Literal div(const Literal& other) const {
    switch (type) {
      case WasmType::f32: {
        float lhs = getf32(), rhs = other.getf32();
        float sign = std::signbit(lhs) == std::signbit(rhs) ? 0.f : -0.f;
        switch (std::fpclassify(rhs)) {
          case FP_ZERO:
          switch (std::fpclassify(lhs)) {
            case FP_NAN: return *this;

AstSemantics says:

When the result of any arithmetic operation other than neg, abs, or copysign is a NaN, the sign bit and the fraction field (which does not include the implicit leading digit of the significand) of the NaN are computed as follows:
*If the operation has any NaN input values, implementations may select any of them to be the result value, but with the most significant bit of the fraction field overwritten to be 1.

  • If the implementation does not choose to use an input NaN as a result value, or if there are no input NaNs, the result value has a nondeterministic sign bit, a fraction field with 1 in the most significant bit and 0 in the remaining bits.

So the requirement is to set 1 as the MSB of the fraction field, rest can be zero or the original value, and sign bit can be anything. The test seems over-constrained, but I also have a bug in binaryen. I'll fix :-)

@sunfishcode
Copy link
Member Author

It's testing that a floating-point divide of a signaling NaN operand divided by an immediate zero returns a quiet NaN, per IEEE 754, and that the rest of the NaN payload and sign bit are propagated per wasm's rules, except that it's not yet updated for WebAssembly/design#660. I plan to update it when I update all the tests.

@@ -1219,6 +1271,19 @@

(assert_return (invoke "llvm_pr27153" (i32.const 33554434)) (f32.const 25165824.000000))

;; Test that (float)x + (float)y is not optimzied to (float)(x + y) when unsafe.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optimized

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed; thanks!

jfbastien added a commit to WebAssembly/binaryen that referenced this pull request May 5, 2016
@jfbastien
Copy link
Member

NaN / 0 fixed in WebAssembly/binaryen#442. It still seems like the test is over-constrained? For now all the implementations do "the right thing" so maybe just comment that it's over-constrained?

@sunfishcode
Copy link
Member Author

Can you clarify in what way it's over-constrained?

jfbastien added a commit to WebAssembly/binaryen that referenced this pull request May 5, 2016
@jfbastien
Copy link
Member

Can you clarify in what way it's over-constrained?

I'm not sure if I'm misunderstanding, but this seems to give leeway to implementations:

  • If the operation has any NaN input values, implementations may select any of them to be the result value, but with the most significant bit of the fraction field overwritten to be 1.
  • If the implementation does not choose to use an input NaN as a result value, or if there are no input NaNs, the result value has a nondeterministic sign bit, a fraction field with 1 in the most significant bit and 0 in the remaining bits.

It sounds like the sign can be whatever, and the NaN can propagate to quiet (same payload, but not signaling) or be quiet NaN with zeroes as fraction. Is that correct?

@sunfishcode
Copy link
Member Author

That's the new text in WebAssembly/design#660. Many existing tests also need to be updated as well. I plan to do all the updates at once.

binji added a commit to WebAssembly/wabt that referenced this pull request May 5, 2016
@jfbastien
Copy link
Member

That's the new text in WebAssembly/design#660. Many existing tests also need to be updated as well. I plan to do all the updates at once.

Ah right, ty! /me unconfused

@jfbastien
Copy link
Member

High-level lgtm!

@sunfishcode
Copy link
Member Author

Merging, with high-level lgtm, since these just add new tests with no spec changes. As always, these and all tests continue to be open to comments and questions.

@sunfishcode sunfishcode merged commit 88ed8e5 into master May 6, 2016
@jfbastien jfbastien deleted the more-tests branch May 6, 2016 01:04
@rossberg
Copy link
Member

rossberg commented May 9, 2016

Sorry, I was away. Also LGTM

ngzhian added a commit to ngzhian/spec that referenced this pull request Nov 4, 2021
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.

5 participants