[turbofan] Add effects to StringAt operators

Add effect input and output to String.p.char[Code]At/codePointAt.
This is necessary to fix an hard to reproduce bug, a repro for
which is included. However, the only way to get the repro
included in this CL to fail is to run it with the patch of

  873382:
  [turbofan] Speculate on bounds checks for String#char[Code]At

but WITHOUT this patch. This fixes a scheduling problem triggered
by 873382 that caused a bounds check to get scheduled after the
associated access.

Bug: v8:7326
Change-Id: I4b97c1726caac92ff8f74c23df2788f0ecfb1304
Reviewed-on: https://chromium-review.googlesource.com/881781Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Sigurd Schneider <sigurds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50832}

src/compiler/js-builtin-reducer.cc

@@ -2032,81 +2032,10 @@ Reduction JSBuiltinReducer::ReduceStringIteratorNext(Node* node) {
     Node* vtrue0;
     {
       done_true = jsgraph()->FalseConstant();
-      Node* lead = graph()->NewNode(simplified()->StringCharCodeAt(), string,
-                                    index, if_true0);
-
-      // branch1: if ((lead & 0xFC00) === 0xD800)
-      Node* check1 =
-          graph()->NewNode(simplified()->NumberEqual(),
-                           graph()->NewNode(simplified()->NumberBitwiseAnd(),
-                                            lead, jsgraph()->Constant(0xFC00)),
-                           jsgraph()->Constant(0xD800));
-      Node* branch1 = graph()->NewNode(common()->Branch(BranchHint::kFalse),
-                                       check1, if_true0);
-      Node* if_true1 = graph()->NewNode(common()->IfTrue(), branch1);
-      Node* vtrue1;
-      {
-        Node* next_index = graph()->NewNode(simplified()->NumberAdd(), index,
-                                            jsgraph()->OneConstant());
-        // branch2: if ((index + 1) < length)
-        Node* check2 = graph()->NewNode(simplified()->NumberLessThan(),
-                                        next_index, length);
-        Node* branch2 = graph()->NewNode(common()->Branch(BranchHint::kTrue),
-                                         check2, if_true1);
-        Node* if_true2 = graph()->NewNode(common()->IfTrue(), branch2);
-        Node* vtrue2;
-        {
-          Node* trail = graph()->NewNode(simplified()->StringCharCodeAt(),
-                                         string, next_index, if_true2);
-          // branch3: if ((trail & 0xFC00) === 0xDC00)
-          Node* check3 = graph()->NewNode(
-              simplified()->NumberEqual(),
-              graph()->NewNode(simplified()->NumberBitwiseAnd(), trail,
-                               jsgraph()->Constant(0xFC00)),
-              jsgraph()->Constant(0xDC00));
-          Node* branch3 = graph()->NewNode(common()->Branch(BranchHint::kTrue),
-                                           check3, if_true2);
-          Node* if_true3 = graph()->NewNode(common()->IfTrue(), branch3);
-          Node* vtrue3;
-          {
-            vtrue3 = graph()->NewNode(
-                simplified()->NumberBitwiseOr(),
-// Need to swap the order for big-endian platforms
-#if V8_TARGET_BIG_ENDIAN
-                graph()->NewNode(simplified()->NumberShiftLeft(), lead,
-                                 jsgraph()->Constant(16)),
-                trail);
-#else
-                graph()->NewNode(simplified()->NumberShiftLeft(), trail,
-                                 jsgraph()->Constant(16)),
-                lead);
-#endif
-          }
-
-          Node* if_false3 = graph()->NewNode(common()->IfFalse(), branch3);
-          Node* vfalse3 = lead;
-          if_true2 = graph()->NewNode(common()->Merge(2), if_true3, if_false3);
-          vtrue2 =
-              graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2),
-                               vtrue3, vfalse3, if_true2);
-        }
-
-        Node* if_false2 = graph()->NewNode(common()->IfFalse(), branch2);
-        Node* vfalse2 = lead;
-        if_true1 = graph()->NewNode(common()->Merge(2), if_true2, if_false2);
-        vtrue1 =
-            graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2),
-                             vtrue2, vfalse2, if_true1);
-      }
-
-      Node* if_false1 = graph()->NewNode(common()->IfFalse(), branch1);
-      Node* vfalse1 = lead;
-      if_true0 = graph()->NewNode(common()->Merge(2), if_true1, if_false1);
-      vtrue0 =
-          graph()->NewNode(common()->Phi(MachineRepresentation::kWord32, 2),
-                           vtrue1, vfalse1, if_true0);
+      Node* codepoint = etrue0 = graph()->NewNode(
+          simplified()->StringCodePointAt(), string, index, etrue0, if_true0);
       vtrue0 = graph()->NewNode(
-          simplified()->StringFromCodePoint(UnicodeEncoding::UTF16), vtrue0);
+          simplified()->StringFromCodePoint(UnicodeEncoding::UTF16), codepoint);
 
       // Update iterator.[[NextIndex]]
       Node* char_length =
