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

src/ia32/fast-codegen-ia32.cc

 // Copyright 2009 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 196 matching lines @@
       switch (location_) {
         case kAccumulator:
           if (!reg.is(result_register())) __ mov(result_register(), reg);
           break;
         case kStack:
           __ push(reg);
           break;
       }
       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;
   }
 }
 
 
 void FastCodeGenerator::Apply(Expression::Context context, Slot* slot) {
   switch (context) {
     case Expression::kUninitialized:
       UNREACHABLE();
     case Expression::kEffect:
       // Nothing to do.
       break;
     case Expression::kValue: {
       MemOperand slot_operand = EmitSlotSearch(slot, result_register());
       switch (location_) {
         case kAccumulator:
           __ mov(result_register(), slot_operand);
           break;
         case kStack:
           // Memory operands can be pushed directly.
           __ push(slot_operand);
           break;
       }
       break;
     }
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       Move(result_register(), slot);
       DoTest(context);
       break;
 
     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;
   }
 }
 
 
 void FastCodeGenerator::Apply(Expression::Context context, Literal* lit) {
   switch (context) {
     case Expression::kUninitialized:
       UNREACHABLE();
     case Expression::kEffect:
       // Nothing to do.
       break;
     case Expression::kValue:
       switch (location_) {
         case kAccumulator:
           __ mov(result_register(), lit->handle());
           break;
         case kStack:
           // Immediates can be pushed directly.
           __ push(Immediate(lit->handle()));
           break;
       }
       break;
 
     case Expression::kTest:
+      // For simplicity we always test the accumulator register.
       __ mov(result_register(), lit->handle());
       DoTest(context);
       break;
 
     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;
   }
 }
 
 
 void FastCodeGenerator::ApplyTOS(Expression::Context context) {
   switch (context) {
     case Expression::kUninitialized:
       UNREACHABLE();
 
     case Expression::kEffect:
       __ Drop(1);
       break;
 
     case Expression::kValue:
       switch (location_) {
         case kAccumulator:
           __ pop(result_register());
           break;
         case kStack:
           break;
       }
       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;
   }
 }
 
 
 void FastCodeGenerator::DropAndApply(int count,
                                      Expression::Context context,
                                      Register reg) {
   ASSERT(count > 0);
@@ skipping 13 matching lines @@
           if (!reg.is(result_register())) __ mov(result_register(), reg);
           break;
         case kStack:
           if (count > 1) __ Drop(count - 1);
           __ mov(Operand(esp, 0), reg);
           break;
       }
       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);
       break;
 
     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;
   }
 }
 
 
 void FastCodeGenerator::Apply(Expression::Context context,
                               Label* materialize_true,
                               Label* materialize_false) {
   switch (context) {
@@ skipping 51 matching lines @@
           __ push(Immediate(Factory::false_value()));
           break;
       }
       __ jmp(false_label_);
       break;
   }
 }
 
 
 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());
   __ j(equal, if_false);
   __ cmp(result_register(), Factory::true_value());
   __ j(equal, if_true);
   __ cmp(result_register(), Factory::false_value());
   __ j(equal, if_false);
   ASSERT_EQ(0, kSmiTag);
   __ test(result_register(), Operand(result_register()));
   __ j(zero, if_false);
   __ 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());
   __ CallStub(&stub);
   __ test(eax, Operand(eax));
 
   // The stub returns nonzero for true.  Complete based on the context.
   switch (context) {
     case Expression::kUninitialized:
     case Expression::kEffect:
@@ skipping 1232 matching lines @@
   __ add(Operand(edx), Immediate(masm_->CodeObject()));
   __ mov(Operand(esp, 0), edx);
   // And return.
   __ ret(0);
 }
 
 
 #undef __
 
 } }  // namespace v8::internal