[turbofan] Remove the EmptyFrameState caching on JSGraph.

Caching nodes with mutable inputs is a bad idea and already blew up
twice now, so in order to avoid further breakage, let's kill the
EmptyFrameState caching on JSGraph completely and only cache the empty
state values there.

We can remove the hacking from JSTypedLowering completely once we have
the PlainPrimitiveToNumber in action.

R=jarin@chromium.org

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

src/compiler/js-graph.cc

@@ -218,20 +218,6 @@ Node* JSGraph::ExternalConstant(Runtime::FunctionId function_id) {
   return ExternalConstant(ExternalReference(function_id, isolate()));
 }
 
-
-Node* JSGraph::EmptyFrameState() {
-  Node* empty_frame_state = cached_nodes_[kEmptyFrameState];
-  if (!empty_frame_state || empty_frame_state->IsDead()) {
-    empty_frame_state = graph()->NewNode(
-        common()->FrameState(BailoutId::None(),
-                             OutputFrameStateCombine::Ignore(), nullptr),
-        EmptyStateValues(), EmptyStateValues(), EmptyStateValues(),
-        NoContextConstant(), UndefinedConstant(), graph()->start());
-    cached_nodes_[kEmptyFrameState] = empty_frame_state;
-  }
-  return empty_frame_state;
-}
-
 Node* JSGraph::EmptyStateValues() {
   return CACHED(kEmptyStateValues, graph()->NewNode(common()->StateValues(0)));
 }

src/compiler/js-graph.h

@@ -123,10 +123,6 @@ class JSGraph : public ZoneObject {
   // stubs and runtime functions that do not require a context.
   Node* NoContextConstant() { return ZeroConstant(); }
 
-  // Creates an empty frame states for cases where we know that a function
-  // cannot deopt.
-  Node* EmptyFrameState();
-
   // Creates an empty StateValues node, used when we don't have any concrete
   // values for a certain part of the frame state.
   Node* EmptyStateValues();
@@ -162,7 +158,6 @@ class JSGraph : public ZoneObject {
     kZeroConstant,
     kOneConstant,
     kNaNConstant,
-    kEmptyFrameState,
     kEmptyStateValues,
     kDead,
     kNumCachedNodes  // Must remain last.

src/compiler/js-typed-lowering.cc

@@ -224,9 +224,10 @@ class JSBinopReduction final {
     if (NodeProperties::GetType(node)->Is(Type::NumberOrUndefined())) {
       return node;
     }
+    // TODO(bmeurer): Introduce PlainPrimitiveToNumber here.
     return graph()->NewNode(
         javascript()->ToNumber(), node, jsgraph()->NoContextConstant(),
-        jsgraph()->EmptyFrameState(), graph()->start(), graph()->start());
+        lowering_->EmptyFrameState(), graph()->start(), graph()->start());
   }
 
   Node* ConvertSingleInputToNumber(Node* node, Node* frame_state) {
@@ -791,8 +792,7 @@ Reduction JSTypedLowering::ReduceJSToNumber(Node* node) {
       NodeProperties::ReplaceControlInput(node, graph()->start());
       NodeProperties::ReplaceEffectInput(node, graph()->start());
       DCHECK_EQ(1, OperatorProperties::GetFrameStateInputCount(node->op()));
-      NodeProperties::ReplaceFrameStateInput(node, 0,
-                                             jsgraph()->EmptyFrameState());
+      NodeProperties::ReplaceFrameStateInput(node, 0, EmptyFrameState());
       return Changed(node);
     }
   }
@@ -1860,6 +1860,14 @@ Node* JSTypedLowering::Word32Shl(Node* const lhs, int32_t const rhs) {
                           jsgraph()->Int32Constant(rhs));
 }
 
+Node* JSTypedLowering::EmptyFrameState() {
+  return graph()->NewNode(
+      common()->FrameState(BailoutId::None(), OutputFrameStateCombine::Ignore(),
+                           nullptr),
+      jsgraph()->EmptyStateValues(), jsgraph()->EmptyStateValues(),
+      jsgraph()->EmptyStateValues(), jsgraph()->NoContextConstant(),
+      jsgraph()->UndefinedConstant(), graph()->start());
+}
 
 Factory* JSTypedLowering::factory() const { return jsgraph()->factory(); }
 

src/compiler/js-typed-lowering.h