@@ -2173,6 +2102,7 @@ Reduction JSBuiltinReducer::ReduceStringSlice(Node* node) {
 
       Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
       Node* vfalse;
+      Node* efalse;
       {
         // We need to convince TurboFan that {receiver_length}-1 is a valid
         // Unsigned32 value, so we just apply NumberToUint32 to the result
@@ -2181,14 +2111,16 @@ Reduction JSBuiltinReducer::ReduceStringSlice(Node* node) {
             graph()->NewNode(simplified()->NumberSubtract(), receiver_length,
                              jsgraph()->OneConstant());
         index = graph()->NewNode(simplified()->NumberToUint32(), index);
-        vfalse = graph()->NewNode(simplified()->StringCharAt(), receiver, index,
-                                  if_false);
+        vfalse = efalse = graph()->NewNode(simplified()->StringCharAt(),
+                                           receiver, index, effect, if_false);
       }
 
       control = graph()->NewNode(common()->Merge(2), if_true, if_false);
       Node* value =
           graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2),
                            vtrue, vfalse, control);
+      effect =
+          graph()->NewNode(common()->EffectPhi(2), effect, efalse, control);
       ReplaceWithValue(node, value, effect, control);
       return Replace(value);
     }

src/compiler/js-call-reducer.cc

@@ -3900,9 +3900,9 @@ Reduction JSCallReducer::ReduceStringPrototypeStringAt(
 
   index = graph()->NewNode(simplified()->MaskIndexWithBound(), index,
                            receiver_length);
-
-  Node* vtrue =
-      graph()->NewNode(string_access_operator, receiver, index, if_true);
+  Node* etrue;
+  Node* vtrue = etrue = graph()->NewNode(string_access_operator, receiver,
+                                         index, effect, if_true);
 
   // Return the {default_return} otherwise.
   Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
@@ -3911,6 +3911,7 @@ Reduction JSCallReducer::ReduceStringPrototypeStringAt(
   control = graph()->NewNode(common()->Merge(2), if_true, if_false);
   Node* value = graph()->NewNode(
       common()->Phi(MachineRepresentation::kTagged, 2), vtrue, vfalse, control);
+  effect = graph()->NewNode(common()->EffectPhi(2), etrue, effect, control);
 
   ReplaceWithValue(node, value, effect, control);
   return Replace(value);

src/compiler/js-native-context-specialization.cc

@@ -2524,13 +2524,16 @@ Node* JSNativeContextSpecialization::BuildIndexedStringLoad(
         graph()->NewNode(simplified()->MaskIndexWithBound(), index, length);
 
     Node* if_true = graph()->NewNode(common()->IfTrue(), branch);
-    Node* vtrue = graph()->NewNode(simplified()->StringCharAt(), receiver,
-                                   masked_index, if_true);
+    Node* etrue;
+    Node* vtrue = etrue = graph()->NewNode(
+        simplified()->StringCharAt(), receiver, masked_index, *effect, if_true);
 
     Node* if_false = graph()->NewNode(common()->IfFalse(), branch);
     Node* vfalse = jsgraph()->UndefinedConstant();
 
     *control = graph()->NewNode(common()->Merge(2), if_true, if_false);
+    *effect =
+        graph()->NewNode(common()->EffectPhi(2), etrue, *effect, *control);
     return graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2),
                             vtrue, vfalse, *control);
   } else {
@@ -2543,8 +2546,10 @@ Node* JSNativeContextSpecialization::BuildIndexedStringLoad(
         graph()->NewNode(simplified()->MaskIndexWithBound(), index, length);
 
     // Return the character from the {receiver} as single character string.
-    return graph()->NewNode(simplified()->StringCharAt(), receiver,
-                            masked_index, *control);
+    Node* value = *effect =
+        graph()->NewNode(simplified()->StringCharAt(), receiver, masked_index,
+                         *effect, *control);
+    return value;
   }
 }
 

src/compiler/simplified-operator.cc

