Fix a bug in the short-circuit logical operations in the toplevel

code generator.

Review URL: http://codereview.chromium.org/542105

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

diff --git a/src/ia32/fast-codegen-ia32.cc b/src/ia32/fast-codegen-ia32.cc
index 27cdf7b..26aab67 100644 (file)
--- a/src/ia32/fast-codegen-ia32.cc
+++ b/src/ia32/fast-codegen-ia32.cc
@@ -214,17 +214,24 @@ void FastCodeGenerator::Apply(Expression::Context context, Register reg) {
       }
       break;
 
-    case Expression::kValueTest:
-    case Expression::kTestValue:
-      // Push an extra copy of the value in case it's needed.
-      __ push(reg);
-      // Fall through.
-
     case Expression::kTest:
       // For simplicity we always test the accumulator register.
       if (!reg.is(result_register())) __ mov(result_register(), reg);
       DoTest(context);
       break;
+
+    case Expression::kValueTest:
+    case Expression::kTestValue:
+      if (!reg.is(result_register())) __ mov(result_register(), reg);
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
+      DoTest(context);
+      break;
   }
 }
 
@@ -251,6 +258,7 @@ void FastCodeGenerator::Apply(Expression::Context context, Slot* slot) {
     }
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       Move(result_register(), slot);
       DoTest(context);
       break;
