[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2639353002
Cr-Original-Commit-Position: refs/heads/master@{#42512}
Committed: https://chromium.googlesource.com/v8/v8/+/7739affa5b57e0d28674d476f63de60d71728fb6
Review-Url: https://codereview.chromium.org/2639353002
Cr-Commit-Position: refs/heads/master@{#42545}
Committed: https://chromium.googlesource.com/v8/v8/+/ea9254317149c7904a7bf6bc821af35958e4da7f

[ahaas, 2017-01-18 17:53:14]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 17:53:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Clemens Hammacher, 2017-01-18 18:03:49]

lgtm

https://codereview.chromium.org/2639353002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2639353002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1556: // because on ia32 we may loose the
signalling bit of NaNs otherwise.
lose, not loose. I would also write signaling instead of signalling (there are
more occurences).

[ahaas, 2017-01-18 18:13:13]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 18:13:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-18 18:13:49]

https://codereview.chromium.org/2639353002/diff/1/src/wasm/wasm-interpreter.cc
File src/wasm/wasm-interpreter.cc (right):

https://codereview.chromium.org/2639353002/diff/1/src/wasm/wasm-interpreter.c...
src/wasm/wasm-interpreter.cc:1556: // because on ia32 we may loose the
signalling bit of NaNs otherwise.
On 2017/01/18 at 18:03:49, Clemens Hammacher wrote:
> lose, not loose. I would also write signaling instead of signalling (there are
more occurences).

I rewrote the comment.

[commit-bot: I haz the power, 2017-01-18 18:42:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 18:42:43]

Dry run: This issue passed the CQ dry run.

[titzer, 2017-01-19 13:13:01]

lgtm with comments

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:2109: uint32_t src =
bit_cast<uint32_t>(src_constant.ToFloat32AsInt());
I think you can remove the bit_cast now that this method returns an
int32_t...but you probably want it to return a uint32_t.

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:2120: uint64_t src =
bit_cast<uint64_t>(src_constant.ToFloat64AsInt());
Same here.

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/instructio...
src/compiler/instruction.h:1067: float ToFloat32() const {
Maybe we should add a TODO to remove these methods, since they can cause the
signaling bit to be unset, as you've discovered. At least add a comment.

https://codereview.chromium.org/2639353002/diff/20001/src/wasm/function-body-...
File src/wasm/function-body-decoder.h (right):

https://codereview.chromium.org/2639353002/diff/20001/src/wasm/function-body-...
src/wasm/function-body-decoder.h:73: // Do not use bit_cast to preserve a
potential signalling bit of a NaN.
Comment is a little misleading (since it might be parsed as meaning the
opposite).

Might read better as "avoid bit_cast because it might not preserve the signaling
bit of a NaN"

[ahaas, 2017-01-19 15:46:19]

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
File src/compiler/ia32/code-generator-ia32.cc (right):

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:2109: uint32_t src =
bit_cast<uint32_t>(src_constant.ToFloat32AsInt());
On 2017/01/19 at 13:13:01, titzer wrote:
> I think you can remove the bit_cast now that this method returns an
int32_t...but you probably want it to return a uint32_t.

Done.

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/ia32/code-...
src/compiler/ia32/code-generator-ia32.cc:2120: uint64_t src =
bit_cast<uint64_t>(src_constant.ToFloat64AsInt());
On 2017/01/19 at 13:13:01, titzer wrote:
> Same here.

Done.

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/instruction.h
File src/compiler/instruction.h (right):

https://codereview.chromium.org/2639353002/diff/20001/src/compiler/instructio...
src/compiler/instruction.h:1067: float ToFloat32() const {
On 2017/01/19 at 13:13:01, titzer wrote:
> Maybe we should add a TODO to remove these methods, since they can cause the
signaling bit to be unset, as you've discovered. At least add a comment.

Done.

https://codereview.chromium.org/2639353002/diff/20001/src/wasm/function-body-...
File src/wasm/function-body-decoder.h (right):

https://codereview.chromium.org/2639353002/diff/20001/src/wasm/function-body-...
src/wasm/function-body-decoder.h:73: // Do not use bit_cast to preserve a
potential signalling bit of a NaN.
On 2017/01/19 at 13:13:01, titzer wrote:
> Comment is a little misleading (since it might be parsed as meaning the
opposite).
> 
> Might read better as "avoid bit_cast because it might not preserve the
signaling bit of a NaN"

Done.

[ahaas, 2017-01-19 15:48:24]

The CQ bit was checked by ahaas@chromium.org

[ahaas, 2017-01-19 15:48:24]

The patchset sent to the CQ was uploaded after l-g-t-m from
clemensh@chromium.org, titzer@chromium.org
Link to the patchset: https://codereview.chromium.org/2639353002/#ps40001
(title: "Address comments")

[commit-bot: I haz the power, 2017-01-19 15:48:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 16:21:03]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484840904220660,
"parent_rev": "ca0f957329828c61f02437f640ed8004a549018a", "commit_rev":
"7739affa5b57e0d28674d476f63de60d71728fb6"}

[commit-bot: I haz the power, 2017-01-19 16:21:07]

Description was changed from

==========
[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org
==========

to

==========
[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2639353002
Cr-Commit-Position: refs/heads/master@{#42512}
Committed:
https://chromium.googlesource.com/v8/v8/+/7739affa5b57e0d28674d476f63de60d717...
==========

[commit-bot: I haz the power, 2017-01-19 16:21:08]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/v8/v8/+/7739affa5b57e0d28674d476f63de60d717...

[ahaas, 2017-01-19 17:00:48]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2645693003/ by ahaas@chromium.org.

The reason for reverting is: compilation problems on mips.

[ahaas, 2017-01-20 09:56:31]

The CQ bit was checked by ahaas@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 09:56:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ahaas, 2017-01-20 10:01:15]

On 2017/01/19 at 17:00:48, ahaas wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/2645693003/ by ahaas@chromium.org.
> 
> The reason for reverting is: compilation problems on mips.

PTAL.
Doing "value = *reinterpret_cast<double*>(&tmp);" is bad on certain 
compilers and may produce unexpected results on certain compiler. I 
replaced it now with a "memcpy(&value, &tmp, sizeof(value));",
which is more or less inlining the original bit_cast manually. All
changes after the revert are in function-body-decoder.h.

[titzer, 2017-01-20 10:02:38]

lgtm

[Clemens Hammacher, 2017-01-20 10:11:02]

lgtm

[commit-bot: I haz the power, 2017-01-20 10:27:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 10:27:58]

Dry run: This issue passed the CQ dry run.

[ahaas, 2017-01-20 10:39:34]

The CQ bit was checked by ahaas@chromium.org

[commit-bot: I haz the power, 2017-01-20 10:39:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-20 10:46:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484908774869150,
"parent_rev": "0ff07a5bfb26af1684816c522b6e5b2970fdbbbf", "commit_rev":
"ea9254317149c7904a7bf6bc821af35958e4da7f"}

[commit-bot: I haz the power, 2017-01-20 10:46:52]

Description was changed from

==========
[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2639353002
Cr-Commit-Position: refs/heads/master@{#42512}
Committed:
https://chromium.googlesource.com/v8/v8/+/7739affa5b57e0d28674d476f63de60d717...
==========

to

==========
[wasm] Fix I32ReinterpretF32 and I64ReinterpretF64 on ia32.

On ia32 return statements in C++ automatically convert signalling NaNs
to quiet NaNs, even when bit_cast is used. This CL removes all uses of
bit_cast<float> and bit_cast<double> in the wasm compiler and wasm
interpreter.

R=titzer@chromium.org, clemensh@chromium.org

Review-Url: https://codereview.chromium.org/2639353002
Cr-Original-Commit-Position: refs/heads/master@{#42512}
Committed:
https://chromium.googlesource.com/v8/v8/+/7739affa5b57e0d28674d476f63de60d717...
Review-Url: https://codereview.chromium.org/2639353002
Cr-Commit-Position: refs/heads/master@{#42545}
Committed:
https://chromium.googlesource.com/v8/v8/+/ea9254317149c7904a7bf6bc821af35958e...
==========

[commit-bot: I haz the power, 2017-01-20 10:46:53]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/v8/v8/+/ea9254317149c7904a7bf6bc821af35958e...