@@ -655,11 +655,6 @@ bool operator==(CheckMinusZeroParameters const& lhs,
   V(NumberToUint8Clamped, Operator::kNoProperties, 1, 0)         \
   V(NumberSilenceNaN, Operator::kNoProperties, 1, 0)             \
   V(StringToNumber, Operator::kNoProperties, 1, 0)               \
-  V(StringCharAt, Operator::kNoProperties, 2, 1)                 \
-  V(StringCharCodeAt, Operator::kNoProperties, 2, 1)             \
-  V(SeqStringCharCodeAt, Operator::kNoProperties, 2, 1)          \
-  V(StringCodePointAt, Operator::kNoProperties, 2, 1)            \
-  V(SeqStringCodePointAt, Operator::kNoProperties, 2, 1)         \
   V(StringFromCharCode, Operator::kNoProperties, 1, 0)           \
   V(StringIndexOf, Operator::kNoProperties, 3, 0)                \
   V(StringLength, Operator::kNoProperties, 1, 0)                 \
@@ -710,6 +705,13 @@ bool operator==(CheckMinusZeroParameters const& lhs,
   V(NewConsString, Operator::kNoProperties, 3, 0)                \
   V(MaskIndexWithBound, Operator::kNoProperties, 2, 0)
 
+#define EFFECT_DEPENDENT_OP_LIST(V)                     \
+  V(StringCharAt, Operator::kNoProperties, 2, 1)        \
+  V(StringCharCodeAt, Operator::kNoProperties, 2, 1)    \
+  V(SeqStringCharCodeAt, Operator::kNoProperties, 2, 1) \
+  V(StringCodePointAt, Operator::kNoProperties, 2, 1)   \
+  V(SeqStringCodePointAt, Operator::kNoProperties, 2, 1)
+
 #define SPECULATIVE_NUMBER_BINOP_LIST(V)      \
   SIMPLIFIED_SPECULATIVE_NUMBER_BINOP_LIST(V) \
   V(SpeculativeNumberEqual)                   \
@@ -755,6 +757,20 @@ struct SimplifiedOperatorGlobalCache final {
   PURE_OP_LIST(PURE)
 #undef PURE
 
+#define EFFECT_DEPENDENT(Name, properties, value_input_count,              \
+                         control_input_count)                              \
+  struct Name##Operator final : public Operator {                          \
+    Name##Operator()                                                       \
+        : Operator(IrOpcode::k##Name,                                      \
+                   Operator::kNoDeopt | Operator::kNoWrite |               \
+                       Operator::kNoThrow | properties,                    \
+                   #Name, value_input_count, 1, control_input_count, 1, 1, \
+                   0) {}                                                   \
+  };                                                                       \
+  Name##Operator k##Name;
+  EFFECT_DEPENDENT_OP_LIST(EFFECT_DEPENDENT)
+#undef EFFECT_DEPENDENT
+
 #define CHECKED(Name, value_input_count, value_output_count)             \
   struct Name##Operator final : public Operator {                        \
     Name##Operator()                                                     \
@@ -1032,6 +1048,7 @@ SimplifiedOperatorBuilder::SimplifiedOperatorBuilder(Zone* zone)
 #define GET_FROM_CACHE(Name, ...) \
   const Operator* SimplifiedOperatorBuilder::Name() { return &cache_.k##Name; }
 PURE_OP_LIST(GET_FROM_CACHE)
+EFFECT_DEPENDENT_OP_LIST(GET_FROM_CACHE)
 CHECKED_OP_LIST(GET_FROM_CACHE)
 GET_FROM_CACHE(ArrayBufferWasNeutered)
 GET_FROM_CACHE(ArgumentsFrame)
@@ -1463,6 +1480,7 @@ const Operator* SimplifiedOperatorBuilder::TransitionAndStoreNonNumberElement(
 }
 
 #undef PURE_OP_LIST
+#undef EFFECT_DEPENDENT_OP_LIST
 #undef SPECULATIVE_NUMBER_BINOP_LIST
 #undef CHECKED_WITH_FEEDBACK_OP_LIST
 #undef CHECKED_OP_LIST

test/mjsunit/regress/regress-stringAt-boundsCheck.js

+// Copyright 2018 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: --opt --allow-natives-syntax
+
+(() => {
+  function f(u) {
+    for (var j = 0; j < 20; ++j) {
+        print("" + u.codePointAt());
+    }
+  }
+
+  f("test");
+  f("foo");
+  %OptimizeFunctionOnNextCall(f);
+  f("");
+})();