@@ -87,6 +87,8 @@ class JSTypedLowering final : public AdvancedReducer {
 
   Node* Word32Shl(Node* const lhs, int32_t const rhs);
 
+  Node* EmptyFrameState();
+
   Factory* factory() const;
   Graph* graph() const;
   JSGraph* jsgraph() const { return jsgraph_; }

test/cctest/compiler/test-js-context-specialization.cc

@@ -186,81 +186,6 @@ TEST(ReduceJSStoreContext) {
 }
 
 
-// TODO(titzer): factor out common code with effects checking in typed lowering.
-static void CheckEffectInput(Node* effect, Node* use) {
-  CHECK_EQ(effect, NodeProperties::GetEffectInput(use));
-}
-
-
-TEST(SpecializeToContext) {
-  ContextSpecializationTester t;
-
-  Node* start = t.graph()->NewNode(t.common()->Start(0));
-  t.graph()->SetStart(start);
-
-  // Make a context and initialize it a bit for this test.
-  Handle<Context> native = t.factory()->NewNativeContext();
-  Handle<Object> expected = t.factory()->InternalizeUtf8String("gboy!");
-  const int slot = Context::NATIVE_CONTEXT_INDEX;
-  native->set(slot, *expected);
-
-  Node* const_context = t.jsgraph()->Constant(native);
-  Node* param_context = t.graph()->NewNode(t.common()->Parameter(0), start);
-
-  {
-    // Check that specialization replaces values and forwards effects
-    // correctly, and folds values from constant and non-constant contexts
-    Node* effect_in = start;
-    Node* load = t.graph()->NewNode(t.javascript()->LoadContext(0, slot, true),
-                                    const_context, const_context, effect_in);
-
-
-    Node* value_use =
-        t.graph()->NewNode(t.simplified()->ChangeTaggedToInt32(), load);
-    Node* other_load =
-        t.graph()->NewNode(t.javascript()->LoadContext(0, slot, true),
-                           param_context, param_context, load);
-    Node* effect_use = other_load;
-    Node* other_use =
-        t.graph()->NewNode(t.simplified()->ChangeTaggedToInt32(), other_load);
-
-    Node* add = t.graph()->NewNode(
-        t.javascript()->Add(BinaryOperationHints::Any()), value_use, other_use,
-        param_context, t.jsgraph()->EmptyFrameState(),
-        t.jsgraph()->EmptyFrameState(), other_load, start);
-
-    Node* ret =
-        t.graph()->NewNode(t.common()->Return(), add, effect_use, start);
-    Node* end = t.graph()->NewNode(t.common()->End(1), ret);
-    USE(end);
-    t.graph()->SetEnd(end);
-
-    // Double check the above graph is what we expect, or the test is broken.
-    CheckEffectInput(effect_in, load);
-    CheckEffectInput(load, effect_use);
-
-    // Perform the reduction on the entire graph.
-    GraphReducer graph_reducer(t.main_zone(), t.graph());
-    JSContextSpecialization spec(&graph_reducer, t.jsgraph(),
-                                 MaybeHandle<Context>());
-    graph_reducer.AddReducer(&spec);
-    graph_reducer.ReduceGraph();
-
-    // Effects should have been forwarded (not replaced with a value).
-    CheckEffectInput(effect_in, effect_use);
-
-    // Use of {other_load} should not have been replaced.
-    CHECK_EQ(other_load, other_use->InputAt(0));
-
-    Node* replacement = value_use->InputAt(0);
-    HeapObjectMatcher match(replacement);
-    CHECK(match.HasValue());
-    CHECK_EQ(*expected, *match.Value());
-  }
-  // TODO(titzer): clean up above test and test more complicated effects.
-}
-
-
 TEST(SpecializeJSFunction_ToConstant1) {
   FunctionTester T(
       "(function() { var x = 1; function inc(a)"

test/unittests/compiler/js-intrinsic-lowering-unittest.cc

@@ -43,13 +43,6 @@ class JSIntrinsicLoweringTest : public GraphTest {
     return reducer.Reduce(node);
   }
 
-  Node* EmptyFrameState() {
-    MachineOperatorBuilder machine(zone());
-    JSGraph jsgraph(isolate(), graph(), common(), javascript(), nullptr,
-                    &machine);
-    return jsgraph.EmptyFrameState();
-  }
-
   JSOperatorBuilder* javascript() { return &javascript_; }
 
  private: