[array] Use CallCFunction3 for SmiLexicographicCompare

This CL changes the call-site of SmiLexicographicCompare to a fast
c call instead of a runtime call. The runtime function is not deleted
as it is still used in InnerArraySort.

The test is also moved from mjsunit to cctest, to make removal of the
runtime function easier in the future.

R=cbruni@chromium.org, jgruber@chromium.org

Bug: v8:7382
Change-Id: Ie961eeb094c13018e9ec28b68f7c444d7f889036
Reviewed-on: https://chromium-review.googlesource.com/1201587
Commit-Queue: Simon Zünd <szuend@google.com>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#55642}

src/builtins/base.tq

@@ -220,7 +220,7 @@ extern runtime StringEqual(Context, String, String): Oddball;
 extern builtin StringLessThan(Context, String, String): Boolean;
 
 extern macro StrictEqual(Object, Object): Boolean;
-extern runtime SmiLexicographicCompare(Context, Object, Object): Number;
+extern macro SmiLexicographicCompare(Smi, Smi): Smi;
 
 extern operator '<' macro Int32LessThan(int32, int32): bool;
 extern operator '>' macro Int32GreaterThan(int32, int32): bool;

src/code-stub-assembler.cc

@@ -940,6 +940,17 @@ TNode<Smi> CodeStubAssembler::TrySmiDiv(TNode<Smi> dividend, TNode<Smi> divisor,
   return SmiFromInt32(untagged_result);
 }
 
