[v8-dev] [v8] r4061 committed - Correct handling of adding a string and a smal integer...

Tue, 09 Mar 2010 01:40:53 -0800 

Revision: 4061
Author: [email protected]
Date: Tue Mar  9 01:40:35 2010
Log: Correct handling of adding a string and a smal integer
The fast case of looking up the string convertion of the smi did not handle the case where left/reghe operands could be in eax/edx instead of edx/eax which is the default.
Also got rid of creating an internal frame for calling string and instead patched the argument on the stack and performed a tail call. 

BUG=http://code.google.com/p/v8/issues/detail?id=636
TEST=test/mjsunit/regress/regress-636.js
Review URL: http://codereview.chromium.org/720001
http://code.google.com/p/v8/source/detail?r=4061

Added:
 /branches/bleeding_edge/test/mjsunit/regress/regress-636.js
Modified:
 /branches/bleeding_edge/src/ia32/codegen-ia32.cc

=======================================
--- /dev/null
+++ /branches/bleeding_edge/test/mjsunit/regress/regress-636.js Tue Mar 9 01:40:35 2010 
@@ -0,0 +1,36 @@
+// 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.
+
+function test() {
+  var i, result = "";
+  var value = parseFloat(5.5);
+  value = Math.abs(1025);
+  for(i = 12; --i; result = ( value % 2 ) + result, value >>= 1);
+  return result;
+};
+
+assertEquals("10000000001", test());
=======================================
--- /branches/bleeding_edge/src/ia32/codegen-ia32.cc Mon Mar 8 05:23:54 2010 +++ /branches/bleeding_edge/src/ia32/codegen-ia32.cc Tue Mar 9 01:40:35 2010 
@@ -8564,15 +8564,26 @@
         GenerateLoadArguments(masm);
       }

-      __ test(edx, Immediate(kSmiTagMask));
+      // Registers containing left and right operands respectively.
+      Register lhs, rhs;
+      if (HasArgsReversed()) {
+        lhs = eax;
+        rhs = edx;
+      } else {
+        lhs = edx;
+        rhs = eax;
+      }
+
+      // Test if first argument is a string.
+      __ test(lhs, Immediate(kSmiTagMask));
       __ j(zero, &not_string1);
-      __ CmpObjectType(edx, FIRST_NONSTRING_TYPE, ecx);
+      __ CmpObjectType(lhs, FIRST_NONSTRING_TYPE, ecx);
       __ j(above_equal, &not_string1);

       // First argument is a string, test second.
-      __ test(eax, Immediate(kSmiTagMask));
+      __ test(rhs, Immediate(kSmiTagMask));
       __ j(zero, &string1_smi2);
-      __ CmpObjectType(eax, FIRST_NONSTRING_TYPE, ecx);
+      __ CmpObjectType(rhs, FIRST_NONSTRING_TYPE, ecx);
       __ j(above_equal, &string1);
// First and second argument are strings. Jump to the string add stub. 
@@ -8583,36 +8594,26 @@
// First argument is a string, second is a smi. Try to lookup the number 
       // string for the smi in the number string cache.
       NumberToStringStub::GenerateLookupNumberStringCache(
-          masm, eax, edi, ebx, ecx, true, &string1);
-
-      // Call the string add stub to make the result.
-      __ EnterInternalFrame();
-      __ push(edx);  // Original first argument.
-      __ push(edi);  // Number to string result for second argument.
-      __ CallStub(&string_add_stub);
-      __ LeaveInternalFrame();
-      __ ret(2 * kPointerSize);
-
+          masm, rhs, edi, ebx, ecx, true, &string1);
+
+ // Replace second argument on stack and tailcall string add stub to make 
+      // the result.
+      __ mov(Operand(esp, 1 * kPointerSize), edi);
+      __ TailCallStub(&string_add_stub);
+
+      // Only first argument is a string.
       __ bind(&string1);
-      __ InvokeBuiltin(
-          HasArgsReversed() ?
-              Builtins::STRING_ADD_RIGHT :
-              Builtins::STRING_ADD_LEFT,
-          JUMP_FUNCTION);
+      __ InvokeBuiltin(Builtins::STRING_ADD_LEFT, JUMP_FUNCTION);

       // First argument was not a string, test second.
       __ bind(&not_string1);
-      __ test(eax, Immediate(kSmiTagMask));
+      __ test(rhs, Immediate(kSmiTagMask));
       __ j(zero, &not_strings);
-      __ CmpObjectType(eax, FIRST_NONSTRING_TYPE, ecx);
+      __ CmpObjectType(rhs, FIRST_NONSTRING_TYPE, ecx);
       __ j(above_equal, &not_strings);

       // Only second argument is a string.
-      __ InvokeBuiltin(
-          HasArgsReversed() ?
-              Builtins::STRING_ADD_LEFT :
-              Builtins::STRING_ADD_RIGHT,
-          JUMP_FUNCTION);
+      __ InvokeBuiltin(Builtins::STRING_ADD_RIGHT, JUMP_FUNCTION);

       __ bind(&not_strings);
       // Neither argument is a string.

--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev

Reply via email to