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

src/x64/fast-codegen-x64.cc

Index: src/x64/fast-codegen-x64.cc
===================================================================
--- src/x64/fast-codegen-x64.cc	(revision 3635)
+++ src/x64/fast-codegen-x64.cc	(working copy)
@@ -221,17 +221,24 @@
       }
       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 @@
     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 @@
     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 @@
 
     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 @@
 
     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::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 @@
   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());