[turbofan] Deprecate NodeProperties::ReplaceWithValue.

This deprecates the aforementioned mutator in favor of a simpler
NodeProperties::ReplaceUses that doesn't perform any relaxation.
Preparation for enabling support for try-catch statements.

R=bmeurer@chromium.org
TEST=unittests/NodePropertiesTest

Review URL: https://codereview.chromium.org/1172773003

Cr-Commit-Position: refs/heads/master@{#28897}

src/compiler/ast-graph-builder.cc

@@ -2529,6 +2529,9 @@ void AstGraphBuilder::VisitCallRuntime(CallRuntime* expr) {
 
   // Create node to perform the runtime call.
   Runtime::FunctionId functionId = function->function_id;
+  // TODO(mstarzinger): This bailout is a gigantic hack, the owner is ashamed.
+  if (functionId == Runtime::kInlineGeneratorNext) SetStackOverflow();
+  if (functionId == Runtime::kInlineGeneratorThrow) SetStackOverflow();
   const Operator* call = javascript()->CallRuntime(functionId, args->length());
   Node* value = ProcessArguments(call, args->length());
   PrepareFrameState(value, expr->id(), ast_context()->GetStateCombine());

src/compiler/change-lowering.cc

@@ -257,19 +257,18 @@ Reduction ChangeLowering::ChangeTaggedToFloat64(Node* value, Node* control) {
     Node* vtrue1 = graph()->NewNode(value->op(), object, context, frame_state,
                                     effect, if_true1);
     Node* etrue1 = vtrue1;
-    {
-      Node* check2 = TestNotSmi(vtrue1);
-      Node* branch2 = graph()->NewNode(common()->Branch(), check2, if_true1);
 
-      Node* if_true2 = graph()->NewNode(common()->IfTrue(), branch2);
-      Node* vtrue2 = LoadHeapNumberValue(vtrue1, if_true2);
+    Node* check2 = TestNotSmi(vtrue1);
+    Node* branch2 = graph()->NewNode(common()->Branch(), check2, if_true1);
 
-      Node* if_false2 = graph()->NewNode(common()->IfFalse(), branch2);
-      Node* vfalse2 = ChangeSmiToFloat64(vtrue1);
+    Node* if_true2 = graph()->NewNode(common()->IfTrue(), branch2);
+    Node* vtrue2 = LoadHeapNumberValue(vtrue1, if_true2);
 
-      if_true1 = graph()->NewNode(merge_op, if_true2, if_false2);
-      vtrue1 = graph()->NewNode(phi_op, vtrue2, vfalse2, if_true1);
-    }
+    Node* if_false2 = graph()->NewNode(common()->IfFalse(), branch2);
+    Node* vfalse2 = ChangeSmiToFloat64(vtrue1);
+
+    if_true1 = graph()->NewNode(merge_op, if_true2, if_false2);
+    vtrue1 = graph()->NewNode(phi_op, vtrue2, vfalse2, if_true1);
 
     Node* if_false1 = graph()->NewNode(common()->IfFalse(), branch1);
     Node* vfalse1 = ChangeSmiToFloat64(object);
@@ -279,7 +278,18 @@ Reduction ChangeLowering::ChangeTaggedToFloat64(Node* value, Node* control) {
     Node* ephi1 = graph()->NewNode(ephi_op, etrue1, efalse1, merge1);
     Node* phi1 = graph()->NewNode(phi_op, vtrue1, vfalse1, merge1);
 
-    NodeProperties::ReplaceWithValue(value, phi1, ephi1, merge1);
+    // Wire the new diamond into the graph, {JSToNumber} can still throw.
+    NodeProperties::ReplaceUses(value, phi1, ephi1, etrue1, etrue1);
+
+    // TODO(mstarzinger): This iteration cuts out the IfSuccess projection from
+    // the node and places it inside the diamond. Come up with a helper method!
+    for (Node* use : etrue1->uses()) {
+      if (use->opcode() == IrOpcode::kIfSuccess) {
+        use->ReplaceUses(merge1);
+        NodeProperties::ReplaceControlInput(branch2, use);
+      }
+    }
+
     return Replace(phi1);
   }
 

src/compiler/js-generic-lowering.cc

@@ -195,7 +195,7 @@ void JSGenericLowering::ReplaceWithCompareIC(Node* node, Token::Value token) {
   Node* booleanize = graph()->NewNode(op, compare, jsgraph()->ZeroConstant());
 
   // Finally patch the original node to select a boolean.
-  NodeProperties::ReplaceWithValue(node, node, compare);
+  NodeProperties::ReplaceUses(node, node, compare, compare, compare);
   // TODO(mstarzinger): Just a work-around because SelectLowering might
   // otherwise introduce a Phi without any uses, making Scheduler unhappy.
   if (node->UseCount() == 0) return;
@@ -449,7 +449,7 @@ void JSGenericLowering::LowerJSLoadDynamicGlobal(Node* node) {
       (access.mode() == CONTEXTUAL) ? Runtime::kLoadLookupSlot
                                     : Runtime::kLoadLookupSlotNoReferenceError;
   Node* projection = graph()->NewNode(common()->Projection(0), node);
-  NodeProperties::ReplaceWithValue(node, projection, node, node);
+  NodeProperties::ReplaceUses(node, projection, node, node, node);
   node->RemoveInput(NodeProperties::FirstFrameStateIndex(node) + 1);
   node->RemoveInput(NodeProperties::FirstValueIndex(node));
   node->InsertInput(zone(), 1, jsgraph()->Constant(access.name()));
@@ -461,7 +461,7 @@ void JSGenericLowering::LowerJSLoadDynamicGlobal(Node* node) {
 void JSGenericLowering::LowerJSLoadDynamicContext(Node* node) {
   const DynamicContextAccess& access = DynamicContextAccessOf(node->op());
   Node* projection = graph()->NewNode(common()->Projection(0), node);
-  NodeProperties::ReplaceWithValue(node, projection, node, node);
+  NodeProperties::ReplaceUses(node, projection, node, node, node);
   node->InsertInput(zone(), 1, jsgraph()->Constant(access.name()));
   ReplaceWithRuntimeCall(node, Runtime::kLoadLookupSlot);
   projection->ReplaceInput(0, node);
@@ -748,9 +748,14 @@ void JSGenericLowering::LowerJSForInPrepare(Node* node) {
       edge.UpdateTo(effect);
     } else if (NodeProperties::IsControlEdge(edge)) {
       Node* const use = edge.from();
-      DCHECK_EQ(IrOpcode::kIfSuccess, use->opcode());
-      use->ReplaceUses(control);
-      use->Kill();
+      if (use->opcode() == IrOpcode::kIfSuccess) {
+        use->ReplaceUses(control);
+        use->Kill();
+      } else if (use->opcode() == IrOpcode::kIfException) {
+        edge.UpdateTo(cache_type_true0);
+      } else {
+        UNREACHABLE();
+      }
     } else {
       Node* const use = edge.from();
       DCHECK(NodeProperties::IsValueEdge(edge));
@@ -805,10 +810,19 @@ void JSGenericLowering::LowerJSStackCheck(Node* node) {
   Node* merge = graph()->NewNode(common()->Merge(2), if_true, if_false);
   Node* ephi = graph()->NewNode(common()->EffectPhi(2), etrue, efalse, merge);
 
-  // Relax controls of {node}, i.e. make it free floating.
-  NodeProperties::ReplaceWithValue(node, node, ephi, merge);
+  // Wire the new diamond into the graph, {node} can still throw.
+  NodeProperties::ReplaceUses(node, node, ephi, node, node);
   NodeProperties::ReplaceEffectInput(ephi, efalse, 1);
 
+  // TODO(mstarzinger): This iteration cuts out the IfSuccess projection from
+  // the node and places it inside the diamond. Come up with a helper method!
+  for (Node* use : node->uses()) {
+    if (use->opcode() == IrOpcode::kIfSuccess) {
+      use->ReplaceUses(merge);
+      merge->ReplaceInput(1, use);
+    }
+  }
+
   // Turn the stack check into a runtime call.
   ReplaceWithRuntimeCall(node, Runtime::kStackGuard);
 }

src/compiler/js-typed-lowering.cc

@@ -1348,8 +1348,14 @@ Reduction JSTypedLowering::ReduceJSForInPrepare(Node* node) {
       Revisit(use);
     } else {
       if (NodeProperties::IsControlEdge(edge)) {
-        DCHECK_EQ(IrOpcode::kIfSuccess, use->opcode());
-        Replace(use, control);
+        if (use->opcode() == IrOpcode::kIfSuccess) {
+          Replace(use, control);
+        } else if (use->opcode() == IrOpcode::kIfException) {
+          edge.UpdateTo(cache_type_true0);
+          continue;
+        } else {
+          UNREACHABLE();
+        }
       } else {
         DCHECK(NodeProperties::IsValueEdge(edge));
         DCHECK_EQ(IrOpcode::kProjection, use->opcode());

src/compiler/node-properties.cc

@@ -169,26 +169,25 @@ void NodeProperties::MergeControlToEnd(Graph* graph,
 
 
 // static
-void NodeProperties::ReplaceWithValue(Node* node, Node* value, Node* effect,
-                                      Node* control) {
-  if (!effect && node->op()->EffectInputCount() > 0) {
-    effect = NodeProperties::GetEffectInput(node);
-  }
-  if (control == nullptr && node->op()->ControlInputCount() > 0) {
-    control = NodeProperties::GetControlInput(node);
-  }
-
+void NodeProperties::ReplaceUses(Node* node, Node* value, Node* effect,
+                                 Node* success, Node* exception) {
   // Requires distinguishing between value, effect and control edges.
   for (Edge edge : node->use_edges()) {
     if (IsControlEdge(edge)) {
-      DCHECK_EQ(IrOpcode::kIfSuccess, edge.from()->opcode());
-      DCHECK_NOT_NULL(control);
-      edge.from()->ReplaceUses(control);
-      edge.UpdateTo(NULL);
+      if (edge.from()->opcode() == IrOpcode::kIfSuccess) {
+        DCHECK_NOT_NULL(success);
+        edge.UpdateTo(success);
+      } else if (edge.from()->opcode() == IrOpcode::kIfException) {
+        DCHECK_NOT_NULL(exception);
+        edge.UpdateTo(exception);
+      } else {
+        UNREACHABLE();
+      }
     } else if (IsEffectEdge(edge)) {
       DCHECK_NOT_NULL(effect);
       edge.UpdateTo(effect);
     } else {
+      DCHECK_NOT_NULL(value);
       edge.UpdateTo(value);
     }
   }

src/compiler/node-properties.h

@@ -88,11 +88,11 @@ class NodeProperties final {
   static void MergeControlToEnd(Graph* graph, CommonOperatorBuilder* common,
                                 Node* node);
 
-  // Replace value uses of {node} with {value} and effect uses of {node} with
-  // {effect}. If {effect == NULL}, then use the effect input to {node}. All
-  // control uses will be relaxed assuming {node} cannot throw.
-  static void ReplaceWithValue(Node* node, Node* value, Node* effect = nullptr,
-                               Node* control = nullptr);
+  // Replace all uses of {node} with the given replacement nodes. All occurring
+  // use kinds need to be replaced, {NULL} is only valid if a use kind is
+  // guaranteed not to exist.
+  static void ReplaceUses(Node* node, Node* value, Node* effect = nullptr,
+                          Node* success = nullptr, Node* exception = nullptr);
 
   // ---------------------------------------------------------------------------
   // Miscellaneous utilities.

src/compiler/simplified-lowering.cc

@@ -1270,7 +1270,7 @@ void SimplifiedLowering::DoLoadBuffer(Node* node, MachineType output_type,
     Node* ephi = graph()->NewNode(common()->EffectPhi(2), etrue, efalse, merge);
 
     // Replace effect uses of {node} with the {ephi}.
-    NodeProperties::ReplaceWithValue(node, node, ephi);
+    NodeProperties::ReplaceUses(node, node, ephi);
 
     // Turn the {node} into a Phi.
     node->set_op(common()->Phi(output_type, 2));

test/mjsunit/mjsunit.status

@@ -118,6 +118,7 @@
   'regress/regress-crbug-107996': [PASS, NO_VARIANTS],
   'regress/regress-crbug-171715': [PASS, NO_VARIANTS],
   'regress/regress-crbug-222893': [PASS, NO_VARIANTS],
+  'regress/regress-crbug-323936': [PASS, NO_VARIANTS],
   'regress/regress-crbug-491943': [PASS, NO_VARIANTS],
   'regress/regress-325676': [PASS, NO_VARIANTS],
   'debug-evaluate-closure': [PASS, NO_VARIANTS],

test/unittests/compiler/node-properties-unittest.cc

@@ -10,70 +10,54 @@
 using testing::AnyOf;
 using testing::ElementsAre;
 using testing::IsNull;
-using testing::UnorderedElementsAre;
 
 namespace v8 {
 namespace internal {
 namespace compiler {
 
-typedef TestWithZone NodePropertiesTest;
-
+class NodePropertiesTest : public TestWithZone {
+ public:
+  Node* NewMockNode(const Operator* op, int input_count, Node** inputs) {
+    return Node::New(zone(), 0, op, input_count, inputs, false);
+  }
+};
 
 namespace {
 
 const Operator kMockOperator(IrOpcode::kDead, Operator::kNoProperties,
-                             "MockOperator", 0, 0, 0, 1, 0, 0);
-const Operator kMockOpEffect(IrOpcode::kDead, Operator::kNoProperties,
-                             "MockOpEffect", 0, 1, 0, 1, 1, 0);
-const Operator kMockOpControl(IrOpcode::kDead, Operator::kNoProperties,
-                              "MockOpControl", 0, 0, 1, 1, 0, 1);
+                             "MockOperator", 0, 0, 0, 1, 1, 2);
 const Operator kMockCallOperator(IrOpcode::kCall, Operator::kNoProperties,
                                  "MockCallOperator", 0, 0, 0, 0, 0, 2);
 
 }  // namespace
 
 
-TEST_F(NodePropertiesTest, ReplaceWithValue_ValueUse) {
+TEST_F(NodePropertiesTest, ReplaceUses) {
   CommonOperatorBuilder common(zone());
-  Node* node = Node::New(zone(), 0, &kMockOperator, 0, nullptr, false);
-  Node* use_value = Node::New(zone(), 0, common.Return(), 1, &node, false);
-  Node* replacement = Node::New(zone(), 0, &kMockOperator, 0, nullptr, false);
-  NodeProperties::ReplaceWithValue(node, replacement);
-  EXPECT_EQ(replacement, use_value->InputAt(0));
-  EXPECT_EQ(0, node->UseCount());
-  EXPECT_EQ(1, replacement->UseCount());
-  EXPECT_THAT(replacement->uses(), ElementsAre(use_value));
-}
-
-
-TEST_F(NodePropertiesTest, ReplaceWithValue_EffectUse) {
-  CommonOperatorBuilder common(zone());
-  Node* start = Node::New(zone(), 0, common.Start(1), 0, nullptr, false);
-  Node* node = Node::New(zone(), 0, &kMockOpEffect, 1, &start, false);
-  Node* use_effect = Node::New(zone(), 0, common.EffectPhi(1), 1, &node, false);
-  Node* replacement = Node::New(zone(), 0, &kMockOperator, 0, nullptr, false);
-  NodeProperties::ReplaceWithValue(node, replacement);
-  EXPECT_EQ(start, use_effect->InputAt(0));
-  EXPECT_EQ(0, node->UseCount());
-  EXPECT_EQ(2, start->UseCount());
-  EXPECT_EQ(0, replacement->UseCount());
-  EXPECT_THAT(start->uses(), UnorderedElementsAre(use_effect, node));
-}
-
-
-TEST_F(NodePropertiesTest, ReplaceWithValue_ControlUse) {
-  CommonOperatorBuilder common(zone());
-  Node* start = Node::New(zone(), 0, common.Start(1), 0, nullptr, false);
-  Node* node = Node::New(zone(), 0, &kMockOpControl, 1, &start, false);
-  Node* success = Node::New(zone(), 0, common.IfSuccess(), 1, &node, false);
-  Node* use_control = Node::New(zone(), 0, common.Merge(1), 1, &success, false);
-  Node* replacement = Node::New(zone(), 0, &kMockOperator, 0, nullptr, false);
-  NodeProperties::ReplaceWithValue(node, replacement);
-  EXPECT_EQ(start, use_control->InputAt(0));
+  IfExceptionHint kNoHint = IfExceptionHint::kLocallyCaught;
+  Node* node = NewMockNode(&kMockOperator, 0, nullptr);
+  Node* use_value = NewMockNode(common.Return(), 1, &node);
+  Node* use_effect = NewMockNode(common.EffectPhi(1), 1, &node);
+  Node* use_success = NewMockNode(common.IfSuccess(), 1, &node);
+  Node* use_exception = NewMockNode(common.IfException(kNoHint), 1, &node);
+  Node* r_value = NewMockNode(&kMockOperator, 0, nullptr);
+  Node* r_effect = NewMockNode(&kMockOperator, 0, nullptr);
+  Node* r_success = NewMockNode(&kMockOperator, 0, nullptr);
+  Node* r_exception = NewMockNode(&kMockOperator, 0, nullptr);
+  NodeProperties::ReplaceUses(node, r_value, r_effect, r_success, r_exception);
+  EXPECT_EQ(r_value, use_value->InputAt(0));
+  EXPECT_EQ(r_effect, use_effect->InputAt(0));
+  EXPECT_EQ(r_success, use_success->InputAt(0));
+  EXPECT_EQ(r_exception, use_exception->InputAt(0));
   EXPECT_EQ(0, node->UseCount());
-  EXPECT_EQ(2, start->UseCount());
-  EXPECT_EQ(0, replacement->UseCount());
-  EXPECT_THAT(start->uses(), UnorderedElementsAre(use_control, node));
+  EXPECT_EQ(1, r_value->UseCount());
+  EXPECT_EQ(1, r_effect->UseCount());
+  EXPECT_EQ(1, r_success->UseCount());
+  EXPECT_EQ(1, r_exception->UseCount());
+  EXPECT_THAT(r_value->uses(), ElementsAre(use_value));
+  EXPECT_THAT(r_effect->uses(), ElementsAre(use_effect));
+  EXPECT_THAT(r_success->uses(), ElementsAre(use_success));
+  EXPECT_THAT(r_exception->uses(), ElementsAre(use_exception));
 }