Skip to content

Convert remaining NaN tests to use new assert types #448

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

Closed
wants to merge 2 commits into from

Conversation

Cellule
Copy link
Contributor

@Cellule Cellule commented Mar 27, 2017

Use assert_return_canonical_nan and assert_return_arithmetic_nan instead of assert_return with a nan expected value.
It has been a while since those should have changed to assert_return_nan, but now that we've introduced 2 new types of asserts for NaN, I thought it would be a good time to clean up.

…instead of `assert_return` with a `nan` expected value
@Cellule
Copy link
Contributor Author

Cellule commented Mar 27, 2017

It seems the interpreter considers the value 0x7fa00000 to be an invalid type of nan in the test
(assert_return_arithmetic_nan (invoke "f32.reinterpret_i32" (i32.const 0x7fa00000))) (conversion.wast)

With the implementation

| [Values.F32 got_f32] -> let pos_nan = F32.to_bits F32.pos_nan in
        Int32.logand (F32.to_bits got_f32) pos_nan = pos_nan

0x7fa00000 & 0x7fc00000 != 0x7fc00000

I read WebAssembly/design#660 one more time, but I can't figure out what is the correct thing to do in this case.
Also, I am not very fluent in caml and could use some help on this

@Cellule
Copy link
Contributor Author

Cellule commented Mar 27, 2017

I think I found the problem, I think we should use bare_nan instead of pos_nan to check if it's a valid nan for assert_return_arithmetic_nan

@rossberg
Copy link
Member

Actually, various uses of assert_return on a NaN are intentional: they specifically test for exact bit patterns. Wasm is only loose on the bit patterns of NaNs generated from arithmetic operations, in other cases like reinterpret or memory reads/writes the exact bit pattern must be preserved. The new NaN assertions are special cases to be used only where the result value is underspecified, and multiple values are possible.

@sunfishcode
Copy link
Member

The promote/demote test changes are right; it looks like we missed those when we converted to the new asserts. The others are as @rossberg-chromium says. In particular, abs, neg, and copysign are also defined to be bitpattern-preserving (in accordance with IEEE 754).

@Cellule Cellule closed this Mar 27, 2017
@sunfishcode sunfishcode mentioned this pull request Mar 31, 2017
@Cellule Cellule deleted the nan branch September 13, 2017 20:39
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.

3 participants