[turbofan] put TypeGuard into the effect chain and maintain it until EffectControlLinearizer

We need to maintain TypeGuard nodes until the EffectControlLinearizer, because they can protect partial operations from floating above a check. In the linked bug, it was a DeadValue node that got scheduled too early.

In LoadElimination and EscapeAnalysis, the inserted TypeGuard nodes might depend on map checks on the effect chain. Thus TypeGuard has to be an effect chain node too.

Bug: chromium:800929
Change-Id: Icdcff96a2273d96b7f8cd6f85511ad62c1cb129a
Reviewed-on: https://chromium-review.googlesource.com/860405Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50661}

src/compiler/bytecode-graph-builder.cc

@@ -2671,7 +2671,9 @@ void BytecodeGraphBuilder::VisitForInNext() {
   // We need to rename the {index} here, as in case of OSR we loose the
   // information that the {index} is always a valid unsigned Smi value.
   index = graph()->NewNode(common()->TypeGuard(Type::UnsignedSmall()), index,
+                           environment()->GetEffectDependency(),
                            environment()->GetControlDependency());
+  environment()->UpdateEffectDependency(index);
 
   FeedbackSlot slot =
       feedback_vector()->ToSlot(bytecode_iterator().GetIndexOperand(3));

src/compiler/common-operator.cc

@@ -1136,7 +1136,7 @@ const Operator* CommonOperatorBuilder::TypeGuard(Type* type) {
   return new (zone()) Operator1<Type*>(       // --
       IrOpcode::kTypeGuard, Operator::kPure,  // opcode
       "TypeGuard",                            // name
-      1, 0, 1, 1, 0, 0,                       // counts
+      1, 1, 1, 1, 1, 0,                       // counts
       type);                                  // parameter
 }
 

src/compiler/effect-control-linearizer.cc

@@ -145,9 +145,10 @@ bool HasIncomingBackEdges(BasicBlock* block) {
   return false;
 }
 
-void RemoveRegionNode(Node* node) {
+void RemoveRenameNode(Node* node) {
   DCHECK(IrOpcode::kFinishRegion == node->opcode() ||
-         IrOpcode::kBeginRegion == node->opcode());
+         IrOpcode::kBeginRegion == node->opcode() ||
+         IrOpcode::kTypeGuard == node->opcode());
   // Update the value/context uses to the value input of the finish node and
   // the effect uses to the effect input.
   for (Edge edge : node->use_edges()) {
@@ -538,7 +539,7 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state,
     region_observability_ = RegionObservability::kObservable;
     // Update the value uses to the value input of the finish node and
     // the effect uses to the effect input.
-    return RemoveRegionNode(node);
+    return RemoveRenameNode(node);
   }
   if (node->opcode() == IrOpcode::kBeginRegion) {
     // Determine the observability for this region and use that for all
@@ -548,7 +549,10 @@ void EffectControlLinearizer::ProcessNode(Node* node, Node** frame_state,
     region_observability_ = RegionObservabilityOf(node->op());
     // Update the value uses to the value input of the finish node and
     // the effect uses to the effect input.
-    return RemoveRegionNode(node);
+    return RemoveRenameNode(node);
+  }
+  if (node->opcode() == IrOpcode::kTypeGuard) {
+    return RemoveRenameNode(node);
   }
 
   // Special treatment for checkpoint nodes.

src/compiler/escape-analysis-reducer.cc

@@ -33,18 +33,39 @@ EscapeAnalysisReducer::EscapeAnalysisReducer(
       arguments_elements_(zone),
       zone_(zone) {}
 
-Node* EscapeAnalysisReducer::MaybeGuard(Node* original, Node* replacement) {
-  // We might need to guard the replacement if the type of the {replacement}
-  // node is not in a sub-type relation to the type of the the {original} node.
+Reduction EscapeAnalysisReducer::ReplaceNode(Node* original,
+                                             Node* replacement) {
+  const VirtualObject* vobject =
+      analysis_result().GetVirtualObject(replacement);
+  if (replacement->opcode() == IrOpcode::kDead ||
+      (vobject && !vobject->HasEscaped())) {
+    RelaxEffectsAndControls(original);
+    return Replace(replacement);
+  }
   Type* const replacement_type = NodeProperties::GetType(replacement);
   Type* const original_type = NodeProperties::GetType(original);
-  if (!replacement_type->Is(original_type)) {
-    Node* const control = NodeProperties::GetControlInput(original);
-    replacement = jsgraph()->graph()->NewNode(
-        jsgraph()->common()->TypeGuard(original_type), replacement, control);
-    NodeProperties::SetType(replacement, original_type);
+  if (replacement_type->Is(original_type)) {
+    RelaxEffectsAndControls(original);
+    return Replace(replacement);
   }
-  return replacement;
+
+  // We need to guard the replacement if we would widen the type otherwise.
+  DCHECK_EQ(1, original->op()->EffectOutputCount());
+  DCHECK_EQ(1, original->op()->EffectInputCount());
+  DCHECK_EQ(1, original->op()->ControlInputCount());
+  Node* effect = NodeProperties::GetEffectInput(original);
+  Node* control = NodeProperties::GetControlInput(original);
+  original->TrimInputCount(0);
+  original->AppendInput(jsgraph()->zone(), replacement);
+  original->AppendInput(jsgraph()->zone(), effect);
+  original->AppendInput(jsgraph()->zone(), control);
+  NodeProperties::SetType(
+      original,
+      Type::Intersect(original_type, replacement_type, jsgraph()->zone()));
+  NodeProperties::ChangeOp(original,
+                           jsgraph()->common()->TypeGuard(original_type));
+  ReplaceWithValue(original, original, original, control);
+  return NoChange();
 }
 
 namespace {
@@ -74,11 +95,7 @@ Reduction EscapeAnalysisReducer::Reduce(Node* node) {
     DCHECK(node->opcode() != IrOpcode::kAllocate &&
            node->opcode() != IrOpcode::kFinishRegion);
     DCHECK_NE(replacement, node);
-    if (replacement != jsgraph()->Dead()) {
-      replacement = MaybeGuard(node, replacement);
-    }
-    RelaxEffectsAndControls(node);
-    return Replace(replacement);
+    return ReplaceNode(node, replacement);
   }
 
   switch (node->opcode()) {

src/compiler/escape-analysis-reducer.h

@@ -97,7 +97,7 @@ class V8_EXPORT_PRIVATE EscapeAnalysisReducer final
   void ReduceFrameStateInputs(Node* node);
   Node* ReduceDeoptState(Node* node, Node* effect, Deduplicator* deduplicator);
   Node* ObjectIdNode(const VirtualObject* vobject);
-  Node* MaybeGuard(Node* original, Node* replacement);
+  Reduction ReplaceNode(Node* original, Node* replacement);
 
   JSGraph* jsgraph() const { return jsgraph_; }
   EscapeAnalysisResult analysis_result() const { return analysis_result_; }

src/compiler/graph-trimmer.cc

@@ -25,6 +25,7 @@ bool IsSideEffectFree(const Operator* op) {
     case IrOpcode::kLoad:
     case IrOpcode::kLoadElement:
     case IrOpcode::kLoadField:
+    case IrOpcode::kTypeGuard:
       return true;
     default:
       return false;

src/compiler/js-builtin-reducer.cc

@@ -1139,8 +1139,9 @@ Reduction JSBuiltinReducer::ReduceCollectionIteratorNext(
           // Abort loop with resulting value.
           Node* control = graph()->NewNode(common()->IfFalse(), branch1);
           Node* effect = etrue0;
-          Node* value = graph()->NewNode(
-              common()->TypeGuard(Type::NonInternal()), entry_key, control);
+          Node* value = effect =
+              graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
+                               entry_key, effect, control);
           Node* done = jsgraph()->FalseConstant();
 
           // Advance the index on the {receiver}.

src/compiler/js-call-reducer.cc

@@ -950,8 +950,8 @@ Reduction JSCallReducer::ReduceArrayForEach(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   frame_state = CreateJavaScriptBuiltinContinuationFrameState(
@@ -1164,8 +1164,8 @@ Reduction JSCallReducer::ReduceArrayReduce(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   frame_state = CreateJavaScriptBuiltinContinuationFrameState(
@@ -1401,8 +1401,8 @@ Reduction JSCallReducer::ReduceArrayReduceRight(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   frame_state = CreateJavaScriptBuiltinContinuationFrameState(
@@ -1612,8 +1612,8 @@ Reduction JSCallReducer::ReduceArrayMap(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   // This frame state is dealt with by hand in
@@ -1838,8 +1838,8 @@ Reduction JSCallReducer::ReduceArrayFilter(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   Node* callback_value = nullptr;
@@ -2160,8 +2160,8 @@ Node* JSCallReducer::DoFilterPostCallbackWork(ElementsKind kind, Node** control,
 
     // We know that {to} is in Unsigned31 range here, being smaller than
     // {original_length} at all times.
-    Node* checked_to =
-        graph()->NewNode(common()->TypeGuard(Type::Unsigned31()), to, if_true);
+    Node* checked_to = etrue = graph()->NewNode(
+        common()->TypeGuard(Type::Unsigned31()), to, etrue, if_true);
     Node* elements_length = etrue = graph()->NewNode(
         simplified()->LoadField(AccessBuilder::ForFixedArrayLength()), elements,
         etrue, if_true);
@@ -2405,8 +2405,8 @@ Reduction JSCallReducer::ReduceArrayEvery(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   Node* callback_value = nullptr;
@@ -2632,8 +2632,8 @@ Reduction JSCallReducer::ReduceArraySome(Handle<JSFunction> function,
     // The contract is that we don't leak "the hole" into "user JavaScript",
     // so we must rename the {element} here to explicitly exclude "the hole"
     // from the type of {element}.
-    element = graph()->NewNode(common()->TypeGuard(Type::NonInternal()),
-                               element, control);
+    element = effect = graph()->NewNode(
+        common()->TypeGuard(Type::NonInternal()), element, effect, control);
   }
 
   Node* callback_value = nullptr;

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

@@ -2584,8 +2584,8 @@ Node* JSNativeContextSpecialization::BuildExtendPropertiesBackingStore(
         common()->Select(MachineRepresentation::kTaggedSigned),
         graph()->NewNode(simplified()->ObjectIsSmi(), properties), properties,
         jsgraph()->SmiConstant(PropertyArray::kNoHashSentinel));
-    hash = graph()->NewNode(common()->TypeGuard(Type::SignedSmall()), hash,
-                            control);
+    hash = effect = graph()->NewNode(common()->TypeGuard(Type::SignedSmall()),
+                                     hash, effect, control);
     hash =
         graph()->NewNode(simplified()->NumberShiftLeft(), hash,
                          jsgraph()->Constant(PropertyArray::HashField::kShift));

src/compiler/js-typed-lowering.cc

@@ -707,8 +707,9 @@ Reduction JSTypedLowering::ReduceCreateConsString(Node* node) {
       Revisit(graph()->end());
     }
     control = graph()->NewNode(common()->IfTrue(), branch);
-    length = graph()->NewNode(
-        common()->TypeGuard(type_cache_.kStringLengthType), length, control);
+    length = effect =
+        graph()->NewNode(common()->TypeGuard(type_cache_.kStringLengthType),
+                         length, effect, control);
   }
 
   Node* value =

src/compiler/loop-variable-optimizer.cc

@@ -301,7 +301,8 @@ const InductionVariable* LoopVariableOptimizer::FindInductionVariable(
 
 InductionVariable* LoopVariableOptimizer::TryGetInductionVariable(Node* phi) {
   DCHECK_EQ(2, phi->op()->ValueInputCount());
-  DCHECK_EQ(IrOpcode::kLoop, NodeProperties::GetControlInput(phi)->opcode());
+  Node* loop = NodeProperties::GetControlInput(phi);
+  DCHECK_EQ(IrOpcode::kLoop, loop->opcode());
   Node* initial = phi->InputAt(0);
   Node* arith = phi->InputAt(1);
   InductionVariable::ArithmeticType arithmeticType;
@@ -320,9 +321,18 @@ InductionVariable* LoopVariableOptimizer::TryGetInductionVariable(Node* phi) {
   // TODO(jarin) Support both sides.
   if (arith->InputAt(0) != phi) return nullptr;
 
+  Node* effect_phi = nullptr;
+  for (Node* use : loop->uses()) {
+    if (use->opcode() == IrOpcode::kEffectPhi) {
+      DCHECK_NULL(effect_phi);
+      effect_phi = use;
+    }
+  }
+  if (!effect_phi) return nullptr;
+
   Node* incr = arith->InputAt(1);
-  return new (zone())
-      InductionVariable(phi, arith, incr, initial, zone(), arithmeticType);
+  return new (zone()) InductionVariable(phi, effect_phi, arith, incr, initial,
+                                        zone(), arithmeticType);
 }
 
 void LoopVariableOptimizer::DetectInductionVariables(Node* loop) {
@@ -392,10 +402,14 @@ void LoopVariableOptimizer::ChangeToPhisAndInsertGuards() {
       Type* backedge_type = NodeProperties::GetType(backedge_value);
       Type* phi_type = NodeProperties::GetType(induction_var->phi());
       if (!backedge_type->Is(phi_type)) {
-        Node* backedge_control =
-            NodeProperties::GetControlInput(induction_var->phi())->InputAt(1);
-        Node* rename = graph()->NewNode(common()->TypeGuard(phi_type),
-                                        backedge_value, backedge_control);
+        Node* loop = NodeProperties::GetControlInput(induction_var->phi());
+        Node* backedge_control = loop->InputAt(1);
+        Node* backedge_effect =
+            NodeProperties::GetEffectInput(induction_var->effect_phi(), 1);
+        Node* rename =
+            graph()->NewNode(common()->TypeGuard(phi_type), backedge_value,
+                             backedge_effect, backedge_control);
+        induction_var->effect_phi()->ReplaceInput(1, rename);
         induction_var->phi()->ReplaceInput(1, rename);
       }
     }

src/compiler/loop-variable-optimizer.h

@@ -18,6 +18,7 @@ class Node;
 class InductionVariable : public ZoneObject {
  public:
   Node* phi() const { return phi_; }
+  Node* effect_phi() const { return effect_phi_; }
   Node* arith() const { return arith_; }
   Node* increment() const { return increment_; }
   Node* init_value() const { return init_value_; }
@@ -39,9 +40,10 @@ class InductionVariable : public ZoneObject {
  private:
   friend class LoopVariableOptimizer;
 
-  InductionVariable(Node* phi, Node* arith, Node* increment, Node* init_value,
-                    Zone* zone, ArithmeticType arithmeticType)
+  InductionVariable(Node* phi, Node* effect_phi, Node* arith, Node* increment,
+                    Node* init_value, Zone* zone, ArithmeticType arithmeticType)
       : phi_(phi),
+        effect_phi_(effect_phi),
         arith_(arith),
         increment_(increment),
         init_value_(init_value),
@@ -53,6 +55,7 @@ class InductionVariable : public ZoneObject {
   void AddLowerBound(Node* bound, ConstraintKind kind);
 
   Node* phi_;
+  Node* effect_phi_;
   Node* arith_;
   Node* increment_;
   Node* init_value_;

src/compiler/simplified-lowering.cc

@@ -2977,7 +2977,6 @@ class RepresentationSelector {
         }
         ProcessRemainingInputs(node, 1);
         SetOutput(node, representation);
-        if (lower()) DeferReplacement(node, node->InputAt(0));
         return;
       }