From 9202e05016c8f68507e3cfc2da104c74913f29b8 Mon Sep 17 00:00:00 2001 From: "fschneider@chromium.org" Date: Tue, 16 Mar 2010 16:03:40 +0000 Subject: [PATCH] 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 | 6 +++ src/ia32/codegen-ia32.cc | 63 ++++++++++++++++-------------- test/mjsunit/compiler/loopcount.js | 55 ++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 29 deletions(-) create mode 100644 test/mjsunit/compiler/loopcount.js diff --git a/src/data-flow.cc b/src/data-flow.cc index 3ac952078..278b82bc6 100644 --- a/src/data-flow.cc +++ b/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; } diff --git a/src/ia32/codegen-ia32.cc b/src/ia32/codegen-ia32.cc index 7841304d7..b2e64cf98 100644 --- a/src/ia32/codegen-ia32.cc +++ b/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. diff --git a/test/mjsunit/compiler/loopcount.js b/test/mjsunit/compiler/loopcount.js new file mode 100644 index 000000000..d41eea1d5 --- /dev/null +++ b/test/mjsunit/compiler/loopcount.js @@ -0,0 +1,55 @@ +// 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()); -- 2.34.1