Use signaling NaN for holes in fixed double arrays.

Use signaling NaN for holes in fixed double arrays.

TEST=mjsunit,cctest,unittests
R=jkummerow@chromium.org

Committed: https://crrev.com/9eace97bbaab72962c0fda62e5f9011a10604d0d
Cr-Commit-Position: refs/heads/master@{#26180}

[Benedikt Meurer, 2015-01-20 13:06:34]

bmeurer@chromium.org changed reviewers:
+ jkummerow@chromium.org

[Benedikt Meurer, 2015-01-20 13:06:40]

PTAL

[Benedikt Meurer, 2015-01-20 13:09:39]

https://codereview.chromium.org/863633002/diff/40001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/863633002/diff/40001/src/arm/simulator-arm.cc...
src/arm/simulator-arm.cc:1920: // Default NaN value, see "NaN handling" in "IEEE
754 standard implementation
This is a drive-by-fix: The canonical_not_the_hole_nan is not the Default NaN
mentioned by the ARM reference manual.

[Jakob Kummerow, 2015-01-20 13:25:38]

LGTM with a comment.

https://codereview.chromium.org/863633002/diff/40001/test/mjsunit/regress/reg...
File test/mjsunit/regress/regress-undefined-nan.js (right):

https://codereview.chromium.org/863633002/diff/40001/test/mjsunit/regress/reg...
test/mjsunit/regress/regress-undefined-nan.js:13: i_view[0] = 0xFFFFFFFF;
You want to change this to the new hole-NaN now.

[Benedikt Meurer, 2015-01-20 13:27:05]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-20 13:27:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/100001

[jbramley, 2015-01-20 14:02:06]

jacob.bramley@arm.com changed reviewers:
+ jacob.bramley@arm.com

[jbramley, 2015-01-20 14:02:07]

Out of curiosity, what's the ultimate purpose for this? I wasn't aware that any
of V8's supported targets could do anything special with signalling NaNs.

https://codereview.chromium.org/863633002/diff/80001/src/arm64/lithium-codege...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/863633002/diff/80001/src/arm64/lithium-codege...
src/arm64/lithium-codegen-arm64.cc:3609: __ Cmp(scratch, Operand(kHoleNanInt64,
RelocInfo::NONE64));
I doubt that this will have a significant effect on performance, but it will add
a couple of instructions. (Probably movz + movk + cmp rather than just cmn.)
I'll run a quick benchmark to see if it has an impact; from memory, I think this
function is quite important for Kraken.

[Benedikt Meurer, 2015-01-20 14:04:14]

> Out of curiosity, what's the ultimate purpose for this? I wasn't aware that
any of V8's supported targets could do anything special with signalling NaNs.

With signaling NaN as hole, we can skip almost all the canonicalizations that
were required previously, leading to an overall 5% improvement on Kraken.

>
https://codereview.chromium.org/863633002/diff/80001/src/arm64/lithium-codege...
> src/arm64/lithium-codegen-arm64.cc:3609: __ Cmp(scratch,
Operand(kHoleNanInt64, RelocInfo::NONE64));
> I doubt that this will have a significant effect on performance, but it will
add a couple of instructions. (Probably movz + movk + cmp rather than just cmn.)
I'll run a quick benchmark to see if it has an impact; from memory, I think this
function is quite important for Kraken.

This is required for correctness now, because the kHoleNanInt64 changed. If you
can come up with a better sequence here, that'd be great.

[Benedikt Meurer, 2015-01-20 14:46:07]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-20 14:47:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/120001

[jbramley, 2015-01-20 14:57:22]

https://codereview.chromium.org/863633002/diff/80001/src/arm/simulator-arm.cc
File src/arm/simulator-arm.cc (right):

https://codereview.chromium.org/863633002/diff/80001/src/arm/simulator-arm.cc...
src/arm/simulator-arm.cc:1922: const uint64_t kDefaultNaN =
V8_UINT64_C(0x7FF8000000000000);
We already have kFP64DefaultNaN (in src/arm64/instructions-arm64.h).

https://codereview.chromium.org/863633002/diff/120001/src/arm64/lithium-codeg...
File src/arm64/lithium-codegen-arm64.cc (right):

https://codereview.chromium.org/863633002/diff/120001/src/arm64/lithium-codeg...
src/arm64/lithium-codegen-arm64.cc:3610: DeoptimizeIf(eq, instr, "hole");
Untested suggestion:

__ Ldr(scratch, mem_op);
__ Eor(scratch, scratch, kHoleNaNInt64);
__ DeoptimizeIfZero(scratch, instr, "hole");

It should assemble to just three instructions, since kHoleNaNInt64 is
representable as a logical immediate:

  ldr x16, [mem_op]
  eor x16, x16, #0x7ff000007ff00000
  cbz x16, deopt_label

[Benedikt Meurer, 2015-01-20 15:18:18]

The CQ bit was unchecked by bmeurer@chromium.org

[Benedikt Meurer, 2015-01-20 15:18:22]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-20 15:18:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/120001

[jbramley, 2015-01-20 15:22:54]

On 2015/01/20 14:57:22, jbramley wrote:
> https://codereview.chromium.org/863633002/diff/80001/src/arm/simulator-arm.cc
> File src/arm/simulator-arm.cc (right):
> 
>
https://codereview.chromium.org/863633002/diff/80001/src/arm/simulator-arm.cc...
> src/arm/simulator-arm.cc:1922: const uint64_t kDefaultNaN =
> V8_UINT64_C(0x7FF8000000000000);
> We already have kFP64DefaultNaN (in src/arm64/instructions-arm64.h).
> 
>
https://codereview.chromium.org/863633002/diff/120001/src/arm64/lithium-codeg...
> File src/arm64/lithium-codegen-arm64.cc (right):
> 
>
https://codereview.chromium.org/863633002/diff/120001/src/arm64/lithium-codeg...
> src/arm64/lithium-codegen-arm64.cc:3610: DeoptimizeIf(eq, instr, "hole");
> Untested suggestion:
> 
> __ Ldr(scratch, mem_op);
> __ Eor(scratch, scratch, kHoleNaNInt64);
> __ DeoptimizeIfZero(scratch, instr, "hole");
> 
> It should assemble to just three instructions, since kHoleNaNInt64 is
> representable as a logical immediate:
> 
>   ldr x16, [mem_op]
>   eor x16, x16, #0x7ff000007ff00000
>   cbz x16, deopt_label

By the way, I'd be happy to produce a follow-up patch to make these changes; it
doesn't need to hold up this one.

[commit-bot: I haz the power, 2015-01-20 17:03:38]

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

[commit-bot: I haz the power, 2015-01-20 17:03:40]

Try jobs failed on following builders:
  v8_win_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/2426)

