[ic] Do string to array index conversion in element loads only

Instead of changing all of TryToName to do the conversion to array
index, this patch narrows this fast path just to the element load IC
handler.

This patch also restores the HeapNumber conversion in TryToIntPtr and
in Turbofan inlining as per the original state of things.

Bug: v8:9449, chromium:1016738, chromium:1016709
Change-Id: Ibf3a2c38637fc36e0ee037dc740f273848d1e8a5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1902386
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64896}

src/codegen/code-stub-assembler.cc

@@ -7665,8 +7665,7 @@ void CodeStubAssembler::TryToName(SloppyTNode<Object> key, Label* if_keyisindex,
 
   TVARIABLE(Int32T, var_instance_type);
   Label if_keyisnotindex(this);
-  *var_index =
-      TryToIntptr(key, &if_keyisnotindex, if_bailout, &var_instance_type);
+  *var_index = TryToIntptr(key, &if_keyisnotindex, &var_instance_type);
   Goto(if_keyisindex);
 
   BIND(&if_keyisnotindex);
@@ -7691,7 +7690,14 @@ void CodeStubAssembler::TryToName(SloppyTNode<Object> key, Label* if_keyisindex,
 
     BIND(&if_string);
     {
-      Label if_thinstring(this);
+      Label if_thinstring(this), if_has_cached_index(this);
+
+      TNode<Uint32T> hash = LoadNameHashField(CAST(key));
+      GotoIf(IsClearWord32(hash, Name::kDoesNotContainCachedArrayIndexMask),
+             &if_has_cached_index);
+      // No cached array index. If the string knows that it contains an index,
+      // then it must be an uncacheable index. Handle this case in the runtime.
+      GotoIf(IsClearWord32(hash, Name::kIsNotArrayIndexMask), if_bailout);
 
       GotoIf(InstanceTypeEqual(var_instance_type.value(), THIN_STRING_TYPE),
              &if_thinstring);
@@ -7707,9 +7713,20 @@ void CodeStubAssembler::TryToName(SloppyTNode<Object> key, Label* if_keyisindex,
       Goto(if_keyisunique);
 
       BIND(&if_thinstring);
-      *var_unique =
-          LoadObjectField<String>(CAST(key), ThinString::kActualOffset);
-      Goto(if_keyisunique);
+      {
+        *var_unique =
+            LoadObjectField<String>(CAST(key), ThinString::kActualOffset);
+        Goto(if_keyisunique);
+      }
+
+      BIND(&if_has_cached_index);
+      {
+        TNode<IntPtrT> index =
+            Signed(DecodeWordFromWord32<String::ArrayIndexValueBits>(hash));
+        CSA_ASSERT(this, IntPtrLessThan(index, IntPtrConstant(INT_MAX)));
+        *var_index = index;
+        Goto(if_keyisindex);
+      }
     }
 
     BIND(&if_keyisother);
@@ -9750,26 +9767,19 @@ TNode<Map> CodeStubAssembler::LoadReceiverMap(SloppyTNode<Object> receiver) {
 }
 
 TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
-    SloppyTNode<Object> key, Label* if_not_intptr, Label* if_bailout,
+    SloppyTNode<Object> key, Label* if_not_intptr,
     TVariable<Int32T>* var_instance_type) {
   TVARIABLE(IntPtrT, var_intptr_key);
   Label done(this, &var_intptr_key), key_is_smi(this), key_is_heapnumber(this);
   GotoIf(TaggedIsSmi(key), &key_is_smi);
 
+  TNode<Int32T> instance_type = LoadInstanceType(CAST(key));
   if (var_instance_type != nullptr) {
-    *var_instance_type = LoadInstanceType(CAST(key));
-  }
-
-  Node* function = ExternalConstant(
-      ExternalReference::object_to_array_index_slow_function());
-  TNode<Int32T> result = UncheckedCast<Int32T>(
-      CallCFunction(function, MachineType::Int32(),
-                    std::make_pair(MachineType::AnyTagged(), key)));
-  GotoIf(Word32Equal(Int32Constant(-1), result), if_not_intptr);
-  if (if_bailout == nullptr) if_bailout = if_not_intptr;
-  GotoIf(Word32Equal(Int32Constant(-2), result), if_bailout);
-  var_intptr_key = ChangeInt32ToIntPtr(result);
-  Goto(&done);
+    *var_instance_type = instance_type;
+  }
+
+  Branch(IsHeapNumberInstanceType(instance_type), &key_is_heapnumber,
+         if_not_intptr);
 
   BIND(&key_is_smi);
   {
@@ -9777,6 +9787,16 @@ TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
     Goto(&done);
   }
 
+  BIND(&key_is_heapnumber);
+  {
+    TNode<Float64T> value = LoadHeapNumberValue(CAST(key));
+    TNode<Int32T> int_value = RoundFloat64ToInt32(value);
+    GotoIfNot(Float64Equal(value, ChangeInt32ToFloat64(int_value)),
+              if_not_intptr);
+    var_intptr_key = ChangeInt32ToIntPtr(int_value);
+    Goto(&done);
+  }
+
   BIND(&done);
   return var_intptr_key.value();
 }

src/codegen/code-stub-assembler.h

@@ -3658,7 +3658,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
                                      GetOwnPropertyMode mode = kCallJSGetter);
 
   TNode<IntPtrT> TryToIntptr(SloppyTNode<Object> key, Label* if_not_intptr,
-                             Label* if_bailout = nullptr,
                              TVariable<Int32T>* var_instance_type = nullptr);
 
   TNode<Context> AllocateSyntheticFunctionContext(

src/codegen/external-reference.cc

@@ -641,8 +641,7 @@ FUNCTION_REFERENCE(copy_typed_array_elements_to_typed_array,
 FUNCTION_REFERENCE(copy_typed_array_elements_slice, CopyTypedArrayElementsSlice)
 FUNCTION_REFERENCE(try_internalize_string_function,
                    StringTable::LookupStringIfExists_NoAllocate)
-FUNCTION_REFERENCE(object_to_array_index_slow_function,
-                   Object::ToArrayIndexSlow)
+FUNCTION_REFERENCE(string_to_array_index_function, String::ToArrayIndex)
 
 static Address LexicographicCompareWrapper(Isolate* isolate, Address smi_x,
                                            Address smi_y) {

src/codegen/external-reference.h

@@ -159,7 +159,6 @@ class StatsCounter;
   V(mutable_big_int_absolute_sub_and_canonicalize_function,                   \
     "MutableBigInt_AbsoluteSubAndCanonicalize")                               \
   V(new_deoptimizer_function, "Deoptimizer::New()")                           \
-  V(object_to_array_index_slow_function, "Object::ToArrayIndexSlow")          \
   V(orderedhashmap_gethash_raw, "orderedhashmap_gethash_raw")                 \
   V(printf_function, "printf")                                                \
   V(refill_math_random, "MathRandom::RefillCache")                            \
@@ -168,6 +167,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(smi_lexicographic_compare_function, "smi_lexicographic_compare_function") \
+  V(string_to_array_index_function, "String::ToArrayIndex")                   \
   V(try_internalize_string_function, "try_internalize_string_function")       \
   V(wasm_call_trap_callback_for_testing,                                      \
     "wasm::call_trap_callback_for_testing")                                   \

src/compiler/effect-control-linearizer.cc

@@ -2495,19 +2495,41 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToArrayIndex(
   // In the Smi case, just convert to int32.
   __ Goto(&done, ChangeSmiToInt32(value));
 
+  // In the non-Smi case, check the heap numberness, load the number and convert
+  // to int32.
   __ Bind(&if_not_smi);
+  auto if_not_heap_number = __ MakeDeferredLabel();
+  Node* value_map = __ LoadField(AccessBuilder::ForMap(), value);
+  Node* is_heap_number = __ TaggedEqual(value_map, __ HeapNumberMapConstant());
+  __ GotoIfNot(is_heap_number, &if_not_heap_number);
+
+  Node* number = __ LoadField(AccessBuilder::ForHeapNumberValue(), value);
+  number =
+      BuildCheckedFloat64ToInt32(CheckForMinusZeroMode::kDontCheckForMinusZero,
+                                 params.feedback(), number, frame_state);
+  __ Goto(&done, number);
+
+  __ Bind(&if_not_heap_number);
+  auto calculate_index = __ MakeDeferredLabel();
+  Node* value_instance_type =
+      __ LoadField(AccessBuilder::ForMapInstanceType(), value_map);
+  Node* is_string = __ Uint32LessThan(value_instance_type,
+                                      __ Uint32Constant(FIRST_NONSTRING_TYPE));
+  __ DeoptimizeIfNot(DeoptimizeReason::kNotAString, params.feedback(),
+                     is_string, frame_state);
+
   MachineSignature::Builder builder(graph()->zone(), 1, 1);
   builder.AddReturn(MachineType::Int32());
   builder.AddParam(MachineType::TaggedPointer());
-  Node* object_to_array_index_function = __ ExternalConstant(
-      ExternalReference::object_to_array_index_slow_function());
+  Node* string_to_array_index_function =
+      __ ExternalConstant(ExternalReference::string_to_array_index_function());
   auto call_descriptor =
       Linkage::GetSimplifiedCDescriptor(graph()->zone(), builder.Build());
   Node* index = __ Call(common()->Call(call_descriptor),
-                        object_to_array_index_function, value);
+                        string_to_array_index_function, value);
 
   __ DeoptimizeIf(DeoptimizeReason::kNotAnArrayIndex, params.feedback(),
-                  __ Int32LessThan(index, __ Int32Constant(0)), frame_state);
+                  __ Word32Equal(index, __ Int32Constant(-1)), frame_state);
 
   __ Goto(&done, index);
 
@@ -2523,7 +2545,6 @@ Node* EffectControlLinearizer::LowerCheckedTaggedToInt32(Node* node,
 
   auto if_not_smi = __ MakeDeferredLabel();
   auto done = __ MakeLabel(MachineRepresentation::kWord32);
-
   Node* check = ObjectIsSmi(value);
   __ GotoIfNot(check, &if_not_smi);
   // In the Smi case, just convert to int32.

src/ic/accessor-assembler.cc

@@ -315,7 +315,9 @@ void AccessorAssembler::HandleLoadICSmiHandlerCase(
   if (support_elements == kSupportElements) {
     Label if_element(this), if_indexed_string(this), if_property(this),
         if_hole(this), unimplemented_elements_kind(this),
-        if_oob(this, Label::kDeferred);
+        if_oob(this, Label::kDeferred), try_string_to_array_index(this),
+        emit_element_load(this);
+    TVARIABLE(IntPtrT, var_intptr_index);
     GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kElement)),
            &if_element);
 
@@ -331,16 +333,42 @@ void AccessorAssembler::HandleLoadICSmiHandlerCase(
     }
 
     BIND(&if_element);
-    Comment("element_load");
-    TNode<IntPtrT> intptr_index = TryToIntptr(p->name(), miss);
-    TNode<BoolT> is_jsarray_condition =
-        IsSetWord<LoadHandler::IsJsArrayBits>(handler_word);
-    TNode<Uint32T> elements_kind =
-        DecodeWord32FromWord<LoadHandler::ElementsKindBits>(handler_word);
-    EmitElementLoad(holder, elements_kind, intptr_index, is_jsarray_condition,
-                    &if_hole, &rebox_double, &var_double_value,
-                    &unimplemented_elements_kind, &if_oob, miss, exit_point,
-                    access_mode);
+    {
+      Comment("element_load");
+      TVARIABLE(Int32T, var_instance_type);
+      TNode<IntPtrT> intptr_index = TryToIntptr(
+          p->name(), &try_string_to_array_index, &var_instance_type);
+      var_intptr_index = intptr_index;
+      Goto(&emit_element_load);
+
+      BIND(&try_string_to_array_index);
+      {
+        GotoIfNot(IsStringInstanceType(var_instance_type.value()), miss);
+
+        Node* function = ExternalConstant(
+            ExternalReference::string_to_array_index_function());
+        TNode<Int32T> result = UncheckedCast<Int32T>(
+            CallCFunction(function, MachineType::Int32(),
+                          std::make_pair(MachineType::AnyTagged(), p->name())));
+        GotoIf(Word32Equal(Int32Constant(-1), result), miss);
+        CSA_ASSERT(this, Int32GreaterThanOrEqual(result, Int32Constant(0)));
+        var_intptr_index = ChangeInt32ToIntPtr(result);
+
+        Goto(&emit_element_load);
+      }
+
+      BIND(&emit_element_load);
+      {
+        TNode<BoolT> is_jsarray_condition =
+            IsSetWord<LoadHandler::IsJsArrayBits>(handler_word);
+        TNode<Uint32T> elements_kind =
+            DecodeWord32FromWord<LoadHandler::ElementsKindBits>(handler_word);
+        EmitElementLoad(holder, elements_kind, var_intptr_index.value(),
+                        is_jsarray_condition, &if_hole, &rebox_double,
+                        &var_double_value, &unimplemented_elements_kind,
+                        &if_oob, miss, exit_point, access_mode);
+      }
+    }
 
     BIND(&unimplemented_elements_kind);
     {
@@ -366,7 +394,7 @@ void AccessorAssembler::HandleLoadICSmiHandlerCase(
       // in case of typed arrays, where integer indexed properties
       // aren't looked up in the prototype chain.
       GotoIf(IsJSTypedArray(holder), &return_undefined);
-      GotoIf(IntPtrLessThan(intptr_index, IntPtrConstant(0)), miss);
+      GotoIf(IntPtrLessThan(var_intptr_index.value(), IntPtrConstant(0)), miss);
 
       // For all other receivers we need to check that the prototype chain
       // doesn't contain any elements.

src/objects/objects.cc

@@ -600,37 +600,6 @@ Object Object::ToBoolean(Isolate* isolate) {
   return isolate->heap()->ToBoolean(BooleanValue(isolate));
 }
 
-int32_t Object::ToArrayIndexSlow(Address addr) {
-  DisallowHeapAllocation no_gc;
-  Object key(addr);
-
-  // Smi case should be handled by the fast path.
-  DCHECK(!key.IsSmi());
-
-  uint32_t index;
-  bool success = false;
-  if (key.IsHeapNumber()) {
-    double num = HeapNumber::cast(key).value();
-    success = DoubleToUint32IfEqualToSelf(num, &index);
-  } else if (key.IsString()) {
-    success = String::cast(key).AsArrayIndex(&index);
-  }
-
-  if (!success) return -1;
-  if (index <= INT_MAX) return index;
-
-  // TODO(gsathya): This check exists because we only support upto
-  // INT_MAX for element access in the builtins. We return -2 to
-  // distinguish the case where index <= JSArray::kMaxArrayIndex and
-  // index > INT_MAX so the builtin can handle this appropriately.
-  //
-  // Once we change the builtins to correctly support element access
-  // for indices up to JSArray::kMaxArrayIndex, this check can go
-  // away.
-  if (index <= JSArray::kMaxArrayIndex) return -2;
-  return -1;
-}
-
 namespace {
 
 // TODO(bmeurer): Maybe we should introduce a marker interface Number,

src/objects/objects.h

@@ -315,18 +315,6 @@ class Object : public TaggedImpl<HeapObjectReferenceType::STRONG, Address> {
   V8_EXPORT_PRIVATE bool ToInt32(int32_t* value);
   inline bool ToUint32(uint32_t* value) const;
 
-  // Converts a HeapNumber or String to an int32. Negative numbers are
-  // not supported. This is used for calculating array indices but
-  // differs from an Array Index in the regard that this does not
-  // support the full array index range. This only supports positive
-  // numbers less than INT_MAX.
-  //
-  // if val < 0, returns -1
-  // if 0 <= val <= INT_MAX, returns val
-  // if INT_MAX < val <= JSArray::kMaxArrayIndex, returns -2
-  // if JSArray::kMaxArrayIndex < val, returns -1
-  static int32_t ToArrayIndexSlow(Address addr);
-
   inline Representation OptimalRepresentation(Isolate* isolate) const;
 
   inline ElementsKind OptimalElementsKind(Isolate* isolate) const;

src/objects/string.cc

@@ -395,6 +395,16 @@ Handle<String> String::Trim(Isolate* isolate, Handle<String> string,
   return isolate->factory()->NewSubString(string, left, right);
 }
 
+int32_t String::ToArrayIndex(Address addr) {
+  DisallowHeapAllocation no_gc;
+  String key(addr);
+
+  uint32_t index;
+  if (!key.AsArrayIndex(&index)) return -1;
+  if (index <= INT_MAX) return index;
+  return -1;
+}
+
 bool String::LooksValid() {
   // TODO(leszeks): Maybe remove this check entirely, Heap::Contains uses
   // basically the same logic as the way we access the heap in the first place.

src/objects/string.h

@@ -318,6 +318,19 @@ class String : public TorqueGeneratedString<String, Name> {
   // Conversion.
   // "array index": an index allowed by the ES spec for JSArrays.
   inline bool AsArrayIndex(uint32_t* index);
+
+  // This is used for calculating array indices but differs from an
+  // Array Index in the regard that this does not support the full
+  // array index range. This only supports positive numbers less than
+  // or equal to INT_MAX.
+  //
+  // String::AsArrayIndex might be a better fit if you're looking to
+  // calculate the array index.
+  //
+  // if val < 0 or val > INT_MAX, returns -1
+  // if 0 <= val <= INT_MAX, returns val
+  static int32_t ToArrayIndex(Address addr);
+
   uint32_t inline ToValidIndex(Object number);
   // "integer index": the string is the decimal representation of an
   // integer in the range of a size_t. Useful for TypedArray accesses.

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

@@ -743,11 +743,10 @@ TEST(TryToName) {
 
   {
     // TryToName(<internalized uncacheable number string less than
-    // INT_MAX>) => is_keyisindex: number.
+    // INT_MAX>) => bailout
     Handle<Object> key =
         isolate->factory()->InternalizeUtf8String("2147483647");
-    Handle<Object> index = isolate->factory()->NewNumber(2147483647);
-    ft.CheckTrue(key, expect_index, index);
+    ft.CheckTrue(key, expect_bailout);
   }
 
   {
@@ -765,8 +764,7 @@ TEST(TryToName) {
     // TryToName(<number string without cached index>) => is_keyisindex: number.
     Handle<String> key = isolate->factory()->NewStringFromAsciiChecked("153");
     CHECK(!key->HasHashCode());
-    Handle<Object> index(Smi::FromInt(153), isolate);
-    ft.CheckTrue(key, expect_index, index);
+    ft.CheckTrue(key, expect_bailout);
   }
 
   {