[turbofan] Check node input/use consistency for changed operators and new nodes.

Verifies consistency of node inputs and uses:
- node inputs should agree with the input count computed from the node's operator.
- effect inputs should have effect outputs (or be a sentinel).
- control inputs should have control outputs (or be a sentinel).
- frame state inputs should be frame states (or be a sentinel).
- if the node has control uses, it should produce control.
- if the node has effect uses, it should produce effect.
- if the node has frame state uses, it must be a frame state.

I also removed some tests, either because they did not seem to be useful (scheduler) or they tested dead functionality (diamond effect phi).

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

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

src/compiler/diamond.h

@@ -52,10 +52,6 @@ struct Diamond {
   Node* Phi(MachineType machine_type, Node* tv, Node* fv) {
     return graph->NewNode(common->Phi(machine_type, 2), tv, fv, merge);
   }
-
-  Node* EffectPhi(Node* tv, Node* fv) {
-    return graph->NewNode(common->EffectPhi(2), tv, fv, merge);
-  }
 };
 
 }  // namespace compiler

src/compiler/graph.cc

@@ -8,7 +8,7 @@
 
 #include "src/base/bits.h"
 #include "src/compiler/node.h"
-#include "src/compiler/operator-properties.h"
+#include "src/compiler/node-properties.h"
 
 namespace v8 {
 namespace internal {
@@ -44,8 +44,11 @@ void Graph::RemoveDecorator(GraphDecorator* decorator) {
 
 Node* Graph::NewNode(const Operator* op, int input_count, Node** inputs,
                      bool incomplete) {
-  DCHECK_EQ(OperatorProperties::GetTotalInputCount(op), input_count);
-  return NewNodeUnchecked(op, input_count, inputs, incomplete);
+  Node* node = NewNodeUnchecked(op, input_count, inputs, incomplete);
+#ifdef DEBUG
+  NodeProperties::Verify(node);
+#endif  // DEBUG
+  return node;
 }
 
 

src/compiler/js-intrinsic-lowering.cc

@@ -283,13 +283,13 @@ Reduction JSIntrinsicLowering::ReduceSeqStringGetChar(
     Node* node, String::Encoding encoding) {
   Node* effect = NodeProperties::GetEffectInput(node);
   Node* control = NodeProperties::GetControlInput(node);
+  RelaxControls(node);
   node->ReplaceInput(2, effect);
   node->ReplaceInput(3, control);
   node->TrimInputCount(4);
   NodeProperties::ChangeOp(
       node,
       simplified()->LoadElement(AccessBuilder::ForSeqStringChar(encoding)));
-  RelaxControls(node);
   return Changed(node);
 }
 
@@ -302,6 +302,8 @@ Reduction JSIntrinsicLowering::ReduceSeqStringSetChar(
   Node* string = NodeProperties::GetValueInput(node, 2);
   Node* effect = NodeProperties::GetEffectInput(node);
   Node* control = NodeProperties::GetControlInput(node);
+  ReplaceWithValue(node, string, node);
+  NodeProperties::RemoveType(node);
   node->ReplaceInput(0, string);
   node->ReplaceInput(1, index);
   node->ReplaceInput(2, chr);
@@ -311,8 +313,6 @@ Reduction JSIntrinsicLowering::ReduceSeqStringSetChar(
   NodeProperties::ChangeOp(
       node,
       simplified()->StoreElement(AccessBuilder::ForSeqStringChar(encoding)));
-  NodeProperties::RemoveType(node);
-  ReplaceWithValue(node, string, node);
   return Changed(node);
 }
 
@@ -552,36 +552,36 @@ Reduction JSIntrinsicLowering::ReduceCallFunction(Node* node) {
 
 Reduction JSIntrinsicLowering::Change(Node* node, const Operator* op, Node* a,
                                       Node* b) {
+  RelaxControls(node);
   node->ReplaceInput(0, a);
   node->ReplaceInput(1, b);
   node->TrimInputCount(2);
   NodeProperties::ChangeOp(node, op);
-  RelaxControls(node);
   return Changed(node);
 }
 
 
 Reduction JSIntrinsicLowering::Change(Node* node, const Operator* op, Node* a,
                                       Node* b, Node* c) {
+  RelaxControls(node);
   node->ReplaceInput(0, a);
   node->ReplaceInput(1, b);
   node->ReplaceInput(2, c);
   node->TrimInputCount(3);
   NodeProperties::ChangeOp(node, op);
-  RelaxControls(node);
   return Changed(node);
 }
 
 
 Reduction JSIntrinsicLowering::Change(Node* node, const Operator* op, Node* a,
                                       Node* b, Node* c, Node* d) {
+  RelaxControls(node);
   node->ReplaceInput(0, a);
   node->ReplaceInput(1, b);
   node->ReplaceInput(2, c);
   node->ReplaceInput(3, d);
   node->TrimInputCount(4);
   NodeProperties::ChangeOp(node, op);
-  RelaxControls(node);
   return Changed(node);
 }
 

src/compiler/js-typed-lowering.cc

@@ -644,10 +644,10 @@ Reduction JSTypedLowering::ReduceJSUnaryNot(Node* node) {
     // chain) because we assume String::length to be immutable.
     Node* length = graph()->NewNode(simplified()->LoadField(access), input,
                                     graph()->start(), graph()->start());
+    ReplaceWithValue(node, node, length);
     node->ReplaceInput(0, length);
     node->ReplaceInput(1, jsgraph()->ZeroConstant());
     NodeProperties::ChangeOp(node, simplified()->NumberEqual());
-    ReplaceWithValue(node, node, length);
     return Changed(node);
   }
   return NoChange();
@@ -905,6 +905,7 @@ Reduction JSTypedLowering::ReduceJSStoreProperty(Node* node) {
         }
         // Check if we can avoid the bounds check.
         if (key_type->Min() >= 0 && key_type->Max() < array->length_value()) {
+          RelaxControls(node);
           node->ReplaceInput(0, buffer);
           DCHECK_EQ(key, node->InputAt(1));
           node->ReplaceInput(2, value);
@@ -915,12 +916,12 @@ Reduction JSTypedLowering::ReduceJSStoreProperty(Node* node) {
               node,
               simplified()->StoreElement(
                   AccessBuilder::ForTypedArrayElement(array->type(), true)));
-          RelaxControls(node);
           return Changed(node);
         }
         // Compute byte offset.
         Node* offset = Word32Shl(key, static_cast<int>(k));
         // Turn into a StoreBuffer operation.
+        RelaxControls(node);
         node->ReplaceInput(0, buffer);
         node->ReplaceInput(1, offset);
         node->ReplaceInput(2, length);
@@ -929,7 +930,6 @@ Reduction JSTypedLowering::ReduceJSStoreProperty(Node* node) {
         node->ReplaceInput(5, control);
         node->TrimInputCount(6);
         NodeProperties::ChangeOp(node, simplified()->StoreBuffer(access));
-        RelaxControls(node);
         return Changed(node);
       }
     }

src/compiler/node-properties.cc

@@ -197,8 +197,11 @@ void NodeProperties::ReplaceUses(Node* node, Node* value, Node* effect,
 
 // static
 void NodeProperties::ChangeOp(Node* node, const Operator* new_op) {
-  DCHECK_EQ(OperatorProperties::GetTotalInputCount(new_op), node->InputCount());
   node->set_op(new_op);
+
+#ifdef DEBUG
+  Verify(node);
+#endif  // DEBUG
 }
 
 
@@ -267,6 +270,51 @@ void NodeProperties::CollectControlProjections(Node* node, Node** projections,
 }
 
 
+// static
+void NodeProperties::Verify(Node* node) {
+  CHECK_EQ(OperatorProperties::GetTotalInputCount(node->op()),
+           node->InputCount());
+  // If this node has no effect or no control outputs,
+  // we check that no its uses are effect or control inputs.
+  bool check_no_control = node->op()->ControlOutputCount() == 0;
+  bool check_no_effect = node->op()->EffectOutputCount() == 0;
+  bool check_no_frame_state = node->opcode() != IrOpcode::kFrameState;
+  if (check_no_effect || check_no_control) {
+    for (Edge edge : node->use_edges()) {
+      Node* const user = edge.from();
+      CHECK(!user->IsDead());
+      if (NodeProperties::IsControlEdge(edge)) {
+        CHECK(!check_no_control);
+      } else if (NodeProperties::IsEffectEdge(edge)) {
+        CHECK(!check_no_effect);
+      } else if (NodeProperties::IsFrameStateEdge(edge)) {
+        CHECK(!check_no_frame_state);
+      }
+    }
+  }
+  // Frame state inputs should be frame states (or sentinels).
+  for (int i = 0; i < OperatorProperties::GetFrameStateInputCount(node->op());
+       i++) {
+    Node* input = NodeProperties::GetFrameStateInput(node, i);
+    CHECK(input->opcode() == IrOpcode::kFrameState ||
+          input->opcode() == IrOpcode::kStart ||
+          input->opcode() == IrOpcode::kDead);
+  }
+  // Effect inputs should be effect-producing nodes (or sentinels).
+  for (int i = 0; i < node->op()->EffectInputCount(); i++) {
+    Node* input = NodeProperties::GetEffectInput(node, i);
+    CHECK(input->op()->EffectOutputCount() > 0 ||
+          input->opcode() == IrOpcode::kDead);
+  }
+  // Control inputs should be control-producing nodes (or sentinels).
+  for (int i = 0; i < node->op()->ControlInputCount(); i++) {
+    Node* input = NodeProperties::GetControlInput(node, i);
+    CHECK(input->op()->ControlOutputCount() > 0 ||
+          input->opcode() == IrOpcode::kDead);
+  }
+}
+
+
 // static
 bool NodeProperties::AllValueInputsAreTyped(Node* node) {
   int input_count = node->op()->ValueInputCount();

src/compiler/node-properties.h

@@ -110,6 +110,16 @@ class NodeProperties final {
   //  - Switch: [ IfValue, ..., IfDefault ]
   static void CollectControlProjections(Node* node, Node** proj, size_t count);
 
+  // Verifies consistency of node inputs and uses:
+  // - node inputs should agree with the input count computed from
+  //   the node's operator.
+  // - effect inputs should have effect outputs.
+  // - control inputs should have control outputs.
+  // - frame state inputs should be frame states.
+  // - if the node has control uses, it should produce control.
+  // - if the node has effect uses, it should produce effect.
+  // - if the node has frame state uses, it must be a frame state.
+  static void Verify(Node* node);
 
   // ---------------------------------------------------------------------------
   // Type.

src/compiler/select-lowering.cc

@@ -52,10 +52,10 @@ Reduction SelectLowering::Reduce(Node* node) {
   }
 
   // Create a Phi hanging off the previously determined merge.
-  NodeProperties::ChangeOp(node, common()->Phi(p.type(), 2));
   node->ReplaceInput(0, vthen);
   node->ReplaceInput(1, velse);
   node->ReplaceInput(2, merge);
+  NodeProperties::ChangeOp(node, common()->Phi(p.type(), 2));
   return Changed(node);
 }
 

test/cctest/compiler/test-loop-analysis.cc

@@ -27,7 +27,7 @@ static Operator kIntAdd(IrOpcode::kInt32Add, Operator::kPure, "Int32Add", 2, 0,
                         0, 1, 0, 0);
 static Operator kIntLt(IrOpcode::kInt32LessThan, Operator::kPure,
                        "Int32LessThan", 2, 0, 0, 1, 0, 0);
-static Operator kStore(IrOpcode::kStore, Operator::kNoProperties, "Store", 0, 2,
+static Operator kStore(IrOpcode::kStore, Operator::kNoProperties, "Store", 1, 1,
                        1, 0, 1, 0);
 
 static const int kNumLeafs = 4;
@@ -234,8 +234,7 @@ struct StoreLoop {
   Node* store;
 
   explicit StoreLoop(While& w)
-      : base(w.t.jsgraph.Int32Constant(12)),
-        val(w.t.jsgraph.Int32Constant(13)) {
+      : base(w.t.graph.start()), val(w.t.jsgraph.Int32Constant(13)) {
     Build(w);
   }
 
@@ -243,7 +242,7 @@ struct StoreLoop {
 
   void Build(While& w) {
     phi = w.t.graph.NewNode(w.t.op(2, true), base, base, w.loop);
-    store = w.t.graph.NewNode(&kStore, phi, val, w.loop);
+    store = w.t.graph.NewNode(&kStore, val, phi, w.loop);
     phi->ReplaceInput(1, store);
   }
 };
@@ -489,7 +488,7 @@ TEST(LaNestedLoop1x) {
   p2a->ReplaceInput(1, p2b);
   p2b->ReplaceInput(1, p2a);
 
-  t.Return(t.p0, p1a, w1.exit);
+  t.Return(t.p0, t.start, w1.exit);
 
   Node* chain[] = {w1.loop, w2.loop};
   t.CheckNestedLoops(chain, 2);

test/cctest/compiler/test-machine-operator-reducer.cc

@@ -715,7 +715,7 @@ TEST(ReduceLoadStore) {
   {
     Node* store = R.graph.NewNode(
         R.machine.Store(StoreRepresentation(kMachInt32, kNoWriteBarrier)), base,
-        index, load, load, load);
+        index, load, load, R.graph.start());
     MachineOperatorReducer reducer(&R.jsgraph);
     Reduction reduction = reducer.Reduce(store);
     CHECK(!reduction.Changed());  // stores should not be reduced.

test/cctest/compiler/test-osr.cc

@@ -93,22 +93,16 @@ class OsrDeconstructorTester : public HandleAndZoneScope {
     return graph.NewNode(common.Phi(kMachAnyTagged, count), count + 1, inputs);
   }
 
-  Node* NewLoop(bool is_osr, int num_backedges, Node* entry = NULL) {
-    CHECK_LT(num_backedges, 4);
-    CHECK_GE(num_backedges, 0);
-    int count = 1 + num_backedges;
-    if (entry == NULL) entry = osr_normal_entry;
-    Node* inputs[5] = {entry, self, self, self, self};
+  Node* NewLoop(bool is_osr, int num_backedges, Node* entry = nullptr) {
+    if (entry == nullptr) entry = osr_normal_entry;
+    Node* loop = graph.NewNode(common.Loop(1), entry);
     if (is_osr) {
-      count = 2 + num_backedges;
-      inputs[1] = osr_loop_entry;
+      loop->AppendInput(graph.zone(), osr_loop_entry);
     }
-
-    Node* loop = graph.NewNode(common.Loop(count), count, inputs);
-    for (int i = 0; i < loop->InputCount(); i++) {
-      if (loop->InputAt(i) == self) loop->ReplaceInput(i, loop);
+    for (int i = 0; i < num_backedges; i++) {
+      loop->AppendInput(graph.zone(), loop);
     }
-
+    NodeProperties::ChangeOp(loop, common.Loop(loop->InputCount()));
     return loop;
   }
 
@@ -497,8 +491,7 @@ TEST(Deconstruct_osr_nested3) {
   loop0.branch->ReplaceInput(0, loop0_cntr);
 
   // middle loop.
-  Node* loop1 = T.graph.NewNode(T.common.Loop(2), loop0.if_true, T.self);
-  loop1->ReplaceInput(0, loop0.if_true);
+  Node* loop1 = T.graph.NewNode(T.common.Loop(1), loop0.if_true);
   Node* loop1_phi = T.graph.NewNode(T.common.Phi(kMachAnyTagged, 2), loop0_cntr,
                                     loop0_cntr, loop1);
 
@@ -521,7 +514,8 @@ TEST(Deconstruct_osr_nested3) {
   Node* if_false = T.graph.NewNode(T.common.IfFalse(), branch);
 
   loop0.loop->ReplaceInput(1, if_true);
-  loop1->ReplaceInput(1, if_false);
+  loop1->AppendInput(T.graph.zone(), if_false);
+  NodeProperties::ChangeOp(loop1, T.common.Loop(2));
 
   Node* ret =
       T.graph.NewNode(T.common.Return(), loop0_cntr, T.start, loop0.exit);

test/unittests/compiler/control-equivalence-unittest.cc

@@ -70,6 +70,10 @@ class ControlEquivalenceTest : public GraphTest {
     return Store(graph()->NewNode(common()->IfFalse(), control));
   }
 
+  Node* Merge1(Node* control) {
+    return Store(graph()->NewNode(common()->Merge(1), control));
+  }
+
   Node* Merge2(Node* control1, Node* control2) {
     return Store(graph()->NewNode(common()->Merge(2), control1, control2));
   }
@@ -107,10 +111,10 @@ TEST_F(ControlEquivalenceTest, Empty1) {
 
 TEST_F(ControlEquivalenceTest, Empty2) {
   Node* start = graph()->start();
-  Node* end = End(start);
-  ComputeEquivalence(end);
+  Node* merge1 = Merge1(start);
+  ComputeEquivalence(merge1);
 
-  ASSERT_EQUIVALENCE(start, end);
+  ASSERT_EQUIVALENCE(start, merge1);
 }
 
 

test/unittests/compiler/diamond-unittest.cc

@@ -128,22 +128,6 @@ TEST_F(DiamondTest, DiamondPhis) {
 }
 
 
-TEST_F(DiamondTest, DiamondEffectPhis) {
-  Node* p0 = Parameter(0);
-  Node* p1 = Parameter(1);
-  Node* p2 = Parameter(2);
-  Diamond d(graph(), common(), p0);
-
-  Node* phi = d.EffectPhi(p1, p2);
-
-  EXPECT_THAT(d.branch, IsBranch(p0, graph()->start()));
-  EXPECT_THAT(d.if_true, IsIfTrue(d.branch));
-  EXPECT_THAT(d.if_false, IsIfFalse(d.branch));
-  EXPECT_THAT(d.merge, IsMerge(d.if_true, d.if_false));
-  EXPECT_THAT(phi, IsEffectPhi(p1, p2, d.merge));
-}
-
-
 TEST_F(DiamondTest, BranchHint) {
   Diamond dn(graph(), common(), Parameter(0));
   CHECK(BranchHint::kNone == BranchHintOf(dn.branch->op()));

test/unittests/compiler/loop-peeling-unittest.cc

@@ -468,7 +468,7 @@ TEST_F(LoopPeelingTest, TwoExitLoop_nope) {
 
 
 const Operator kMockCall(IrOpcode::kCall, Operator::kNoProperties, "MockCall",
-                         0, 0, 1, 1, 0, 2);
+                         0, 0, 1, 1, 1, 2);
 
 
 TEST_F(LoopPeelingTest, TwoExitLoopWithCall_nope) {

test/unittests/compiler/scheduler-unittest.cc

@@ -137,7 +137,7 @@ const Operator kHeapConstant(IrOpcode::kHeapConstant, Operator::kPure,
 const Operator kIntAdd(IrOpcode::kInt32Add, Operator::kPure, "Int32Add", 2, 0,
                        0, 1, 0, 0);
 const Operator kMockCall(IrOpcode::kCall, Operator::kNoProperties, "MockCall",
-                         0, 0, 1, 1, 0, 2);
+                         0, 0, 1, 1, 1, 2);
 const Operator kMockTailCall(IrOpcode::kTailCall, Operator::kNoProperties,
                              "MockTailCall", 1, 1, 1, 0, 0, 1);
 
@@ -649,31 +649,6 @@ TEST_F(SchedulerTest, BuildScheduleOneParameter) {
 }
 
 
-TEST_F(SchedulerTest, BuildScheduleIfSplit) {
-  graph()->SetStart(graph()->NewNode(common()->Start(5)));
-
-  Node* p1 = graph()->NewNode(common()->Parameter(0), graph()->start());
-  Node* p2 = graph()->NewNode(common()->Parameter(1), graph()->start());
-  Node* p3 = graph()->NewNode(common()->Parameter(2), graph()->start());
-  Node* p4 = graph()->NewNode(common()->Parameter(3), graph()->start());
-  Node* p5 = graph()->NewNode(common()->Parameter(4), graph()->start());
-  Node* cmp =
-      graph()->NewNode(js()->LessThanOrEqual(LanguageMode::SLOPPY), p1, p2, p3,
-                       p4, p5, graph()->start(), graph()->start());
-  Node* branch = graph()->NewNode(common()->Branch(), cmp, graph()->start());
-  Node* true_branch = graph()->NewNode(common()->IfTrue(), branch);
-  Node* false_branch = graph()->NewNode(common()->IfFalse(), branch);
-
-  Node* ret1 =
-      graph()->NewNode(common()->Return(), p4, graph()->start(), true_branch);
-  Node* ret2 =
-      graph()->NewNode(common()->Return(), p5, graph()->start(), false_branch);
-  graph()->SetEnd(graph()->NewNode(common()->End(2), ret1, ret2));
-
-  ComputeAndVerifySchedule(13);
-}
-
-
 namespace {
 
 Node* CreateDiamond(Graph* graph, CommonOperatorBuilder* common, Node* cond) {

test/unittests/compiler/value-numbering-reducer-unittest.cc

@@ -73,8 +73,7 @@ TEST_F(ValueNumberingReducerTest, OperatorEqualityNotIdentity) {
   static const size_t kMaxInputCount = 16;
   Node* inputs[kMaxInputCount];
   for (size_t i = 0; i < arraysize(inputs); ++i) {
-    Operator::Opcode opcode = static_cast<Operator::Opcode>(
-        std::numeric_limits<Operator::Opcode>::max() - i);
+    Operator::Opcode opcode = static_cast<Operator::Opcode>(kMaxInputCount + i);
     inputs[i] = graph()->NewNode(
         new (zone()) TestOperator(opcode, Operator::kIdempotent, 0, 1));
   }
@@ -99,8 +98,7 @@ TEST_F(ValueNumberingReducerTest, SubsequentReductionsYieldTheSameNode) {
   static const size_t kMaxInputCount = 16;
   Node* inputs[kMaxInputCount];
   for (size_t i = 0; i < arraysize(inputs); ++i) {
-    Operator::Opcode opcode = static_cast<Operator::Opcode>(
-        std::numeric_limits<Operator::Opcode>::max() - i);
+    Operator::Opcode opcode = static_cast<Operator::Opcode>(2 + i);
     inputs[i] = graph()->NewNode(
         new (zone()) TestOperator(opcode, Operator::kIdempotent, 0, 1));
   }