[ic] Fix KeyedLoadIC for ArrayIndex access

Previously, without support for converting strings to numbers we'd
switch to megamorphic state and go to the runtime always to do the
conversion causing a performance cliff.

This patch improves the following js-perf-test scores:
Object-Lookup-String-Constant-BytecodeHandler: 4.25%
Object-Lookup-Index-String-BytecodeHandler: 5.41%

Bug: v8:9449
Change-Id: I63787fa84373fc946f1304b0141e48a52a1b4bcb
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1690953Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63293}

src/builtins/builtins-internal-gen.cc

@@ -533,7 +533,7 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) {
 
   VARIABLE(var_index, MachineType::PointerRepresentation());
   VARIABLE(var_unique, MachineRepresentation::kTagged, key);
-  Label if_index(this), if_unique_name(this), if_notunique(this),
+  Label if_index(this, &var_index), if_unique_name(this), if_notunique(this),
       if_notfound(this), slow(this), if_proxy(this);
 
   GotoIf(TaggedIsSmi(receiver), &slow);

src/builtins/builtins-object-gen.cc

@@ -384,7 +384,8 @@ TF_BUILTIN(ObjectPrototypeHasOwnProperty, ObjectBuiltinsAssembler) {
     VARIABLE(var_index, MachineType::PointerRepresentation());
     VARIABLE(var_unique, MachineRepresentation::kTagged);
 
-    Label if_index(this), if_unique_name(this), if_notunique_name(this);
+    Label if_index(this, &var_index), if_unique_name(this),
+        if_notunique_name(this);
     TryToName(key, &if_index, &var_index, &if_unique_name, &var_unique,
               &call_runtime, &if_notunique_name);
 

src/codegen/code-stub-assembler.cc

@@ -8303,37 +8303,30 @@ void CodeStubAssembler::TryToName(Node* key, Label* if_keyisindex,
   DCHECK_EQ(MachineRepresentation::kTagged, var_unique->rep());
   Comment("TryToName");
 
-  Label if_hascachedindex(this), if_keyisnotindex(this), if_thinstring(this),
+  TVARIABLE(Int32T, var_instance_type);
+  Label if_keyisnotindex(this), if_thinstring(this),
       if_keyisother(this, Label::kDeferred);
-  // Handle Smi and HeapNumber keys.
-  var_index->Bind(TryToIntptr(key, &if_keyisnotindex));
+  var_index->Bind(TryToIntptr(key, &if_keyisnotindex, &var_instance_type));
   Goto(if_keyisindex);
 
   BIND(&if_keyisnotindex);
-  Node* key_map = LoadMap(key);
   var_unique->Bind(key);
+
   // Symbols are unique.
-  GotoIf(IsSymbolMap(key_map), if_keyisunique);
-  Node* key_instance_type = LoadMapInstanceType(key_map);
+  GotoIf(IsSymbolInstanceType(var_instance_type.value()), if_keyisunique);
+
   // Miss if |key| is not a String.
   STATIC_ASSERT(FIRST_NAME_TYPE == FIRST_TYPE);
-  GotoIfNot(IsStringInstanceType(key_instance_type), &if_keyisother);
-
-  // |key| is a String. Check if it has a cached array index.
-  Node* hash = LoadNameHashField(key);
-  GotoIf(IsClearWord32(hash, Name::kDoesNotContainCachedArrayIndexMask),
-         &if_hascachedindex);
-  // 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);
-  // Check if we have a ThinString.
-  GotoIf(InstanceTypeEqual(key_instance_type, THIN_STRING_TYPE),
-         &if_thinstring);
-  GotoIf(InstanceTypeEqual(key_instance_type, THIN_ONE_BYTE_STRING_TYPE),
+  GotoIfNot(IsStringInstanceType(var_instance_type.value()), &if_keyisother);
+
+  GotoIf(InstanceTypeEqual(var_instance_type.value(), THIN_STRING_TYPE),
          &if_thinstring);
+  GotoIf(
+      InstanceTypeEqual(var_instance_type.value(), THIN_ONE_BYTE_STRING_TYPE),
+      &if_thinstring);
   // Finally, check if |key| is internalized.
   STATIC_ASSERT(kNotInternalizedTag != 0);
-  GotoIf(IsSetWord32(key_instance_type, kIsNotInternalizedMask),
+  GotoIf(IsSetWord32(var_instance_type.value(), kIsNotInternalizedMask),
          if_notinternalized != nullptr ? if_notinternalized : if_bailout);
   Goto(if_keyisunique);
 
@@ -8342,12 +8335,9 @@ void CodeStubAssembler::TryToName(Node* key, Label* if_keyisindex,
       LoadObjectField<String>(CAST(key), ThinString::kActualOffset));
   Goto(if_keyisunique);
 
-  BIND(&if_hascachedindex);
-  var_index->Bind(DecodeWordFromWord32<Name::ArrayIndexValueBits>(hash));
-  Goto(if_keyisindex);
-
   BIND(&if_keyisother);
-  GotoIfNot(InstanceTypeEqual(key_instance_type, ODDBALL_TYPE), if_bailout);
+  GotoIfNot(InstanceTypeEqual(var_instance_type.value(), ODDBALL_TYPE),
+            if_bailout);
   var_unique->Bind(LoadObjectField(key, Oddball::kToStringOffset));
   Goto(if_keyisunique);
 }
@@ -10272,12 +10262,44 @@ TNode<Map> CodeStubAssembler::LoadReceiverMap(SloppyTNode<Object> receiver) {
       [=] { return LoadMap(UncheckedCast<HeapObject>(receiver)); });
 }
 
-TNode<IntPtrT> CodeStubAssembler::TryToIntptr(Node* key, Label* miss) {
+TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
+    Node* key, Label* miss, TVariable<Int32T>* var_instance_type) {
   TVARIABLE(IntPtrT, var_intptr_key);
-  Label done(this, &var_intptr_key), key_is_smi(this);
+  Label done(this, &var_intptr_key), key_is_smi(this), key_is_heapnumber(this);
   GotoIf(TaggedIsSmi(key), &key_is_smi);
-  // Try to convert a heap number to a Smi.
-  GotoIfNot(IsHeapNumber(key), miss);
+
+  TNode<Map> map = LoadMap(key);
+  TNode<Int32T> instance_type = LoadMapInstanceType(map);
+  if (var_instance_type != nullptr) *var_instance_type = instance_type;
+  GotoIf(IsHeapNumberInstanceType(instance_type), &key_is_heapnumber);
+  GotoIfNot(IsStringInstanceType(instance_type), miss);
+
+  Label if_has_cached_index(this), if_calculate_index(this);
+  TNode<Uint32T> hash = LoadNameHashField(key);
+
+  Branch(IsClearWord32(hash, Name::kDoesNotContainCachedArrayIndexMask),
+         &if_has_cached_index, &if_calculate_index);
+
+  BIND(&if_has_cached_index);
+  {
+    var_intptr_key =
+        Signed(DecodeWordFromWord32<String::ArrayIndexValueBits>(hash));
+    Goto(&done);
+  }
+
+  BIND(&if_calculate_index);
+  {
+    Node* function =
+        ExternalConstant(ExternalReference::string_as_array_index_function());
+    TNode<IntPtrT> result = UncheckedCast<IntPtrT>(
+        CallCFunction(function, MachineType::Pointer(),
+                      std::make_pair(MachineType::AnyTagged(), key)));
+    GotoIf(IntPtrEqual(IntPtrConstant(kMaxUInt32), result), miss);
+    var_intptr_key = result;
+    Goto(&done);
+  }
+
+  BIND(&key_is_heapnumber);
   {
     TNode<Float64T> value = LoadHeapNumberValue(key);
     TNode<Int32T> int_value = RoundFloat64ToInt32(value);

src/codegen/code-stub-assembler.h

@@ -3489,7 +3489,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
                                      Node* receiver, Label* if_bailout,
                                      GetOwnPropertyMode mode = kCallJSGetter);
 
-  TNode<IntPtrT> TryToIntptr(Node* key, Label* miss);
+  TNode<IntPtrT> TryToIntptr(Node* key, Label* miss,
+                             TVariable<Int32T>* var_instance_type = nullptr);
 
   void BranchIfPrototypesHaveNoElements(Node* receiver_map,
                                         Label* definitely_no_elements,

src/codegen/external-reference.cc

@@ -649,6 +649,8 @@ 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(string_as_array_index_function,
+                   String::AsArrayIndex_NoAllocation)
 
 static Address LexicographicCompareWrapper(Isolate* isolate, Address smi_x,
                                            Address smi_y) {

src/codegen/external-reference.h

@@ -166,6 +166,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_as_array_index_function, "String::AsArrayIndex_NoAllocation")      \
   V(store_buffer_overflow_function, "StoreBuffer::StoreBufferOverflow")       \
   V(try_internalize_string_function, "try_internalize_string_function")       \
   V(wasm_call_trap_callback_for_testing,                                      \

src/ic/accessor-assembler.cc

@@ -302,7 +302,10 @@ void AccessorAssembler::HandleLoadICSmiHandlerCase(
   TNode<IntPtrT> handler_kind =
       Signed(DecodeWord<LoadHandler::KindBits>(handler_word));
   if (support_elements == kSupportElements) {
-    Label if_element(this), if_indexed_string(this), if_property(this);
+    TVARIABLE(IntPtrT, var_intptr_index);
+    Label if_element(this), if_indexed_string(this), if_property(this),
+        if_hole(this), unimplemented_elements_kind(this),
+        if_oob(this, Label::kDeferred);
     GotoIf(WordEqual(handler_kind, IntPtrConstant(LoadHandler::kElement)),
            &if_element);
 
@@ -318,18 +321,18 @@ void AccessorAssembler::HandleLoadICSmiHandlerCase(
     }
 
     BIND(&if_element);
-    Comment("element_load");
-    Node* intptr_index = TryToIntptr(p->name(), miss);
-    Node* is_jsarray_condition =
-        IsSetWord<LoadHandler::IsJsArrayBits>(handler_word);
-    Node* elements_kind =
-        DecodeWord32FromWord<LoadHandler::ElementsKindBits>(handler_word);
-    Label if_hole(this), unimplemented_elements_kind(this),
-        if_oob(this, Label::kDeferred);
-    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");
+      var_intptr_index = TryToIntptr(p->name(), miss);
+      Node* is_jsarray_condition =
+          IsSetWord<LoadHandler::IsJsArrayBits>(handler_word);
+      Node* 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);
     {
@@ -355,7 +358,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/ic/ic.cc

@@ -1147,29 +1147,26 @@ void KeyedLoadIC::LoadElementPolymorphicHandlers(
 namespace {
 
 bool ConvertKeyToIndex(Handle<Object> receiver, Handle<Object> key,
-                       uint32_t* index, InlineCacheState state) {
-  if (!FLAG_use_ic || state == NO_FEEDBACK) return false;
-  if (receiver->IsAccessCheckNeeded() || receiver->IsJSPrimitiveWrapper()) {
-    return false;
-  }
+                       uint32_t* index) {
+  if (!receiver->IsJSReceiver()) return false;
+  if (key->ToArrayIndex(index)) return true;
+
+  if (!receiver->IsJSTypedArray()) return false;
 
-  // For regular JSReceiver or String receivers, the {key} must be a positive
-  // array index.
-  if (receiver->IsJSReceiver() || receiver->IsString()) {
-    if (key->ToArrayIndex(index)) return true;
-  }
   // For JSTypedArray receivers, we can also support negative keys, which we
   // just map into the [2**31, 2**32 - 1] range via a bit_cast. This is valid
   // because JSTypedArray::length is always a Smi, so such keys will always
   // be detected as OOB.
-  if (receiver->IsJSTypedArray()) {
-    int32_t signed_index;
-    if (key->ToInt32(&signed_index)) {
-      *index = bit_cast<uint32_t>(signed_index);
-      return true;
-    }
-  }
-  return false;
+  int32_t signed_index;
+  if (!key->ToInt32(&signed_index)) return false;
+  *index = bit_cast<uint32_t>(signed_index);
+  return true;
+}
+
+bool CanCache(Handle<Object> receiver, InlineCacheState state) {
+  if (!FLAG_use_ic || state == NO_FEEDBACK) return false;
+  if (!receiver->IsJSReceiver()) return false;
+  return !receiver->IsAccessCheckNeeded() && !receiver->IsJSPrimitiveWrapper();
 }
 
 bool IsOutOfBoundsAccess(Handle<Object> receiver, uint32_t index) {
@@ -1233,13 +1230,18 @@ MaybeHandle<Object> KeyedLoadIC::Load(Handle<Object> object,
   key = TryConvertKey(key, isolate());
 
   uint32_t index;
-  if ((key->IsInternalizedString() &&
-       !String::cast(*key).AsArrayIndex(&index)) ||
+  bool already_found_index = false;
+  if (key->IsInternalizedString()) {
+    already_found_index = String::cast(*key).AsArrayIndex(&index);
+  }
+
+  if ((key->IsInternalizedString() && !already_found_index) ||
       key->IsSymbol()) {
     ASSIGN_RETURN_ON_EXCEPTION(isolate(), load_handle,
                                LoadIC::Load(object, Handle<Name>::cast(key)),
                                Object);
-  } else if (ConvertKeyToIndex(object, key, &index, state())) {
+  } else if (CanCache(object, state()) &&
+             (already_found_index || ConvertKeyToIndex(object, key, &index))) {
     KeyedAccessLoadMode load_mode = GetLoadMode(isolate(), object, index);
     UpdateLoadElement(Handle<HeapObject>::cast(object), load_mode);
     if (is_vector_set()) {

src/ic/keyed-store-generic.cc

@@ -969,8 +969,8 @@ void KeyedStoreGenericAssembler::KeyedStoreGeneric(
     TNode<Object> value, Maybe<LanguageMode> language_mode) {
   TVARIABLE(IntPtrT, var_index);
   TVARIABLE(Object, var_unique, key);
-  Label if_index(this), if_unique_name(this), not_internalized(this),
-      slow(this);
+  Label if_index(this, &var_index), if_unique_name(this),
+      not_internalized(this), slow(this);
 
   GotoIf(TaggedIsSmi(receiver), &slow);
   TNode<Map> receiver_map = LoadMap(CAST(receiver));

src/objects/string-inl.h

@@ -719,6 +719,7 @@ void StringCharacterStream::VisitTwoByteString(const uint16_t* chars,
 }
 
 bool String::AsArrayIndex(uint32_t* index) {
+  DisallowHeapAllocation no_gc;
   uint32_t field = hash_field();
   if (IsHashFieldComputed(field) && (field & kIsNotArrayIndexMask)) {
     return false;

src/objects/string.cc

@@ -1352,6 +1352,19 @@ uint32_t String::ComputeAndSetHash() {
   return result;
 }
 
+int String::AsArrayIndex_NoAllocation(Address key_addr) {
+  DisallowHeapAllocation no_gc;
+  String key(key_addr);
+  uint32_t index;
+  bool found = key.AsArrayIndex(&index);
+  if (!found) {
+    STATIC_ASSERT(JSArray::kMaxArrayIndex < kMaxUInt32);
+    return kMaxUInt32;
+  }
+
+  return index;
+}
+
 bool String::ComputeArrayIndex(uint32_t* index) {
   int length = this->length();
   if (length == 0 || length > kMaxArrayIndexSize) return false;

src/objects/string.h

@@ -311,6 +311,7 @@ class String : public TorqueGeneratedString<String, Name> {
   // Conversion.
   inline bool AsArrayIndex(uint32_t* index);
   uint32_t inline ToValidIndex(Object number);
+  static int AsArrayIndex_NoAllocation(Address key);
 
   // Trimming.
   enum TrimMode { kTrim, kTrimStart, kTrimEnd };

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

@@ -439,6 +439,7 @@ TEST(TryToName) {
     {
       Variable var_index(&m, MachineType::PointerRepresentation());
       Variable var_unique(&m, MachineRepresentation::kTagged);
+      Variable var_expected(&m, MachineType::PointerRepresentation());
 
       m.TryToName(key, &if_keyisindex, &var_index, &if_keyisunique, &var_unique,
                   &if_bailout);
@@ -447,8 +448,24 @@ TEST(TryToName) {
       m.GotoIfNot(m.WordEqual(expected_result,
                               m.SmiConstant(Smi::FromInt(kKeyIsIndex))),
                   &failed);
-      m.Branch(m.WordEqual(m.SmiUntag(expected_arg), var_index.value()),
-               &passed, &failed);
+
+      Label if_expectedissmi(&m), if_expectedisheapnumber(&m), check_result(&m);
+      m.Branch(m.TaggedIsSmi(expected_arg), &if_expectedissmi,
+               &if_expectedisheapnumber);
+
+      m.BIND(&if_expectedissmi);
+      var_expected.Bind(m.SmiUntag(expected_arg));
+      m.Goto(&check_result);
+
+      m.BIND(&if_expectedisheapnumber);
+      CSA_ASSERT(&m, m.IsHeapNumber(expected_arg));
+      TNode<Float64T> value = m.LoadHeapNumberValue(expected_arg);
+      var_expected.Bind(m.ChangeFloat64ToUintPtr(value));
+      m.Goto(&check_result);
+
+      m.BIND(&check_result);
+      m.Branch(m.IntPtrEqual(var_expected.value(), var_index.value()), &passed,
+               &failed);
 
       m.BIND(&if_keyisunique);
       m.GotoIfNot(m.WordEqual(expected_result,
@@ -550,10 +567,12 @@ TEST(TryToName) {
   }
 
   {
-    // TryToName(<internalized uncacheable number string>) => bailout
+    // TryToName(<internalized uncacheable number string>) => is_keyisindex:
+    // number.
     Handle<Object> key =
         isolate->factory()->InternalizeUtf8String("4294967294");
-    ft.CheckTrue(key, expect_bailout);
+    Handle<Object> index = isolate->factory()->NewNumber(4294967294);
+    ft.CheckTrue(key, expect_index, index);
   }
 
   {
@@ -568,10 +587,11 @@ TEST(TryToName) {
   }
 
   {
-    // TryToName(<number string without cached index>) => bailout.
+    // TryToName(<number string without cached index>) => is_keyisindex: number.
     Handle<String> key = isolate->factory()->NewStringFromAsciiChecked("153");
     CHECK(!key->HasHashCode());
-    ft.CheckTrue(key, expect_bailout);
+    Handle<Object> index(Smi::FromInt(153), isolate);
+    ft.CheckTrue(key, expect_index, index);
   }
 
   {