Fix bug in the count operation where we statically know the input is a smi.

Even if we know that the input to a count operation is a smi we still need to check if the result overflowed (and becomes a heap number).

Also fix the smi loop analysis to take two border cases correctly into account.

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

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

src/data-flow.cc

@@ -1117,6 +1117,12 @@ Variable* AssignedVariablesAnalyzer::FindSmiLoopVariable(ForStatement* stmt) {
   if (init_value < term_value && update->op() != Token::INC) return NULL;
   if (init_value > term_value && update->op() != Token::DEC) return NULL;
 
+  // Check that the update operation cannot overflow the smi range. This can
+  // occur in the two cases where the loop bound is equal to the largest or
+  // smallest smi.
+  if (update->op() == Token::INC && term_value == Smi::kMaxValue) return NULL;
+  if (update->op() == Token::DEC && term_value == Smi::kMinValue) return NULL;
+
   // Found a smi loop variable.
   return loop_var;
 }

src/ia32/codegen-ia32.cc

@@ -6684,48 +6684,52 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
     // Ensure the new value is writable.
     frame_->Spill(new_value.reg());
 
-    // In order to combine the overflow and the smi tag check, we need
-    // to be able to allocate a byte register.  We attempt to do so
-    // without spilling.  If we fail, we will generate separate overflow
-    // and smi tag checks.
-    //
-    // We allocate and clear the temporary byte register before
-    // performing the count operation since clearing the register using
-    // xor will clear the overflow flag.
-    Result tmp = allocator_->AllocateByteRegisterWithoutSpilling();
-    if (tmp.is_valid()) {
-      __ Set(tmp.reg(), Immediate(0));
+    Result tmp;
+    if (new_value.is_smi()) {
+      if (FLAG_debug_code) {
+        __ AbortIfNotSmi(new_value.reg(), "Operand not a smi");
+      }
+    } else {
+      // We don't know statically if the input is a smi.
+      // In order to combine the overflow and the smi tag check, we need
+      // to be able to allocate a byte register.  We attempt to do so
+      // without spilling.  If we fail, we will generate separate overflow
+      // and smi tag checks.
+      // We allocate and clear a temporary byte register before performing
+      // the count operation since clearing the register using xor will clear
+      // the overflow flag.
+      tmp = allocator_->AllocateByteRegisterWithoutSpilling();
+      if (tmp.is_valid()) {
+        __ Set(tmp.reg(), Immediate(0));
+      }
     }
 
-
     if (is_increment) {
       __ add(Operand(new_value.reg()), Immediate(Smi::FromInt(1)));
     } else {
       __ sub(Operand(new_value.reg()), Immediate(Smi::FromInt(1)));
     }
 
-    if (new_value.is_smi()) {
-      if (FLAG_debug_code) {
-        __ AbortIfNotSmi(new_value.reg(), "Argument not a smi");
-      }
-      if (tmp.is_valid()) tmp.Unuse();
+    DeferredCode* deferred = NULL;
+    if (is_postfix) {
+      deferred = new DeferredPostfixCountOperation(new_value.reg(),
+                                                   old_value.reg(),
+                                                   is_increment);
     } else {
-      DeferredCode* deferred = NULL;
-      if (is_postfix) {
-        deferred = new DeferredPostfixCountOperation(new_value.reg(),
-                                                     old_value.reg(),
-                                                     is_increment);
-      } else {
-        deferred = new DeferredPrefixCountOperation(new_value.reg(),
-                                                    is_increment);
-      }
+      deferred = new DeferredPrefixCountOperation(new_value.reg(),
+                                                  is_increment);
+    }
 
+    if (new_value.is_smi()) {
+      // In case we have a smi as input just check for overflow.
+      deferred->Branch(overflow);
+    } else {
       // If the count operation didn't overflow and the result is a valid
       // smi, we're done. Otherwise, we jump to the deferred slow-case
       // code.
+      // We combine the overflow and the smi tag check if we could
+      // successfully allocate a temporary byte register.
       if (tmp.is_valid()) {
-        // We combine the overflow and the smi tag check if we could
-        // successfully allocate a temporary byte register.
         __ setcc(overflow, tmp.reg());
         __ or_(Operand(tmp.reg()), new_value.reg());
         __ test(tmp.reg(), Immediate(kSmiTagMask));
@@ -6737,8 +6741,9 @@ void CodeGenerator::VisitCountOperation(CountOperation* node) {
         __ test(new_value.reg(), Immediate(kSmiTagMask));
         deferred->Branch(not_zero);
       }
-      deferred->BindExit();
     }
+    deferred->BindExit();
+
 
     // Postfix: store the old value in the allocated slot under the
     // reference.

test/mjsunit/compiler/loopcount.js

+// Copyright 2010 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
+//       with the distribution.
+//     * Neither the name of Google Inc. nor the names of its
+//       contributors may be used to endorse or promote products derived
+//       from this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+
+// Test postfix count operations with smis.
+
+function f1() { var x = 0x3fffffff; x++; return x; }
+assertEquals(0x40000000, f1());
+
+
+function f2() { var x = -0x40000000; x--; return x; }
+assertEquals(-0x40000001, f2());
+
+
+function f3(x) { x = x & 0x3fffffff; x++; return x; }
+assertEquals(0x40000000, f3(0x3fffffff));
+
+
+function f4() {
+  var i;
+  for (i = 0x3ffffffe; i <= 0x3fffffff; i++) {}
+  return i;
+}
+assertEquals(0x40000000, f4());
+
+
+function f5() {
+  var i;
+  for (i = -0x3fffffff; i >= -0x40000000; i--) {}
+  return i;
+}
+assertEquals(-0x40000001, f5());