Revert "[ic] Inline loads for heapnumber and cached string as ArrayIndex"

This reverts commit 0457bed1fa.

Reason for revert: doesn't help perf too much

Original change's description:
> [ic] Inline loads for heapnumber and cached string as ArrayIndex
> 
> Bug: chromium:1016738, chromium:1016709, v8:9449
> Change-Id: I5b50f21b3e40651e16201e63b4a7010b1bf0c639
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1897890
> Reviewed-by: Toon Verwaest <verwaest@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#64766}

TBR=neis@chromium.org,gsathya@chromium.org,verwaest@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: chromium:1016738, chromium:1016709, v8:9449
Change-Id: I8a68cac329f06fa47516ecd9708f1e91e5d15b77
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1901276
Reviewed-by: Sathya Gunasekaran  <gsathya@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Sathya Gunasekaran  <gsathya@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64821}

src/codegen/code-stub-assembler.cc

@@ -9775,31 +9775,12 @@ TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
   Label done(this, &var_intptr_key), key_is_smi(this), key_is_heapnumber(this);
   GotoIf(TaggedIsSmi(key), &key_is_smi);
 
-  TNode<Map> map = LoadMap(CAST(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), if_not_intptr);
-
-  Label if_has_cached_index(this), if_calculate_index(this);
-  TNode<Uint32T> hash = LoadNameHashField(CAST(key));
-
-  Branch(IsClearWord32(hash, Name::kDoesNotContainCachedArrayIndexMask),
-         &if_has_cached_index, &if_calculate_index);
-
-  BIND(&if_has_cached_index);
-  {
-    TNode<IntPtrT> index =
-        Signed(DecodeWordFromWord32<String::ArrayIndexValueBits>(hash));
-    CSA_ASSERT(this, IntPtrLessThan(index, IntPtrConstant(INT_MAX)));
-    var_intptr_key = index;
-    Goto(&done);
+  if (var_instance_type != nullptr) {
+    *var_instance_type = LoadInstanceType(CAST(key));
   }
 
-  BIND(&if_calculate_index);
-  {
-    Node* function =
-        ExternalConstant(ExternalReference::string_to_int32_function());
+  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)));
@@ -9808,17 +9789,6 @@ TNode<IntPtrT> CodeStubAssembler::TryToIntptr(
   GotoIf(Word32Equal(Int32Constant(-2), result), if_bailout);
   var_intptr_key = ChangeInt32ToIntPtr(result);
   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(&key_is_smi);
   {

src/codegen/external-reference.cc

@@ -641,7 +641,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_to_int32_function, String::ToInt32)
+FUNCTION_REFERENCE(object_to_array_index_slow_function,
+                   Object::ToArrayIndexSlow)
 
 static Address LexicographicCompareWrapper(Isolate* isolate, Address smi_x,
                                            Address smi_y) {

src/codegen/external-reference.h

@@ -159,6 +159,7 @@ 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")                            \
@@ -167,7 +168,6 @@ 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_int32_function, "String::ToInt32")                              \
   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

@@ -2598,58 +2598,22 @@ 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* vfalse = __ LoadField(AccessBuilder::ForHeapNumberValue(), value);
-  vfalse =
-      BuildCheckedFloat64ToInt32(CheckForMinusZeroMode::kDontCheckForMinusZero,
-                                 params.feedback(), vfalse, frame_state);
-  __ Goto(&done, vfalse);
-
-  __ 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);
-  Node* hash = __ LoadField(AccessBuilder::ForNameHashField(), value);
-  Node* has_cached_index = __ Word32Equal(
-      __ Word32And(hash,
-                   __ Int32Constant(Name::kDoesNotContainCachedArrayIndexMask)),
-      __ Int32Constant(0));
-  __ GotoIfNot(has_cached_index, &calculate_index);
-
-  Node* index = __ Word32Shr(
-      __ Word32And(hash, __ Int32Constant(String::ArrayIndexValueBits::kMask)),
-      __ Int32Constant(String::ArrayIndexValueBits::kShift));
-
-  __ Goto(&done, index);
-
-  __ Bind(&calculate_index);
-  {
   MachineSignature::Builder builder(graph()->zone(), 1, 1);
   builder.AddReturn(MachineType::Int32());
   builder.AddParam(MachineType::TaggedPointer());
-    Node* string_to_int32_function =
-        __ ExternalConstant(ExternalReference::string_to_int32_function());
+  Node* object_to_array_index_function = __ ExternalConstant(
+      ExternalReference::object_to_array_index_slow_function());
   auto call_descriptor =
       Linkage::GetSimplifiedCDescriptor(graph()->zone(), builder.Build());
   Node* index = __ Call(common()->Call(call_descriptor),
-                          string_to_int32_function, value);
+                        object_to_array_index_function, value);
 
   __ DeoptimizeIf(DeoptimizeReason::kNotAnArrayIndex, params.feedback(),
                   __ Int32LessThan(index, __ Int32Constant(0)), frame_state);
 
   __ Goto(&done, index);
-  }
+
   __ Bind(&done);
   return done.PhiAt(0);
 }

src/objects/objects.cc

@@ -600,6 +600,37 @@ 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,6 +315,18 @@ 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

@@ -903,25 +903,6 @@ ComparisonResult String::Compare(Isolate* isolate, Handle<String> x,
   return result;
 }
 
-int32_t String::ToInt32(Address key_addr) {
-  DisallowHeapAllocation no_gc;
-  String key(key_addr);
-  uint32_t index;
-  if (!key.AsArrayIndex(&index)) 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;
-}
-
 Object String::IndexOf(Isolate* isolate, Handle<Object> receiver,
                        Handle<Object> search, Handle<Object> position) {
   if (receiver->IsNullOrUndefined(isolate)) {

src/objects/string.h

@@ -323,10 +323,6 @@ class String : public TorqueGeneratedString<String, Name> {
   // integer in the range of a size_t. Useful for TypedArray accesses.
   inline bool AsIntegerIndex(size_t* index);
 
-  // TODO(gsathya): Change this to ToArrayIndex once CSA can handle
-  // UintPtr for element index access.
-  static int32_t ToInt32(Address key);
-
   // Trimming.
   enum TrimMode { kTrim, kTrimStart, kTrimEnd };
   static Handle<String> Trim(Isolate* isolate, Handle<String> string,