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

src/hydrogen-instructions.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 2572 matching lines @@
     stream->Add(" check_hole");
   }
 }
 
 
 bool HLoadKeyed::UsesMustHandleHole() const {
   if (IsFastPackedElementsKind(elements_kind())) {
     return false;
   }
 
+  if (IsExternalArrayElementsKind(elements_kind())) {
+    return false;
+  }
+
   if (hole_mode() == ALLOW_RETURN_HOLE) return true;
 
   if (IsFastDoubleElementsKind(elements_kind())) {
     return false;
   }
 
   for (HUseIterator it(uses()); !it.Done(); it.Advance()) {
     HValue* use = it.value();
     if (!use->IsChange()) {
       return false;
     }
   }
 
   return true;
 }
 
 
 bool HLoadKeyed::RequiresHoleCheck() const {
   if (IsFastPackedElementsKind(elements_kind())) {
     return false;
   }
 
   return !UsesMustHandleHole();

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

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. 

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

Done, good catch.
On 2013/03/28 08:49:39, danno wrote:

 }
 
 
 void HLoadKeyedGeneric::PrintDataTo(StringStream* stream) {
   object()->PrintNameTo(stream);
   stream->Add("[");
   key()->PrintNameTo(stream);
   stream->Add("]");
 }
 
@@ skipping 404 matching lines @@
     return left();
   }
   return NULL;
 }
 
 
 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;
     }
     if (HChange::cast(value())->value()->type().IsSmi()) {
       return false;
     }
   }
   return true;
 }
@@ skipping 465 matching lines @@
 
 
 void HCheckFunction::Verify() {
   HInstruction::Verify();
   ASSERT(HasNoUses());
 }
 
 #endif
 
 } }  // namespace v8::internal