1311403 - wasm: Remove overly constraining NaN-preservation tests

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Lars T Hansen [:lth]]

At the bottom of jit-test/tests/wasm/float.js there are a number of tests that test preservation of NaN payloads.  Based on my reading of the current (Oct 19) spec (Semantics.md) these tests are too constraining and IMO they should be removed and new tests for the current behavior should be written.

For example:

(module
 (func (result i32)
  (i32.reinterpret/f32 (f32.mul (f32.const 0.0) (f32.const -nan:0x222222))))
 (export "" 0))

is expected to evaluate to the bit pattern 0xFFE22222 in the first of those tests.  I think this is wrong, because:

First, the NaN result rule in "Floating Point Operators" applies at the multiplication; since the only NaN operand has a payload that is not MSB=1 followed by zeroes, the second bullet applies, giving us an arbitrary NaN.  (Preserving the input is allowed but not required.)

Second, the NaN rule in "Data Type Conversions, ..." applies at the reinterpretation; again the NaN operand may be assumed to have a payload that is not MSB=1 followed by zeroes, so the second bullet applies, giving us a random NaN.  (Again preserving the input isallowed but not required.)

Note also that 0xFFE22222 is the result if the multiplication is elided or first checks for NaN, and if the reinterpretation subsequently only moves the bits.  Ion turns this into a single load-constant, so the test conforms to the expected output when compiled with Ion.  The baseline compiler actually executes the multiplication and ends up with a canonical NaN instead.

(This is all on ARM btw, running on the ARM simulator.)

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1311403-nan-tests.patch

Remove test cases.

(Obviously this should not land until we've concluded the discussion but these are the tests I think need to go.)

Comment 2

[Dan Gohman [:sunfish]]

I agree with your analysis; these tests should be removed.

FWIW, it is an open issue in the spec repository to implement official tests for the new less-strict behavior: https://github.com/WebAssembly/spec/issues/286.

Comment 3

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/579e82f5c90fd1129577ac1def89803fe5967bbf
Bug 1311403 - remove overly constraining NaN-preservation tests. r=bbouvier

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/579e82f5c90f