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

src/hydrogen-instructions.cc

Index: src/hydrogen-instructions.cc
diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc
index 9b96cfe7ca6a1dcd19ef7394a001c60e6863b84c..c3d5b9dc01c57316b5447d931550732cc75fd42e 100644
--- a/src/hydrogen-instructions.cc
+++ b/src/hydrogen-instructions.cc
@@ -2590,6 +2590,10 @@ bool HLoadKeyed::UsesMustHandleHole() const {
     return false;
   }
 
+  if (IsExternalArrayElementsKind(elements_kind())) {
+    return false;
+  }
+
   if (hole_mode() == ALLOW_RETURN_HOLE) return true;
 
   if (IsFastDoubleElementsKind(elements_kind())) {
@@ -3037,10 +3041,18 @@ bool HStoreKeyed::NeedsCanonicalization() {
   // If value is an integer or smi or comes from the result of a keyed load or
   // constant then it is either be a non-hole value or in the case of a constant
   // the hole is only being stored explicitly: no need for canonicalization.
-  if (value()->IsLoadKeyed() || value()->IsConstant()) {
+  //
+  // The exception to that is keyed loads from external float or double arrays:
+  // these can load arbitrary representation of NaN

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

nit: full stop at the end of the line, please.

+
+  if (value()->IsConstant()) {
     return false;
   }
 
+  if (value()->IsLoadKeyed()) {
+    return HLoadKeyed::cast(value())->can_load_hole_nan();

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

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]

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]

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.

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

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

+  }
+
   if (value()->IsChange()) {
     if (HChange::cast(value())->from().IsInteger32()) {
       return false;