[Benedikt Meurer, 2015-01-20 18:57:08]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-20 18:58:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/140001

[commit-bot: I haz the power, 2015-01-20 19:30:52]

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

[commit-bot: I haz the power, 2015-01-20 19:30:53]

Try jobs failed on following builders:
  v8_win_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel/builds/2431)

[Benedikt Meurer, 2015-01-20 20:16:36]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-20 20:17:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/160001

[commit-bot: I haz the power, 2015-01-20 20:19:42]

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

[commit-bot: I haz the power, 2015-01-20 20:19:43]

Try jobs failed on following builders:
  v8_mac_rel on tryserver.v8
(http://build.chromium.org/p/tryserver.v8/builders/v8_mac_rel/builds/2404)

[Benedikt Meurer, 2015-01-21 08:37:30]

The CQ bit was checked by bmeurer@chromium.org

[commit-bot: I haz the power, 2015-01-21 08:37:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/863633002/180001

[Benedikt Meurer, 2015-01-21 08:52:31]

Committed patchset #11 (id:200001) manually as
9eace97bbaab72962c0fda62e5f9011a10604d0d (presubmit successful).

[commit-bot: I haz the power, 2015-01-21 08:52:32]

Patchset 11 (id:??) landed as
https://crrev.com/9eace97bbaab72962c0fda62e5f9011a10604d0d
Cr-Commit-Position: refs/heads/master@{#26180}

[Vyacheslav Egorov (Google), 2015-01-22 02:52:18]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2015-01-22 02:52:20]

DBC (there's a bug, see below)

(nice sNaN idea!)

https://codereview.chromium.org/863633002/diff/200001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/863633002/diff/200001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:4056: switch (value()->opcode()) {
This check should account for heap number fields (mutable boxes, unboxed fields,
etc).

There we just load payload directly bypassing the HChange.

Repro:

function A(src) { this.x = src[0]; }

new A(fixed_double_elements);
new A(fixed_double_elements);

%OptimizeFunctionOnNextCall(A);

var obj = new A(fixed_double_elements);

function move_x(dst, obj) { dst[0] = obj.x; }

var doubles = [0.5];
move_x(doubles, obj);
move_x(doubles, obj);
%OptimizeFunctionOnNextCall(move_x);
move_x(doubles, obj);
assertTrue(doubles[0] !== undefined);

https://codereview.chromium.org/863633002/diff/200001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-undefined-nan.js (right):

https://codereview.chromium.org/863633002/diff/200001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-undefined-nan.js:18: fixed_double_elements[0] =
f_view[0];
This regression test does not necessarily work as is on ia32. This statement has
to be inside an optimized function to ensure transfer through xmm registers.

If this hits runtime and uses FPU registers for transfer somewhere (e.g. return
value of a function call).

If I add here:

var i32 = new Int32Array(fixed_double_elements.buffer);
assertEquals(i_view[0], i32[0]);
assertEquals(i_view[1], i32[1]);

to verify that test tests what it wants - this fails on ia32.debug build.

Need smth like:

function opt_store() { fixed_double_elements[0] = f_view[0]; }

opt_store();
opt_store();
%OptimizeFunctionOnNextCall(opt_store);
opt_store();

to fix it.

[Benedikt Meurer, 2015-01-22 06:11:15]

Thanks a lot, fixed those.

https://codereview.chromium.org/863633002/diff/200001/src/hydrogen-instructio...
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/863633002/diff/200001/src/hydrogen-instructio...
src/hydrogen-instructions.cc:4056: switch (value()->opcode()) {
Fixed, thanks.

https://codereview.chromium.org/863633002/diff/200001/test/mjsunit/regress/re...
File test/mjsunit/regress/regress-undefined-nan.js (right):

https://codereview.chromium.org/863633002/diff/200001/test/mjsunit/regress/re...
test/mjsunit/regress/regress-undefined-nan.js:18: fixed_double_elements[0] =
f_view[0];
On 2015/01/22 02:52:20, Vyacheslav Egorov (Google) wrote:
> This regression test does not necessarily work as is on ia32. This statement
has
> to be inside an optimized function to ensure transfer through xmm registers.
> 
> If this hits runtime and uses FPU registers for transfer somewhere (e.g.
return
> value of a function call).
> 
> If I add here:
> 
> var i32 = new Int32Array(fixed_double_elements.buffer);
> assertEquals(i_view[0], i32[0]);
> assertEquals(i_view[1], i32[1]);
> 
> to verify that test tests what it wants - this fails on ia32.debug build.
> 
> Need smth like:
> 
> function opt_store() { fixed_double_elements[0] = f_view[0]; }
> 
> opt_store();
> opt_store();
> %OptimizeFunctionOnNextCall(opt_store);
> opt_store();
> 
> to fix it. 

Done.