Revert of [turbofan] Speculatively optimize string character access. (patchset...

Revert of [turbofan] Speculatively optimize string character access. (patchset #1 id:1 of https://codereview.chromium.org/2905623003/ )

Reason for revert:
Changes layout tests:
https://build.chromium.org/p/client.v8.fyi/builders/V8-Blink%20Linux%2064/builds/15867

See:
https://github.com/v8/v8/wiki/Blink-layout-tests

Original issue's description:
> [turbofan] Speculatively optimize string character access.
>
> Add a protector cell for string bounds checks that is being used to
> protect speculative bounds for String.prototype.charCodeAt and
> String.prototype.charAt in TurboFan (and Crankshaft). This way we don't
> have the diamond in optimized code, which stands in the way of other
> optimizations for charCodeAt that are currently being worked on by
> petermarshall@.
>
> BUG=v8:6391
> TBR=mlippautz@chromium.org
> R=petermarshall@chromium.org
>
> Review-Url: https://codereview.chromium.org/2905623003
> Cr-Commit-Position: refs/heads/master@{#45514}
> Committed: https://chromium.googlesource.com/v8/v8/+/9d8bd0551657d60c66b2f96544eecedfba1cba8a

TBR=petermarshall@chromium.org,mlippautz@chromium.org,bmeurer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:6391

Review-Url: https://codereview.chromium.org/2900333002
Cr-Commit-Position: refs/heads/master@{#45521}

src/builtins/builtins-string-gen.cc

@@ -656,13 +656,7 @@ TF_BUILTIN(StringPrototypeCharAt, CodeStubAssembler) {
            &if_positioninbounds);
 
     BIND(&return_emptystring);
-    {
-      // Invalidate the "String Bounds Check" protector.
-      Node* invalid = SmiConstant(Isolate::kProtectorInvalid);
-      Node* cell = LoadRoot(Heap::kStringBoundsCheckProtectorRootIndex);
-      StoreObjectFieldNoWriteBarrier(cell, Cell::kValueOffset, invalid);
-      Return(EmptyStringConstant());
-    }
+    Return(EmptyStringConstant());
 
     BIND(&if_positioninbounds);
   }
@@ -701,13 +695,7 @@ TF_BUILTIN(StringPrototypeCharCodeAt, CodeStubAssembler) {
            &if_positioninbounds);
 
     BIND(&return_nan);
-    {
-      // Invalidate the "String Bounds Check" protector.
-      Node* invalid = SmiConstant(Isolate::kProtectorInvalid);
-      Node* cell = LoadRoot(Heap::kStringBoundsCheckProtectorRootIndex);
-      StoreObjectFieldNoWriteBarrier(cell, Cell::kValueOffset, invalid);
-      Return(NaNConstant());
-    }
+    Return(NaNConstant());
 
     BIND(&if_positioninbounds);
   }

src/compiler/js-builtin-reducer.cc

@@ -1922,26 +1922,8 @@ Reduction JSBuiltinReducer::ReduceStringCharAt(Node* node) {
     Node* effect = NodeProperties::GetEffectInput(node);
     Node* control = NodeProperties::GetControlInput(node);
 
-    if (Node* receiver = GetStringWitness(node)) {
-      if (isolate()->IsStringBoundsCheckIntact()) {
-        // Determine the {receiver} length.
-        Node* receiver_length = effect = graph()->NewNode(
-            simplified()->LoadField(AccessBuilder::ForStringLength()), receiver,
-            effect, control);
-
-        // Check that the {index} is within the range of {receiver}.
-        index = effect = graph()->NewNode(simplified()->CheckBounds(), index,
-                                          receiver_length, effect, control);
-
-        // Return the character from the {receiver} as single character string.
-        Node* value = graph()->NewNode(simplified()->StringCharAt(), receiver,
-                                       index, control);
-
-        ReplaceWithValue(node, value, effect, control);
-        return Replace(value);
-      }
-
-      if (index_type->Is(Type::Integral32OrMinusZeroOrNaN())) {
+    if (index_type->Is(Type::Integral32OrMinusZeroOrNaN())) {
+      if (Node* receiver = GetStringWitness(node)) {
         if (!index_type->Is(Type::Unsigned32())) {
           // Map -0 and NaN to 0 (as per ToInteger), and the values in
           // the [-2^31,-1] range to the [2^31,2^32-1] range, which will
@@ -1994,26 +1976,8 @@ Reduction JSBuiltinReducer::ReduceStringCharCodeAt(Node* node) {
     Node* effect = NodeProperties::GetEffectInput(node);
     Node* control = NodeProperties::GetControlInput(node);
 
-    if (Node* receiver = GetStringWitness(node)) {
-      if (isolate()->IsStringBoundsCheckIntact()) {
-        // Determine the {receiver} length.
-        Node* receiver_length = effect = graph()->NewNode(
-            simplified()->LoadField(AccessBuilder::ForStringLength()), receiver,
-            effect, control);
-
-        // Check that the {index} is within the range of {receiver}.
-        index = effect = graph()->NewNode(simplified()->CheckBounds(), index,
-                                          receiver_length, effect, control);
-
-        // Return the character from the {receiver} as character code.
-        Node* value = graph()->NewNode(simplified()->StringCharCodeAt(),
-                                       receiver, index, control);
-
-        ReplaceWithValue(node, value, effect, control);
-        return Replace(value);
-      }
-
-      if (index_type->Is(Type::Integral32OrMinusZeroOrNaN())) {
+    if (index_type->Is(Type::Integral32OrMinusZeroOrNaN())) {
+      if (Node* receiver = GetStringWitness(node)) {
         if (!index_type->Is(Type::Unsigned32())) {
           // Map -0 and NaN to 0 (as per ToInteger), and the values in
           // the [-2^31,-1] range to the [2^31,2^32-1] range, which will

src/crankshaft/hydrogen.cc

@@ -8351,7 +8351,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall(
     }
     case kStringCharCodeAt:
     case kStringCharAt:
-      if (argument_count == 2 && isolate()->IsStringBoundsCheckIntact()) {
+      if (argument_count == 2) {
         HValue* index = Pop();
         HValue* string = Pop();
         Drop(1);  // Function.

src/heap/heap.cc

@@ -2855,10 +2855,6 @@ void Heap::CreateInitialObjects() {
       handle(Smi::FromInt(Isolate::kProtectorValid), isolate()));
   set_species_protector(*species_cell);
 
-  Handle<Cell> string_bounds_check_cell = factory->NewCell(
-      handle(Smi::FromInt(Isolate::kProtectorValid), isolate()));
-  set_string_bounds_check_protector(*string_bounds_check_cell);
-
   cell = factory->NewPropertyCell();
   cell->set_value(Smi::FromInt(Isolate::kProtectorValid));
   set_string_length_protector(*cell);

src/heap/heap.h

@@ -172,7 +172,6 @@ using v8::MemoryPressureLevel;
   V(PropertyCell, array_protector, ArrayProtector)                             \
   V(Cell, is_concat_spreadable_protector, IsConcatSpreadableProtector)         \
   V(Cell, species_protector, SpeciesProtector)                                 \
-  V(Cell, string_bounds_check_protector, StringBoundsCheckProtector)           \
   V(PropertyCell, string_length_protector, StringLengthProtector)              \
   V(Cell, fast_array_iteration_protector, FastArrayIterationProtector)         \
   V(PropertyCell, array_iterator_protector, ArrayIteratorProtector)            \

src/isolate-inl.h

@@ -133,11 +133,6 @@ bool Isolate::IsArraySpeciesLookupChainIntact() {
          Smi::cast(species_cell->value())->value() == kProtectorValid;
 }
 
-bool Isolate::IsStringBoundsCheckIntact() {
-  Cell* string_bounds_check_cell = heap()->string_bounds_check_protector();
-  return string_bounds_check_cell->value() == Smi::FromInt(kProtectorValid);
-}
-
 bool Isolate::IsStringLengthOverflowIntact() {
   PropertyCell* string_length_cell = heap()->string_length_protector();
   return string_length_cell->value() == Smi::FromInt(kProtectorValid);

src/isolate.h

@@ -1041,7 +1041,6 @@ class Isolate {
   inline bool IsArraySpeciesLookupChainIntact();
   bool IsIsConcatSpreadableLookupChainIntact();
   bool IsIsConcatSpreadableLookupChainIntact(JSReceiver* receiver);
-  inline bool IsStringBoundsCheckIntact();
   inline bool IsStringLengthOverflowIntact();
   inline bool IsArrayIteratorLookupChainIntact();
 

test/mjsunit/regress/regress-6391.js

-// Copyright 2017 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 --opt --no-always-opt
-
-(function() {
-  function foo(i) { return "a".charAt(i); }
-
-  assertEquals("a", foo(0));
-  assertEquals("a", foo(0));
-  assertEquals("", foo(1));
-  %OptimizeFunctionOnNextCall(foo);
-  assertEquals("", foo(1));
-  assertOptimized(foo);
-})();
-
-(function() {
-  function foo(i) { return "a".charCodeAt(i); }
-
-  assertEquals(97, foo(0));
-  assertEquals(97, foo(0));
-  assertEquals(NaN, foo(1));
-  %OptimizeFunctionOnNextCall(foo);
-  assertEquals(NaN, foo(1));
-  assertOptimized(foo);
-})();