+TNode<Smi> CodeStubAssembler::SmiLexicographicCompare(TNode<Smi> x,
+                                                      TNode<Smi> y) {
+  TNode<ExternalReference> smi_lexicographic_compare =
+      ExternalConstant(ExternalReference::smi_lexicographic_compare_function());
+  TNode<ExternalReference> isolate_ptr =
+      ExternalConstant(ExternalReference::isolate_address(isolate()));
+  return CAST(CallCFunction3(MachineType::AnyTagged(), MachineType::Pointer(),
+                             MachineType::AnyTagged(), MachineType::AnyTagged(),
+                             smi_lexicographic_compare, isolate_ptr, x, y));
+}
+
 TNode<Int32T> CodeStubAssembler::TruncateIntPtrToInt32(
     SloppyTNode<IntPtrT> value) {
   if (Is64()) {

src/code-stub-assembler.h

@@ -596,6 +596,13 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // if the division needs to be performed as a floating point operation.
   TNode<Smi> TrySmiDiv(TNode<Smi> dividend, TNode<Smi> divisor, Label* bailout);
 
+  // Compares two Smis a and b as if they were converted to strings and then
+  // compared lexicographically. Returns:
+  // -1 iff x < y.
+  //  0 iff x == y.
+  //  1 iff x > y.
+  TNode<Smi> SmiLexicographicCompare(TNode<Smi> x, TNode<Smi> y);
+
   // Smi | HeapNumber operations.
   TNode<Number> NumberInc(SloppyTNode<Number> value);
   TNode<Number> NumberDec(SloppyTNode<Number> value);

src/external-reference.cc

@@ -786,6 +786,10 @@ ExternalReference ExternalReference::try_internalize_string_function() {
       Redirect(FUNCTION_ADDR(StringTable::LookupStringIfExists_NoAllocate)));
 }
 
+ExternalReference ExternalReference::smi_lexicographic_compare_function() {
+  return ExternalReference(Redirect(FUNCTION_ADDR(Smi::LexicographicCompare)));
+}
+
 ExternalReference ExternalReference::check_object_type() {
   return ExternalReference(Redirect(FUNCTION_ADDR(CheckObjectType)));
 }

src/external-reference.h

@@ -140,6 +140,7 @@ class StatsCounter;
   V(search_string_raw_two_one, "search_string_raw_two_one")                   \
   V(search_string_raw_two_two, "search_string_raw_two_two")                   \
   V(try_internalize_string_function, "try_internalize_string_function")       \
+  V(smi_lexicographic_compare_function, "smi_lexicographic_compare_function") \
   V(wasm_call_trap_callback_for_testing,                                      \
     "wasm::call_trap_callback_for_testing")                                   \
   V(wasm_f32_ceil, "wasm::f32_ceil_wrapper")                                  \

src/objects.cc

@@ -18843,6 +18843,82 @@ MaybeHandle<Name> FunctionTemplateInfo::TryGetCachedPropertyName(
   return MaybeHandle<Name>();
 }
 
+Smi* Smi::LexicographicCompare(Isolate* isolate, Smi* x, Smi* y) {
+  DisallowHeapAllocation no_allocation;
+  DisallowJavascriptExecution no_js(isolate);
+
+  int x_value = Smi::ToInt(x);
+  int y_value = Smi::ToInt(y);
+
+  // If the integers are equal so are the string representations.
+  if (x_value == y_value) return Smi::FromInt(0);
+
+  // If one of the integers is zero the normal integer order is the
+  // same as the lexicographic order of the string representations.
+  if (x_value == 0 || y_value == 0)
+    return Smi::FromInt(x_value < y_value ? -1 : 1);
+
+  // If only one of the integers is negative the negative number is
+  // smallest because the char code of '-' is less than the char code
+  // of any digit.  Otherwise, we make both values positive.
+
+  // Use unsigned values otherwise the logic is incorrect for -MIN_INT on
+  // architectures using 32-bit Smis.
+  uint32_t x_scaled = x_value;
+  uint32_t y_scaled = y_value;
+  if (x_value < 0 || y_value < 0) {
+    if (y_value >= 0) return Smi::FromInt(-1);
+    if (x_value >= 0) return Smi::FromInt(1);
+    x_scaled = -x_value;
+    y_scaled = -y_value;
+  }
+
+  // clang-format off
+  static const uint32_t kPowersOf10[] = {
+      1,                 10,                100,         1000,
+      10 * 1000,         100 * 1000,        1000 * 1000, 10 * 1000 * 1000,
+      100 * 1000 * 1000, 1000 * 1000 * 1000};
+  // clang-format on
+
+  // If the integers have the same number of decimal digits they can be
+  // compared directly as the numeric order is the same as the
+  // lexicographic order.  If one integer has fewer digits, it is scaled
+  // by some power of 10 to have the same number of digits as the longer
+  // integer.  If the scaled integers are equal it means the shorter
+  // integer comes first in the lexicographic order.
+
+  // From http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
+  int x_log2 = 31 - base::bits::CountLeadingZeros(x_scaled);
+  int x_log10 = ((x_log2 + 1) * 1233) >> 12;
+  x_log10 -= x_scaled < kPowersOf10[x_log10];
+
+  int y_log2 = 31 - base::bits::CountLeadingZeros(y_scaled);
+  int y_log10 = ((y_log2 + 1) * 1233) >> 12;
+  y_log10 -= y_scaled < kPowersOf10[y_log10];
+
+  int tie = 0;
+
+  if (x_log10 < y_log10) {
+    // X has fewer digits.  We would like to simply scale up X but that
+    // might overflow, e.g when comparing 9 with 1_000_000_000, 9 would
+    // be scaled up to 9_000_000_000. So we scale up by the next
+    // smallest power and scale down Y to drop one digit. It is OK to
+    // drop one digit from the longer integer since the final digit is
+    // past the length of the shorter integer.
+    x_scaled *= kPowersOf10[y_log10 - x_log10 - 1];
+    y_scaled /= 10;
+    tie = -1;
+  } else if (y_log10 < x_log10) {
+    y_scaled *= kPowersOf10[x_log10 - y_log10 - 1];
+    x_scaled /= 10;
+    tie = 1;
+  }
+
+  if (x_scaled < y_scaled) return Smi::FromInt(-1);
+  if (x_scaled > y_scaled) return Smi::FromInt(1);
+  return Smi::FromInt(tie);
+}
+
 // Force instantiation of template instances class.
 // Please note this list is compiler dependent.
 // Keep this at the end of this file

src/objects.h

@@ -1588,6 +1588,13 @@ class Smi: public Object {
     return result;
   }
 
+  // Compare two Smis x, y as if they were converted to strings and then
+  // compared lexicographically. Returns:
+  // -1 if x < y.
+  //  0 if x == y.
+  //  1 if x > y.
+  static Smi* LexicographicCompare(Isolate* isolate, Smi* x, Smi* y);
+
   DECL_CAST(Smi)
 
   // Dispatched behavior.

src/runtime/runtime-numbers.cc

@@ -82,80 +82,16 @@ RUNTIME_FUNCTION(Runtime_NumberToString) {
 // -1 if x < y
 //  0 if x == y
 //  1 if x > y
+// TODO(szuend): Remove once the call-site in src/js/array.js is gone.
 RUNTIME_FUNCTION(Runtime_SmiLexicographicCompare) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(2, args.length());
-  CONVERT_SMI_ARG_CHECKED(x_value, 0);
-  CONVERT_SMI_ARG_CHECKED(y_value, 1);
-
-  // If the integers are equal so are the string representations.
-  if (x_value == y_value) return Smi::FromInt(0);
-
-  // If one of the integers is zero the normal integer order is the
-  // same as the lexicographic order of the string representations.
-  if (x_value == 0 || y_value == 0)
-    return Smi::FromInt(x_value < y_value ? -1 : 1);
-
-  // If only one of the integers is negative the negative number is
-  // smallest because the char code of '-' is less than the char code
-  // of any digit.  Otherwise, we make both values positive.
-
-  // Use unsigned values otherwise the logic is incorrect for -MIN_INT on
-  // architectures using 32-bit Smis.
-  uint32_t x_scaled = x_value;
-  uint32_t y_scaled = y_value;
-  if (x_value < 0 || y_value < 0) {
-    if (y_value >= 0) return Smi::FromInt(-1);
-    if (x_value >= 0) return Smi::FromInt(1);
-    x_scaled = -x_value;
-    y_scaled = -y_value;
-  }
-
-  static const uint32_t kPowersOf10[] = {
-      1,                 10,                100,         1000,
-      10 * 1000,         100 * 1000,        1000 * 1000, 10 * 1000 * 1000,
-      100 * 1000 * 1000, 1000 * 1000 * 1000};
-
-  // If the integers have the same number of decimal digits they can be
-  // compared directly as the numeric order is the same as the
-  // lexicographic order.  If one integer has fewer digits, it is scaled
-  // by some power of 10 to have the same number of digits as the longer
-  // integer.  If the scaled integers are equal it means the shorter
-  // integer comes first in the lexicographic order.
-
-  // From http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10
-  int x_log2 = 31 - base::bits::CountLeadingZeros(x_scaled);
-  int x_log10 = ((x_log2 + 1) * 1233) >> 12;
-  x_log10 -= x_scaled < kPowersOf10[x_log10];
-
-  int y_log2 = 31 - base::bits::CountLeadingZeros(y_scaled);
-  int y_log10 = ((y_log2 + 1) * 1233) >> 12;
-  y_log10 -= y_scaled < kPowersOf10[y_log10];
-
-  int tie = 0;
-
-  if (x_log10 < y_log10) {
-    // X has fewer digits.  We would like to simply scale up X but that
-    // might overflow, e.g when comparing 9 with 1_000_000_000, 9 would
-    // be scaled up to 9_000_000_000. So we scale up by the next
-    // smallest power and scale down Y to drop one digit. It is OK to
-    // drop one digit from the longer integer since the final digit is
-    // past the length of the shorter integer.
-    x_scaled *= kPowersOf10[y_log10 - x_log10 - 1];
-    y_scaled /= 10;
-    tie = -1;
-  } else if (y_log10 < x_log10) {
-    y_scaled *= kPowersOf10[x_log10 - y_log10 - 1];
-    x_scaled /= 10;
-    tie = 1;
-  }
+  CONVERT_ARG_CHECKED(Smi, x_value, 0);
+  CONVERT_ARG_CHECKED(Smi, y_value, 1);
 
-  if (x_scaled < y_scaled) return Smi::FromInt(-1);
-  if (x_scaled > y_scaled) return Smi::FromInt(1);
-  return Smi::FromInt(tie);
+  return Smi::LexicographicCompare(isolate, x_value, y_value);
 }
 
-
 RUNTIME_FUNCTION(Runtime_MaxSmi) {
   SealHandleScope shs(isolate);
   DCHECK_EQ(0, args.length());
@@ -184,6 +120,5 @@ RUNTIME_FUNCTION(Runtime_GetHoleNaNLower) {
   return *isolate->factory()->NewNumberFromUint(kHoleNanLower32);
 }
 
-
 }  // namespace internal
 }  // namespace v8

