Skip to content
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

Implement the new NaN semantics #286

Closed
sunfishcode opened this issue May 12, 2016 · 10 comments
Closed

Implement the new NaN semantics #286

sunfishcode opened this issue May 12, 2016 · 10 comments
Milestone

Comments

@sunfishcode
Copy link
Member

The reference interpreter and testsuite should be updated to reflect WebAssembly/design#660. I expect to work on this at some point, but I don't know when I'll find sufficient time. I'd be happy to help anyone else interested in working on this in the meantime.

One possible implementation would be to extend assert_return_nan. Currently, assert_return_nan tests for either negative or positive default NaN. If we made it also accept any nan that appears as an operand, but with the fraction field MSB set to 1, then we could use it both for cases like this where there are no nan operands:

f32.wast:
(assert_return_nan (invoke "add" (f32.const -infinity) (f32.const infinity))

and cases like this where there's a nan operand and we want to validate that we either get that operand with the fraction field MSB set to 1, or one of the default NaN values:

float_misc.wast:
(assert_return (invoke "f32.add" (f32.const nan:0x200000) (f32.const 1.0)) (f32.const nan:0x600000))

@sunfishcode sunfishcode added this to the MVP milestone May 12, 2016
@rossberg
Copy link
Member

rossberg commented May 19, 2016

In general, there is no relation between arguments and result of an invoke, so looking at the arguments would perhaps be a bit weird. Instead, what if assert_result_nan simply allowed specifying a list of acceptable NaN values?

@sunfishcode
Copy link
Member Author

The list of acceptable NaN values is a function of the argument list, and it's the same function for all arithmetic operators (neg, abs, copysign notwithstanding), so it'd be very redundant to specify them in a separate list on each test. However it's just convenience for testcases, so I'd be ok either way.

@rossberg
Copy link
Member

That's the case for operators, but not for functions, which are what gets invoked in an assert. I can easily write one where the result has nothing whatsoever to do with the arguments. ;)

@sunfishcode
Copy link
Member Author

What I've proposed here is indeed a specialized mechanism. While specialized mechanisms typically have fewer uses than general-purpose mechanisms, they can typically offer those uses they do have greater advantages.

Specifically, my proposal would keep a large number of existing tests readable and maintainable, and makes it easier to add tests for new future operators, and makes it easier to tweak the NaN semantics (in case that becomes necessary, which is a very real possibility). The general-purpose mechanism proposed here would not have these advantages, even if it would have other advantages.

@sunfishcode
Copy link
Member Author

WebAssembly/design#716 is now merged in the design repo and has updated NaN propagation semantics.

@sunfishcode
Copy link
Member Author

Specifically, with the rules now in place, we need tests for:

  • Test that arithmetic instructions other than abs/neg/copysign correctly set the most-significant bit of the significand of a NaN result to 1 (in other words, convert "signaling NaN" to "quiet NaN"). We have some coverage of this for some operators, but not all.
  • Test that arithmetic instructions with no "non-canonical" NaN inputs return "canonical" NaNs. Note that "canonical" in wasm can have either value for the sign bit, so tests like (assert_return (invoke "sqrt" (f64.const -nan)) (f64.const -nan)) should be relaxed to accept either nan or -nan.

@sunfishcode
Copy link
Member Author

Testing that operators set the most-significant bit of NaN significands (the first bullet, above) is desirable for all operators, but it's particularly important for min/max because of the way these operators are often implemented on platforms that don't have native support.

@rfk
Copy link
Contributor

rfk commented Jan 23, 2017

I find myself needing this update in order to properly test a little wasm-to-js converter I've been toying with [1], so I'm going to try to brush off my ML and and give it a shot. IIUC there are three behaviours that need to be tested for:

  • Whether operators like abs/neg/copysign produce a specific NaN bitpattern
  • Whether other operators produce one of the two canonical NaNs when given only canonical inputs
  • Whether other operators produce valid quiet NaNs when given non-canonical inputs

Would it suffice to have three assert_return variants for these three cases?

  • assert_return, which is already used for testing for a specific NaN
  • assert_return_nan, which could accept any quiet NaN
  • assert_return_canonical_nan, which could accept only nan and -nan

Thoughts? I won't make any promises, but I'll try to make some time to poke at this over the next couple of days.

[1] https://github.com/rfk/wasm-polyfill

@rossberg
Copy link
Member

@rfk, thanks, sounds good to me!

@sunfishcode
Copy link
Member Author

With #414 merged, this is now fixed.

dhil pushed a commit to dhil/webassembly-spec that referenced this issue Oct 3, 2023
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

No branches or pull requests

3 participants