@@ -258,7 +266,13 @@ void FastCodeGenerator::Apply(Expression::Context context, Slot* slot) {
     case Expression::kValueTest:
     case Expression::kTestValue:
       Move(result_register(), slot);
-      __ push(result_register());
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -285,6 +299,7 @@ void FastCodeGenerator::Apply(Expression::Context context, Literal* lit) {
       break;
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       __ mov(result_register(), lit->handle());
       DoTest(context);
       break;
@@ -292,7 +307,13 @@ void FastCodeGenerator::Apply(Expression::Context context, Literal* lit) {
     case Expression::kValueTest:
     case Expression::kTestValue:
       __ mov(result_register(), lit->handle());
-      __ push(result_register());
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -319,13 +340,21 @@ void FastCodeGenerator::ApplyTOS(Expression::Context context) {
       break;
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       __ pop(result_register());
       DoTest(context);
       break;
 
     case Expression::kValueTest:
     case Expression::kTestValue:
-      __ mov(result_register(), Operand(esp, 0));
+      switch (location_) {
+        case kAccumulator:
+          __ pop(result_register());
+          break;
+        case kStack:
+          __ mov(result_register(), Operand(esp, 0));
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -359,6 +388,7 @@ void FastCodeGenerator::DropAndApply(int count,
       break;
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       __ Drop(count);
       if (!reg.is(result_register())) __ mov(result_register(), reg);
       DoTest(context);
@@ -366,9 +396,17 @@ void FastCodeGenerator::DropAndApply(int count,
 
     case Expression::kValueTest:
     case Expression::kTestValue:
-      if (count > 1) __ Drop(count - 1);
-      if (!reg.is(result_register())) __ mov(result_register(), reg);
-      __ mov(Operand(esp, 0), result_register());
+      switch (location_) {
+        case kAccumulator:
+          __ Drop(count);
+          if (!reg.is(result_register())) __ mov(result_register(), reg);
+          break;
+        case kStack:
+          if (count > 1) __ Drop(count - 1);
+          __ mov(result_register(), reg);
+          __ mov(Operand(esp, 0), result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -440,18 +478,44 @@ void FastCodeGenerator::Apply(Expression::Context context,
 
 
 void FastCodeGenerator::DoTest(Expression::Context context) {
-  // The value to test is in the accumulator, and duplicated on the stack if
-  // necessary (for value/test and test/value contexts).
+  // The value to test is in the accumulator.  If the value might be needed
+  // on the stack (value/test and test/value contexts with a stack location
+  // desired), then the value is already duplicated on the stack.
   ASSERT_NE(NULL, true_label_);
   ASSERT_NE(NULL, false_label_);
 
-  // If there is a value on the stack, use a discard label for the
-  // value-is-unneeded branch in the inlined part of the test.
+  // In value/test and test/value expression contexts with stack as the
+  // desired location, there is already an extra value on the stack.  Use a
+  // label to discard it if unneeded.
   Label discard;
-  Label* if_true =
-      (context == Expression::kTestValue) ? &discard : true_label_;
-  Label* if_false =
-      (context == Expression::kValueTest) ? &discard : false_label_;
+  Label* if_true = true_label_;
+  Label* if_false = false_label_;
+  switch (context) {
+    case Expression::kUninitialized:
+    case Expression::kEffect:
+    case Expression::kValue:
+      UNREACHABLE();
+    case Expression::kTest:
+      break;
+    case Expression::kValueTest:
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          if_false = &discard;
+          break;
+      }
+      break;
+    case Expression::kTestValue:
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          if_true = &discard;
+          break;
+      }
+      break;
+  }
 
   // Emit the inlined tests assumed by the stub.
   __ cmp(result_register(), Factory::undefined_value());
@@ -466,6 +530,34 @@ void FastCodeGenerator::DoTest(Expression::Context context) {
   __ test(result_register(), Immediate(kSmiTagMask));
   __ j(zero, if_true);
 
+  // Save a copy of the value if it may be needed and isn't already saved.
+  switch (context) {
+    case Expression::kUninitialized:
+    case Expression::kEffect:
+    case Expression::kValue:
+      UNREACHABLE();
+    case Expression::kTest:
+      break;
+    case Expression::kValueTest:
+      switch (location_) {
+        case kAccumulator:
+          __ push(result_register());
+          break;
+        case kStack:
+          break;
+      }
+      break;
+    case Expression::kTestValue:
+      switch (location_) {
+        case kAccumulator:
+          __ push(result_register());
+          break;
+        case kStack:
+          break;
+      }
+      break;
+  }
+
   // Call the ToBoolean stub for all other cases.
   ToBooleanStub stub;
   __ push(result_register());

diff --git a/src/x64/fast-codegen-x64.cc b/src/x64/fast-codegen-x64.cc
index b87f03e..e4314b2 100644 (file)
--- a/src/x64/fast-codegen-x64.cc
+++ b/src/x64/fast-codegen-x64.cc
@@ -221,17 +221,24 @@ void FastCodeGenerator::Apply(Expression::Context context, Register reg) {
       }
       break;
 
-    case Expression::kValueTest:
-    case Expression::kTestValue:
-      // Push an extra copy of the value in case it's needed.
-      __ push(reg);
-      // Fall through.
-
     case Expression::kTest:
       // For simplicity we always test the accumulator register.
       if (!reg.is(result_register())) __ movq(result_register(), reg);
       DoTest(context);
       break;
+
+    case Expression::kValueTest:
+    case Expression::kTestValue:
+      if (!reg.is(result_register())) __ movq(result_register(), reg);
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
+      DoTest(context);
+      break;
   }
 }
 
@@ -265,7 +272,13 @@ void FastCodeGenerator::Apply(Expression::Context context, Slot* slot) {
     case Expression::kValueTest:
     case Expression::kTestValue:
       Move(result_register(), slot);
-      __ push(result_register());
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -298,7 +311,13 @@ void FastCodeGenerator::Apply(Expression::Context context, Literal* lit) {
     case Expression::kValueTest:
     case Expression::kTestValue:
       __ Move(result_register(), lit->handle());
-      __ push(result_register());
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          __ push(result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -331,7 +350,14 @@ void FastCodeGenerator::ApplyTOS(Expression::Context context) {
 
     case Expression::kValueTest:
     case Expression::kTestValue:
-      __ movq(result_register(), Operand(rsp, 0));
+      switch (location_) {
+        case kAccumulator:
+          __ pop(result_register());
+          break;
+        case kStack:
+          __ movq(result_register(), Operand(rsp, 0));
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -372,9 +398,17 @@ void FastCodeGenerator::DropAndApply(int count,
 
     case Expression::kValueTest:
     case Expression::kTestValue:
-      if (count > 1) __ Drop(count - 1);
-      if (!reg.is(result_register())) __ movq(result_register(), reg);
-      __ movq(Operand(rsp, 0), result_register());
+      switch (location_) {
+        case kAccumulator:
+          __ Drop(count);
+          if (!reg.is(result_register())) __ movq(result_register(), reg);
+          break;
+        case kStack:
+          if (count > 1) __ Drop(count - 1);
+          __ movq(result_register(), reg);
+          __ movq(Operand(rsp, 0), result_register());
+          break;
+      }
       DoTest(context);
       break;
   }
@@ -446,18 +480,44 @@ void FastCodeGenerator::Apply(Expression::Context context,
 
 
 void FastCodeGenerator::DoTest(Expression::Context context) {
-  // The value to test is in the accumulator, and duplicated on the stack if
-  // necessary (for value/test and test/value contexts).
+  // The value to test is in the accumulator.  If the value might be needed
+  // on the stack (value/test and test/value contexts with a stack location
+  // desired), then the value is already duplicated on the stack.
   ASSERT_NE(NULL, true_label_);
   ASSERT_NE(NULL, false_label_);
 
-  // If there is a value on the stack, use a discard label for the
-  // value-is-unneeded branch in the inlined part of the test.
+  // In value/test and test/value expression contexts with stack as the
+  // desired location, there is already an extra value on the stack.  Use a
+  // label to discard it if unneeded.
   Label discard;
-  Label* if_true =
-      (context == Expression::kTestValue) ? &discard : true_label_;
-  Label* if_false =
-      (context == Expression::kValueTest) ? &discard : false_label_;
+  Label* if_true = true_label_;
+  Label* if_false = false_label_;
+  switch (context) {
+    case Expression::kUninitialized:
+    case Expression::kEffect:
+    case Expression::kValue:
+      UNREACHABLE();
+    case Expression::kTest:
+      break;
+    case Expression::kValueTest:
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          if_false = &discard;
+          break;
+      }
+      break;
+    case Expression::kTestValue:
+      switch (location_) {
+        case kAccumulator:
+          break;
+        case kStack:
+          if_true = &discard;
+          break;
+      }
+      break;
+  }
 
   // Emit the inlined tests assumed by the stub.
   __ CompareRoot(result_register(), Heap::kUndefinedValueRootIndex);
@@ -472,6 +532,34 @@ void FastCodeGenerator::DoTest(Expression::Context context) {
   Condition is_smi = masm_->CheckSmi(result_register());
   __ j(is_smi, if_true);
 
+  // Save a copy of the value if it may be needed and isn't already saved.
+  switch (context) {
+    case Expression::kUninitialized:
+    case Expression::kEffect:
+    case Expression::kValue:
+      UNREACHABLE();
+    case Expression::kTest:
+      break;
+    case Expression::kValueTest:
+      switch (location_) {
+        case kAccumulator:
+          __ push(result_register());
+          break;
+        case kStack:
+          break;
+      }
+      break;
+    case Expression::kTestValue:
+      switch (location_) {
+        case kAccumulator:
+          __ push(result_register());
+          break;
+        case kStack:
+          break;
+      }
+      break;
+  }
+
   // Call the ToBoolean stub for all other cases.
   ToBooleanStub stub;
   __ push(result_register());

diff --git a/src/x64/virtual-frame-x64.h b/src/x64/virtual-frame-x64.h
index 88cf2bc..8e3e40f 100644 (file)
--- a/src/x64/virtual-frame-x64.h
+++ b/src/x64/virtual-frame-x64.h
@@ -343,7 +343,7 @@ class VirtualFrame : public ZoneObject {
   // of the frame.  Key and receiver are not dropped.
   Result CallKeyedStoreIC();
 
-  // Call call IC.  Arguments, reciever, and function name are found
+  // Call call IC.  Arguments, receiver, and function name are found
   // on top of the frame.  Function name slot is not dropped.  The
   // argument count does not include the receiver.
   Result CallCallIC(RelocInfo::Mode mode, int arg_count, int loop_nesting);