Only set binary operation side effects flags, when observable.

BUG=
R=verwaest@chromium.org

Review URL: https://codereview.chromium.org/26712002

git-svn-id: http://v8.googlecode.com/svn/branches/bleeding_edge@17147 ce2b1a6d-e550-0410-aec6-3dcde31c8c00

diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h
index 733347b..1a0fdb6 100644 (file)
--- a/src/hydrogen-instructions.h
+++ b/src/hydrogen-instructions.h
@@ -3947,11 +3947,12 @@ class HBitwiseBinaryOperation : public HBinaryOperation {
   }
 
   virtual void RepresentationChanged(Representation to) V8_OVERRIDE {
-    if (to.IsTagged()) {
+    if (to.IsTagged()) SetGVNFlag(kChangesNewSpacePromotion);
+    if (to.IsTagged() &&
+        (left()->ToNumberCanBeObserved() || right()->ToNumberCanBeObserved())) {
       SetAllSideEffects();
       ClearFlag(kUseGVN);
     } else {
-      ASSERT(to.IsSmiOrInteger32());
       ClearAllSideEffects();
       SetFlag(kUseGVN);
     }
@@ -4023,7 +4024,9 @@ class HArithmeticBinaryOperation : public HBinaryOperation {
   }
 
   virtual void RepresentationChanged(Representation to) V8_OVERRIDE {
-    if (to.IsTagged()) {
+    if (to.IsTagged()) SetGVNFlag(kChangesNewSpacePromotion);
+    if (to.IsTagged() &&
+        (left()->ToNumberCanBeObserved() || right()->ToNumberCanBeObserved())) {
       SetAllSideEffects();
       ClearFlag(kUseGVN);
     } else {
@@ -4562,8 +4565,19 @@ class HAdd V8_FINAL : public HArithmeticBinaryOperation {
   }
 
   virtual void RepresentationChanged(Representation to) V8_OVERRIDE {
-    if (to.IsTagged()) ClearFlag(kAllowUndefinedAsNaN);
-    HArithmeticBinaryOperation::RepresentationChanged(to);
+    if (to.IsTagged()) {
+      SetGVNFlag(kChangesNewSpacePromotion);
+      ClearFlag(kAllowUndefinedAsNaN);
+    }
+    if (to.IsTagged() &&
+        (left()->ToNumberCanBeObserved() || right()->ToNumberCanBeObserved() ||
+         left()->ToStringCanBeObserved() || right()->ToStringCanBeObserved())) {
+      SetAllSideEffects();
+      ClearFlag(kUseGVN);
+    } else {
+      ClearAllSideEffects();
+      SetFlag(kUseGVN);
+    }
   }
 
   DECLARE_CONCRETE_INSTRUCTION(Add)
@@ -6632,20 +6646,25 @@ class HStringAdd V8_FINAL : public HBinaryOperation {
   HStringAdd(HValue* context, HValue* left, HValue* right, StringAddFlags flags)
       : HBinaryOperation(context, left, right, HType::String()), flags_(flags) {
     set_representation(Representation::Tagged());
-    if (flags_ == STRING_ADD_CHECK_NONE) {
+    if (MightHaveSideEffects()) {
+      SetAllSideEffects();
+    } else {
       SetFlag(kUseGVN);
       SetGVNFlag(kDependsOnMaps);
       SetGVNFlag(kChangesNewSpacePromotion);
-    } else {
-      SetAllSideEffects();
     }
   }
 
+  bool MightHaveSideEffects() const {
+    return flags_ != STRING_ADD_CHECK_NONE &&
+      (left()->ToStringCanBeObserved() || right()->ToStringCanBeObserved());
+  }
+
   // No side-effects except possible allocation:
   // NOTE: this instruction does not call ToString() on its inputs, when flags_
   // is set to STRING_ADD_CHECK_NONE.
   virtual bool IsDeletable() const V8_OVERRIDE {
-    return flags_ == STRING_ADD_CHECK_NONE;
+    return !MightHaveSideEffects();
   }
 
   const StringAddFlags flags_;

diff --git a/test/mjsunit/regress/regress-binop.js b/test/mjsunit/regress/regress-binop.js
index 6050b6d..7a8b419 100644 (file)
--- a/test/mjsunit/regress/regress-binop.js
+++ b/test/mjsunit/regress/regress-binop.js
@@ -166,3 +166,16 @@ assertEquals(t2(undefined,2), NaN/2);
 assertEquals(t2(1,1<<30), 1/(1<<30));
 assertEquals(t2(1,2), 1/2);
 
+
+// Assert that the hole is not truncated to nan for string add.
+function string_add(a,i) {
+  var d = [0.1, ,0.3];
+  return a + d[i];
+}
+
+string_add(1.1, 0);
+string_add("", 0);
+%OptimizeFunctionOnNextCall(string_add);
+string_add(1.1, 0);
+// There comes the hole
+assertEquals("undefined", string_add("", 1));