src/runtime/runtime.h

@@ -773,6 +773,8 @@ enum class OptimizationStatus {
   kTopmostFrameIsTurboFanned = 1 << 11,
 };
 
+Smi* SmiLexicographicCompare(Smi* x_value, Smi* y_value);
+
 }  // namespace internal
 }  // namespace v8
 

test/cctest/BUILD.gn

@@ -215,6 +215,7 @@ v8_source_set("cctest_sources") {
     "test-roots.cc",
     "test-sampler-api.cc",
     "test-serialize.cc",
+    "test-smi-lexicographic-compare.cc",
     "test-strings.cc",
     "test-strtod.cc",
     "test-symbols.cc",

test/cctest/test-smi-lexicographic-compare.cc

+// Copyright 2018 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <set>
+
+#include "src/objects-inl.h"
+#include "src/v8.h"
+#include "test/cctest/cctest.h"
+
+namespace v8 {
+namespace internal {
+
+namespace {
+
+void AddSigned(std::set<Smi*>& smis, int64_t x) {
+  if (!Smi::IsValid(x)) return;
+
+  smis.insert(Smi::FromInt(static_cast<int>(x)));
+  smis.insert(Smi::FromInt(static_cast<int>(-x)));
+}
+
+// Uses std::lexicographical_compare twice to convert the result to -1, 0 or 1.
+int ExpectedCompareResult(Smi* a, Smi* b) {
+  std::string str_a = std::to_string(a->value());
+  std::string str_b = std::to_string(b->value());
+  bool expected_a_lt_b = std::lexicographical_compare(
+      str_a.begin(), str_a.end(), str_b.begin(), str_b.end());
+  bool expected_b_lt_a = std::lexicographical_compare(
+      str_b.begin(), str_b.end(), str_a.begin(), str_a.end());
+
+  if (!expected_a_lt_b && !expected_b_lt_a) {
+    return 0;
+  } else if (expected_a_lt_b) {
+    return -1;
+  } else {
+    CHECK(expected_b_lt_a);
+    return 1;
+  }
+}
+
+bool Test(Isolate* isolate, Smi* a, Smi* b) {
+  int actual = Smi::LexicographicCompare(isolate, a, b)->value();
+  int expected = ExpectedCompareResult(a, b);
+
+  return actual == expected;
+}
+
+}  // namespace
+
+TEST(TestSmiLexicographicCompare) {
+  Isolate* isolate = CcTest::InitIsolateOnce();
+  HandleScope scope(isolate);
+
+  std::set<Smi*> smis;
+
+  for (int64_t xb = 1; xb <= Smi::kMaxValue; xb *= 10) {
+    for (int64_t xf = 0; xf <= 9; ++xf) {
+      for (int64_t xo = -1; xo <= 1; ++xo) {
+        AddSigned(smis, xb * xf + xo);
+      }
+    }
+  }
+
+  for (int64_t yb = 1; yb <= Smi::kMaxValue; yb *= 2) {
+    for (int64_t yo = -2; yo <= 2; ++yo) {
+      AddSigned(smis, yb + yo);
+    }
+  }
+
+  for (Smi* a : smis) {
+    for (Smi* b : smis) {
+      CHECK(Test(isolate, a, b));
+    }
+  }
+}
+
+}  // namespace internal
+}  // namespace v8

test/mjsunit/lexicographic-compare.js

-// Copyright 2018 the V8 project authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-// Flags: --allow-natives-syntax
-
-(function () {
-
-  assertFalse(%IsSmi(2147483648), 'Update test for >32 bit Smi');
-
-  // Collect a list of interesting Smis.
-  const seen = {};
-  const smis = [];
-  function add(x) {
-    if (x | 0 == x) {
-      x = x | 0;  // Canonicalizes to Smi if 32-bit signed and fits in Smi.
-    }
-    if (%_IsSmi(x) && !seen[x]) {
-      seen[x] = 1;
-      smis.push(x);
-    }
-  }
-  function addSigned(x) {
-    add(x);
-    add(-x);
-  }
-
-  var BIGGER_THAN_ANY_SMI = 10 * 1000 * 1000 * 1000;
-  for (var xb = 1; xb <= BIGGER_THAN_ANY_SMI; xb *= 10) {
-    for (var xf = 0; xf <= 9; xf++) {
-      for (var xo = -1; xo <= 1; xo++) {
-        addSigned(xb * xf + xo);
-      }
-    }
-  }
-
-  console.log("A")
-
-  for (var yb = 1; yb <= BIGGER_THAN_ANY_SMI; yb *= 2) {
-    for (var yo = -2; yo <= 2; yo++) {
-      addSigned(yb + yo);
-    }
-  }
-
-  function test(x,y) {
-    const lex = %SmiLexicographicCompare(x, y);
-    const expected = (x == y) ? 0 : (("" + x) < ("" + y) ? -1 : 1);
-    return lex == expected;
-  }
-
-  console.log(smis.length);
-
-  for (var i = 0; i < smis.length; i++) {
-    for (var j = 0; j < smis.length; j++) {
-      const x = smis[i];
-      const y = smis[j];
-      assertTrue(test(x, y), x + " < " + y);;
-    }
-  }
-
-  console.log("C")
-})();

test/mjsunit/mjsunit.status

@@ -274,7 +274,6 @@
 
   # TODO(mstarzinger): Takes too long with TF.
   'array-sort': [PASS, NO_VARIANTS],
-  'lexicographic-compare': [PASS, NO_VARIANTS],
   'regress/regress-91008': [PASS, NO_VARIANTS],
   'regress/regress-transcendental': [PASS, ['arch == arm64', NO_VARIANTS]],
   'compiler/osr-regress-max-locals': [PASS, NO_VARIANTS],
@@ -367,7 +366,6 @@
   'compiler/osr-with-args': [PASS, SLOW],
   'generated-transition-stub': [PASS, SLOW],
   'json2': [PASS, SLOW],
-  'lexicographic-compare': [PASS, SLOW],
   'math-floor-of-div-nosudiv': [PASS, SLOW],
   'math-floor-of-div': [PASS, SLOW],
   'messages': [PASS, SLOW],
@@ -393,7 +391,6 @@
 
   # Pass but take too long with the simulator in debug mode.
   'array-sort': [PASS, SLOW],
-  'lexicographic-compare': [PASS, SLOW],
   'packed-elements': [SKIP],
   'regexp-global': [SKIP],
   'math-floor-of-div': [PASS, SLOW],
@@ -505,7 +502,6 @@
   # Slow tests.
   'array-sort': [PASS, SLOW],
   'compiler/osr-with-args': [PASS, SLOW],
-  'lexicographic-compare': [PASS, SLOW],
   'packed-elements': [PASS, SLOW],
   'regress/regress-2790': [PASS, SLOW],
   'regress/regress-91008': [PASS, SLOW],
@@ -796,7 +792,6 @@
 ##############################################################################
 ['variant == nooptimization and (arch == arm or arch == arm64) and simulator_run', {
   # Slow tests: https://crbug.com/v8/7783
-  'lexicographic-compare': [SKIP],
   'md5': [SKIP],
   'wasm/asm-wasm-f32': [SKIP],
   'wasm/asm-wasm-f64': [SKIP],

third_party/v8/builtins/array-sort.tq

@@ -284,8 +284,7 @@ module array {
     assert(comparefn == Undefined);
 
     if (TaggedIsSmi(x) && TaggedIsSmi(y)) {
-      // TODO(szuend): Replace with a fast CallCFunction call.
-      return SmiLexicographicCompare(context, x, y);
+      return SmiLexicographicCompare(unsafe_cast<Smi>(x), unsafe_cast<Smi>(y));
     }
 
     // 5. Let xString be ? ToString(x).