Skip to content

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