[wasm interpreter] Support Propagate Arithmetic NaN in wasm interpreter [v8/v8 : main]
Paolo Severini (Gerrit)
Paolo Severini added 4 comments![]( "Open in Gerrit")
Thanks!!! Just a few small comments.
if (V8_UNLIKELY(std::isnan(val))) {I see that this change fixes many tests with NaNs, which is very good.
However, I have two small concertn:
1. The WebAssembly spec says that "When a WebAssembly instruction performs an operation on floating-point numbers and the result is a NaN (Not-a-Number), the exact bit pattern of the NaN is carried through unchanged, rather than being replaced with a canonical NaN."
The whole binary representation is preserved, not just the fact that the NaN is signaling or quiet. It is still an improvement, though.
2. Even if Clang can optimize this code a lot, we are adding a branch to some floating-point operations. Only to `floor`, `ceil` and `trunc`, so it should not be a huge problem.
ctype volatile rval = static_cast<ctype>(reg); \Just to learn more: why these need to be `volatile`?
'test-run-wasm/RunWasmInterpreter_Float32Add*': [SKIP],Can you try to re-enable also the mjsunit test `'wasm/nan-constant'` which is disabled in test/mjsunit/mjsunit.status, line 978?
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 2 comments![]( "Open in Gerrit")
if (V8_UNLIKELY(std::isnan(val))) {I see that this change fixes many tests with NaNs, which is very good.
However, I have two small concertn:1. The WebAssembly spec says that "When a WebAssembly instruction performs an operation on floating-point numbers and the result is a NaN (Not-a-Number), the exact bit pattern of the NaN is carried through unchanged, rather than being replaced with a canonical NaN."
The whole binary representation is preserved, not just the fact that the NaN is signaling or quiet. It is still an improvement, though.2. Even if Clang can optimize this code a lot, we are adding a branch to some floating-point operations. Only to `floor`, `ceil` and `trunc`, so it should not be a huge problem.
Thanks for your comments! However I am not replacing the result with a canonical NaN, but a arithmetic NaN. And I didn't see anywhere mentions that the bit pattern of NaN needed to be exact.
In the wasm-spec-test a test case may like this:
~~~
(assert_return (invoke "floor" (f32.const -nan:0x200000)) (f32.const nan:arithmetic))
~~~
The nan:arithmetic test case means that the bit pattern could be in a range with the QuietNanBit = 1.
In the NaN Propagation of wasm3.0 spec at https://webassembly.github.io/spec/core/exec/numerics.html#nan-propagation. It has been mentioned that "the payload is picked non-deterministically among all arithmetic NaNs; that is, its most significant bit is 1 and all others are unspecified.".
ctype volatile rval = static_cast<ctype>(reg); \Just to learn more: why these need to be `volatile`?
As static_cast was replaced by ReadRegister. The clang was able to change the (lval op rval) to (rval op lval) in release build. When the input of f32add get a +nan and a -nan as operand, the result has been changed from nan to -nan. After the i32.reinterpret_f32 we get a -1 not a 2147483647.
The test case may as follows:
~~~
(module
(func (export "test") (result i32)
(f32.reinterpret_i32 (i32.const 2147483647))
(f32.reinterpret_i32 (i32.const -1))
f32.add
(i32.reinterpret_f32)
)
)
~~~
But nan and -nan is both canonical NaN, these result is both accepted in Wasm3.0. This test adds a restriction on the WASM standard, that the sign of the result needs to be a precise result.
A case in wasm-spec-test as follows:
~~~
(assert_return (invoke "add" (f32.const nan) (f32.const -nan)) (f32.const nan:canonical))
~~~
The result nan:canonical means it can be both nan or -nan in wasm spec.
But v8 think the Wasm spec is pretty relaxed on this test case, so it has been added to cctest. To pass this test case, I simply add a volatile to prevent the swap of the operands. Maybe a inline asm will be better.
Related details
- Paolo Severini
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini voted Code-Review+1![]( "Open in Gerrit")
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 1 comment![]( "Open in Gerrit")
Thanks for the clarifications!
Related details
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
'test-run-wasm/RunWasmInterpreter_Float32Add*': [SKIP],Can you try to re-enable also the mjsunit test `'wasm/nan-constant'` which is disabled in test/mjsunit/mjsunit.status, line 978?
This test case hasn't been supported so I cann't enable it now.
In this test case, we get the result 0x7fe7a1b9 and the expected one is 0x7fa7a1b9.
When we interpreted and execute call_ref op, we just call into WasmToJSInterpreterWrapper. And then we call WasmFloat32ToNumber builtin to cast the f32 param to a number in JS. This cast like a static_cast in c++, and it changes the f32 from a signaling NaN to a quiet NaN. So we get a wrong result.
I find a issue at https://chromium-review.googlesource.com/c/v8/v8/+/6933682 talking about the same case like this. And I write a test case as follows:
~~~
# js file
const bytes = readbuffer(arguments[0]);
const mod = new WebAssembly.Module(bytes);
function check_f32_bits(val) {
let buffer = new ArrayBuffer(8);
let view = new DataView(buffer);
view.setFloat32(0, val);
let bits = new Uint8Array(buffer);
console.log(bits);
}const instance = new WebAssembly.Instance(mod, {
import: {
func: check_f32_bits,
},
});
const { test } = instance.exports;check_f32_bits(test());
~~~
~~~
;; wat file
(module
(func (import "import" "func") (param f32))
(func (export "test") (result f32)
f32.const nan:0x27a1b9
call 0
f32.const nan:0x27a1b9
return
)
)
~~~
In this case, drumbrake, liftoff and turbofan get the same result 0x7fe7a1b9.
But in the case in nan-constant, liftoff and turbofan can pass it. This maybe becauce the CallRef in liftoff and turbofan just a wasm to wasm call, with no WasmToJSWrapper in it. Maybe I can fix it in the next CL.
Related details
- Paolo Severini
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini added 1 comment![]( "Open in Gerrit")
- Clemens Backes
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 2 comments![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
2 comments
LGTM with a minor comment about a change in `wasm-interpreter.cc` which we otherwise don't care much about :)
ctype volatile rval = ReadRegister<ctype>(reg); \Is `volatile` here to prevent optimizing `lval op rval` into `rval op lval`? If so, please add a short comment (same below).
Related details
- Daniel Lehmann
- 王忠齐
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
Is `volatile` here to prevent optimizing `lval op rval` into `rval op lval`? If so, please add a short comment (same below).
- Clemens Backes
- Daniel Lehmann
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
- Daniel Lehmann
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted Code-Review+1![]( "Open in Gerrit")
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
王忠齐 (Gerrit)
王忠齐 added 1 comment![]( "Open in Gerrit")
Please help to CQ Dry Run.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Lehmann (Gerrit)
Daniel Lehmann voted and added 1 comment![]( "Open in Gerrit")
Votes added by Daniel Lehmann
| Code-Review | +1 |
1 comment
Related details
- Clemens Backes
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted and added 1 comment![]( "Open in Gerrit")
Votes added by Clemens Backes
| Code-Review | +1 |
1 comment
Let me know if I should click the button to land this.
Related details
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paolo Severini (Gerrit)
Paolo Severini voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- 王忠齐
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[wasm interpreter] Support Propagate Arithmetic NaN in wasm interpreter
The arithmetic NaNs has most significant bit is 1 and the others are
unspecified which is the same as QNaN in x86 and arm64. This is a good
news that less instructions are needed to change there result to a
arithmetic NaN.
- M src/wasm/interpreter/wasm-interpreter.cc
- M test/cctest/cctest.status
- M test/wasm-spec-tests/wasm-spec-tests.status
Code-Review: +1 by Daniel Lehmann, +1 by Clemens Backes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |