Canonicalize NaNs on store to Fast(Float|Double) arrays

Canonicalize NaNs on store to Fast(Float|Double) arrays 
Also treat holey NaN coming from external float/double arrays correctly


BUG=2596
Committed: https://code.google.com/p/v8/source/detail?r=14094

[danno, 2013-03-28 08:49:39]

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:2619: return !UsesMustHandleHole();
Watch out. By returning false in UsesMustHandleHole, you will return true here
and the load will generate a hole check (I think). You also need to return false
if IsExternalArrayElementsKind is true.

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3053: return
HLoadKeyed::cast(value())->can_load_hole_nan();
I am not sure you need a separate predicate for this, I'm a bit torn. Since the
functionality is specific to the StoreKeyed, I think it's sufficient to just
have:

if (value()->IsLoadKeyed()) {
  return
IsExternalArrayElementsKind(HLoadKeyed::cast(value())->elements_kind());
}

[Dmitry Lomov (no reviews), 2013-03-28 10:01:27]

PTAL. I disagree with one comment (see below)

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:2619: return !UsesMustHandleHole();
Done, good catch.
On 2013/03/28 08:49:39, danno wrote:
> Watch out. By returning false in UsesMustHandleHole, you will return true here
> and the load will generate a hole check (I think). You also need to return
false
> if IsExternalArrayElementsKind is true.

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3053: return
HLoadKeyed::cast(value())->can_load_hole_nan();
On 2013/03/28 08:49:39, danno wrote:
> I am not sure you need a separate predicate for this, I'm a bit torn. Since
the
> functionality is specific to the StoreKeyed, I think it's sufficient to just
> have:
> 
> if (value()->IsLoadKeyed()) {
>   return
> IsExternalArrayElementsKind(HLoadKeyed::cast(value())->elements_kind());
> }

I strongly disagree. We have enough hidden dependencies in our instructions
already. What I have here is way more readable.

[danno, 2013-03-28 10:07:16]

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3053: return
HLoadKeyed::cast(value())->can_load_hole_nan();
On 2013/03/28 10:01:27, Dmitry Lomov (chromium) wrote:
> On 2013/03/28 08:49:39, danno wrote:
> > I am not sure you need a separate predicate for this, I'm a bit torn. Since
> the
> > functionality is specific to the StoreKeyed, I think it's sufficient to just
> > have:
> > 
> > if (value()->IsLoadKeyed()) {
> >   return
> > IsExternalArrayElementsKind(HLoadKeyed::cast(value())->elements_kind());
> > }
> 
> I strongly disagree. We have enough hidden dependencies in our instructions
> already. What I have here is way more readable.

Well, the name you have chose isn't correct, and it confuses me. It is perfectly
valid to load a hole_nan from a FixedDoubleArray, but your predicate doesn't
return true for that. In fact, you really do care that it is a external array...
that is the reason for the special handling, not that it returns hole_nan, so if
you split this up, you need to be more careful with what the predicate actually
means.

[Jakob Kummerow, 2013-03-28 10:17:38]

I have a few nits to add to the review party :-)

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3046: // these can load arbitrary representation of
NaN
nit: full stop at the end of the line, please.

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.h
File src/hydrogen-instructions.h (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.h#n...
src/hydrogen-instructions.h:5388: || elements_kind() ==
EXTERNAL_DOUBLE_ELEMENTS;
nit: 4 spaces indentation

https://codereview.chromium.org/12918028/diff/1/test/mjsunit/regress/regress-...
File test/mjsunit/regress/regress-2596.js (right):

https://codereview.chromium.org/12918028/diff/1/test/mjsunit/regress/regress-...
test/mjsunit/regress/regress-2596.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
nit: 2013

[Dmitry Lomov (no reviews), 2013-03-28 12:19:40]

PTAL

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/1/src/hydrogen-instructions.cc#...
src/hydrogen-instructions.cc:3053: return
HLoadKeyed::cast(value())->can_load_hole_nan();
On 2013/03/28 10:07:16, danno wrote:
> On 2013/03/28 10:01:27, Dmitry Lomov (chromium) wrote:
> > On 2013/03/28 08:49:39, danno wrote:
> > > I am not sure you need a separate predicate for this, I'm a bit torn.
Since
> > the
> > > functionality is specific to the StoreKeyed, I think it's sufficient to
just
> > > have:
> > > 
> > > if (value()->IsLoadKeyed()) {
> > >   return
> > > IsExternalArrayElementsKind(HLoadKeyed::cast(value())->elements_kind());
> > > }
> > 
> > I strongly disagree. We have enough hidden dependencies in our instructions
> > already. What I have here is way more readable.
> 
> Well, the name you have chose isn't correct, and it confuses me. It is
perfectly
> valid to load a hole_nan from a FixedDoubleArray, but your predicate doesn't
> return true for that. In fact, you really do care that it is a external
array...
> that is the reason for the special handling, not that it returns hole_nan, so
if
> you split this up, you need to be more careful with what the predicate
actually
> means.

Alright so we have had a little offline discussion. The true meaning of this
predicate is "the load can load a hole nan that should be interpreted as a
float/double value, and not a hole". On one hand it is really a new bit of
information that is not adequately conveyed by existing
UsesMustHandleHole/RequiresHoleCheck flags, which strengthens the case for
adding a new predicate on HLoadKeyed. On the other hand, as Daniel points out,
the uses of this predicate, at least in our current system, will really be
limited to HStoreKeyed::NeedsCanonicalization.

So I reluctantly agree to remove the predicate here, but if other reviewers have
different opinions, please let me know

https://codereview.chromium.org/12918028/diff/1/test/mjsunit/regress/regress-...
File test/mjsunit/regress/regress-2596.js (right):

https://codereview.chromium.org/12918028/diff/1/test/mjsunit/regress/regress-...
test/mjsunit/regress/regress-2596.js:1: // Copyright 2011 the V8 project
authors. All rights reserved.
On 2013/03/28 10:17:38, Jakob wrote:
> nit: 2013

Done.

[danno, 2013-03-28 12:30:39]

lgtm with a nit

https://codereview.chromium.org/12918028/diff/7002/src/hydrogen-instructions.cc
File src/hydrogen-instructions.cc (right):

https://codereview.chromium.org/12918028/diff/7002/src/hydrogen-instructions....
src/hydrogen-instructions.cc:3060: return load_elements_kind ==
EXTERNAL_FLOAT_ELEMENTS
Maybe add a predicate to elements-kind.h: IsExternalFloatOrDoubleElementsKind?

[Dmitry Lomov (no reviews), 2013-03-28 12:51:20]

On 2013/03/28 12:30:39, danno wrote:
> lgtm with a nit
> 
>
https://codereview.chromium.org/12918028/diff/7002/src/hydrogen-instructions.cc
> File src/hydrogen-instructions.cc (right):
> 
>
https://codereview.chromium.org/12918028/diff/7002/src/hydrogen-instructions....
> src/hydrogen-instructions.cc:3060: return load_elements_kind ==
> EXTERNAL_FLOAT_ELEMENTS
> Maybe add a predicate to elements-kind.h: IsExternalFloatOrDoubleElementsKind?
Single-use predicate, riiight...
Joking, it actually makes sense, will do.

[Dmitry Lomov (no reviews), 2013-03-28 13:30:24]

Committed patchset #3 manually as r14094 (presubmit successful).