[runtime] Remove unecessary ToString conversion for Array.prototype.forEach

Given that the index we use is checked to be in array index range there is no
need for a costly ToString conversion. All involved helpers for lookup up
properties directly support Smi/HeapNumber indices directly.

Cleanup: Rename GotoUnlessNumberLessThan => GotoIfNumberGreaterThanOrEqual

Change-Id: Iaddc4940f5d984572aa218d568ca71bf694cee74
Reviewed-on: https://chromium-review.googlesource.com/640388
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#48039}

src/builtins/builtins-array-gen.cc

@@ -535,30 +535,23 @@ class ArrayBuiltinCodeStubAssembler : public CodeStubAssembler {
     {
       if (direction == ForEachDirection::kForward) {
         // 8. Repeat, while k < len
-        GotoUnlessNumberLessThan(k(), len_, &after_loop);
+        GotoIfNumberGreaterThanOrEqual(k(), len_, &after_loop);
       } else {
         // OR
         // 10. Repeat, while k >= 0
-        GotoUnlessNumberLessThan(SmiConstant(-1), k(), &after_loop);
+        GotoIfNumberGreaterThanOrEqual(SmiConstant(-1), k(), &after_loop);
       }
 
       Label done_element(this, &to_);
       // a. Let Pk be ToString(k).
-      // We never have to perform a ToString conversion for Smi keys because
-      // they are guaranteed to be stored as elements. We easily hit this case
-      // when using any iteration builtin on a dictionary elements Array.
-      VARIABLE(p_k, MachineRepresentation::kTagged, k());
-      {
-        Label continue_with_key(this);
-        GotoIf(TaggedIsSmi(p_k.value()), &continue_with_key);
-        p_k.Bind(ToString(context(), p_k.value()));
-        Goto(&continue_with_key);
-        BIND(&continue_with_key);
-      }
+      // We never have to perform a ToString conversion as the above guards
+      // guarantee that we have a positive {k} which also is a valid array
+      // index in the range [0, 2^32-1).
+      CSA_ASSERT(this, IsNumberArrayIndex(k()));
 
       // b. Let kPresent be HasProperty(O, Pk).
       // c. ReturnIfAbrupt(kPresent).
-      Node* k_present = HasProperty(o(), p_k.value(), context(), kHasProperty);
+      Node* k_present = HasProperty(o(), k(), context(), kHasProperty);
 
       // d. If kPresent is true, then
       GotoIf(WordNotEqual(k_present, TrueConstant()), &done_element);
@@ -2364,7 +2357,7 @@ TF_BUILTIN(ArrayIteratorPrototypeNext, CodeStubAssembler) {
         length = var_length.value();
       }
 
-      GotoUnlessNumberLessThan(index, length, &set_done);
+      GotoIfNumberGreaterThanOrEqual(index, length, &set_done);
 
       StoreObjectField(iterator, JSArrayIterator::kNextIndexOffset,
                        NumberInc(index));

src/code-stub-assembler.cc

@@ -1445,6 +1445,9 @@ Node* CodeStubAssembler::LoadWeakCellValue(Node* weak_cell, Label* if_cleared) {
 Node* CodeStubAssembler::LoadFixedArrayElement(Node* object, Node* index_node,
                                                int additional_offset,
                                                ParameterMode parameter_mode) {
+  CSA_SLOW_ASSERT(this, IntPtrGreaterThanOrEqual(
+                            ParameterToWord(index_node, parameter_mode),
+                            IntPtrConstant(0)));
   int32_t header_size =
       FixedArray::kHeaderSize + additional_offset - kHeapObjectTag;
   Node* offset = ElementOffsetFromIndex(index_node, HOLEY_ELEMENTS,
@@ -3940,6 +3943,36 @@ Node* CodeStubAssembler::IsNumberPositive(Node* number) {
                 MachineRepresentation::kWord32);
 }
 
+Node* CodeStubAssembler::IsNumberArrayIndex(Node* number) {
+  VARIABLE(var_result, MachineRepresentation::kWord32, Int32Constant(1));
+
+  Label check_upper_bound(this), check_is_integer(this), out(this),
+      return_false(this);
+
+  GotoIfNumberGreaterThanOrEqual(number, NumberConstant(0), &check_upper_bound);
+  Goto(&return_false);
+
+  BIND(&check_upper_bound);
+  GotoIfNumberGreaterThanOrEqual(number, NumberConstant(kMaxUInt32),
+                                 &return_false);
+  Goto(&check_is_integer);
+
+  BIND(&check_is_integer);
+  GotoIf(TaggedIsSmi(number), &out);
+  // Check that the HeapNumber is a valid uint32
+  Node* value = LoadHeapNumberValue(number);
+  Node* int_value = ChangeFloat64ToUint32(value);
+  GotoIf(Float64Equal(value, ChangeUint32ToFloat64(int_value)), &out);
+  Goto(&return_false);
+
+  BIND(&return_false);
+  var_result.Bind(Int32Constant(0));
+  Goto(&out);
+
+  BIND(&out);
+  return var_result.value();
+}
+
 TNode<Uint32T> CodeStubAssembler::StringCharCodeAt(
     SloppyTNode<String> string, Node* index, ParameterMode parameter_mode) {
   CSA_ASSERT(this, MatchesParameterMode(index, parameter_mode));
@@ -7631,11 +7664,12 @@ void CodeStubAssembler::BranchIfNumericRelationalComparison(
   }
 }
 
-void CodeStubAssembler::GotoUnlessNumberLessThan(Node* lhs, Node* rhs,
-                                                 Label* if_false) {
-  Label if_true(this);
-  BranchIfNumericRelationalComparison(kLessThan, lhs, rhs, &if_true, if_false);
-  BIND(&if_true);
+void CodeStubAssembler::GotoIfNumberGreaterThanOrEqual(Node* lhs, Node* rhs,
+                                                       Label* if_true) {
+  Label if_false(this);
+  BranchIfNumericRelationalComparison(kGreaterThanOrEqual, lhs, rhs, if_true,
+                                      &if_false);
+  BIND(&if_false);
 }
 
 Node* CodeStubAssembler::RelationalComparison(RelationalComparisonMode mode,

src/code-stub-assembler.h

@@ -950,6 +950,9 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
   // within Smi range.
   Node* IsNumberNormalized(Node* number);
   Node* IsNumberPositive(Node* number);
+  // True iff {number} is a positive number and a valid array index in the range
+  // [0, 2^32-1).
+  Node* IsNumberArrayIndex(Node* number);
 
   // ElementsKind helpers:
   Node* IsFastElementsKind(Node* elements_kind);
@@ -1563,7 +1566,7 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler {
                                            Node* lhs, Node* rhs, Label* if_true,
                                            Label* if_false);
 
-  void GotoUnlessNumberLessThan(Node* lhs, Node* rhs, Label* if_false);
+  void GotoIfNumberGreaterThanOrEqual(Node* lhs, Node* rhs, Label* if_false);
 
   Node* Equal(Node* lhs, Node* rhs, Node* context,
               Variable* var_type_feedback = nullptr);

test/cctest/test-code-stub-assembler.cc

@@ -2670,6 +2670,83 @@ TEST(GotoIfNotWhiteSpaceOrLineTerminator) {
   }
 }
 
+TEST(BranchIfNumericRelationalComparison) {
+  Isolate* isolate(CcTest::InitIsolateOnce());
+  Factory* f = isolate->factory();
+  const int kNumParams = 2;
+  CodeAssemblerTester asm_tester(isolate, kNumParams);
+  {
+    CodeStubAssembler m(asm_tester.state());
+    Label return_true(&m), return_false(&m);
+    m.BranchIfNumericRelationalComparison(
+        CodeStubAssembler::kGreaterThanOrEqual, m.Parameter(0), m.Parameter(1),
+        &return_true, &return_false);
+    m.BIND(&return_true);
+    m.Return(m.BooleanConstant(true));
+    m.BIND(&return_false);
+    m.Return(m.BooleanConstant(false));
+  }
+
+  FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
+
+  ft.CheckTrue(f->NewNumber(0), f->NewNumber(0));
+  ft.CheckTrue(f->NewNumber(1), f->NewNumber(0));
+  ft.CheckTrue(f->NewNumber(1), f->NewNumber(1));
+  ft.CheckFalse(f->NewNumber(0), f->NewNumber(1));
+  ft.CheckFalse(f->NewNumber(-1), f->NewNumber(0));
+  ft.CheckTrue(f->NewNumber(-1), f->NewNumber(-1));
+
+  ft.CheckTrue(f->NewNumber(-1), f->NewNumber(-1.5));
+  ft.CheckFalse(f->NewNumber(-1.5), f->NewNumber(-1));
+  ft.CheckTrue(f->NewNumber(-1.5), f->NewNumber(-1.5));
+}
+
+TEST(IsNumberArrayIndex) {
+  Isolate* isolate(CcTest::InitIsolateOnce());
+  const int kNumParams = 1;
+  CodeAssemblerTester asm_tester(isolate, kNumParams);
+  {
+    CodeStubAssembler m(asm_tester.state());
+    m.Return(m.SmiFromWord32(m.IsNumberArrayIndex(m.Parameter(0))));
+  }
+
+  FunctionTester ft(asm_tester.GenerateCode(), kNumParams);
+
+  double indices[] = {Smi::kMinValue,
+                      -11,
+                      -1,
+                      0,
+                      1,
+                      2,
+                      Smi::kMaxValue,
+                      -11.0,
+                      -11.1,
+                      -2.0,
+                      -1.0,
+                      -0.0,
+                      0.0,
+                      0.00001,
+                      0.1,
+                      1,
+                      2,
+                      Smi::kMinValue - 1.0,
+                      Smi::kMinValue + 1.0,
+                      Smi::kMinValue + 1.2,
+                      kMaxInt + 1.2,
+                      kMaxInt - 10.0,
+                      kMaxInt - 1.0,
+                      kMaxInt,
+                      kMaxInt + 1.0,
+                      kMaxInt + 10.0};
+
+  for (size_t i = 0; i < arraysize(indices); i++) {
+    Handle<Object> index = isolate->factory()->NewNumber(indices[i]);
+    uint32_t array_index;
+    CHECK_EQ(index->ToArrayIndex(&array_index),
+             (ft.CallChecked<Smi>(index)->value() == 1));
+  }
+}
+
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8

test/mjsunit/array-reduce.js

@@ -537,3 +537,23 @@ var arr = [];
 Object.defineProperty(arr, "0", { get: function() { delete this[0] },
   configurable: true});
 assertEquals(undefined, arr.reduceRight(function(val) { return val }));
+
+
+(function ReduceRightMaxIndex() {
+  const kMaxIndex = 0xffffffff-1;
+  let array = [];
+  array[kMaxIndex-2] = 'value-2';
+  array[kMaxIndex-1] = 'value-1';
+  // Use the maximum array index possible.
+  array[kMaxIndex] = 'value';
+  // Add the next index which is a normal property and thus will not show up.
+  array[kMaxIndex+1] = 'normal property';
+  assertThrowsEquals( () => {
+      array.reduceRight((sum, value) => {
+        assertEquals('initial', sum);
+        assertEquals('value', value);
+        // Throw at this point as we would very slowly loop down from kMaxIndex.
+        throw 'do not continue';
+      }, 'initial')
+  }, 'do not continue');
+})();