Revert of Reland "[Interpreter] Add CreateClosure to BytecodeGraphBuilder."...

Revert of Reland "[Interpreter] Add CreateClosure to BytecodeGraphBuilder." (patchset #1 id:1 of https://codereview.chromium.org/1475793003/ )

Reason for revert:
[Sheriff] Breaks cfi:
https://build.chromium.org/p/client.v8/builders/V8%20Linux64%20-%20cfi/builds/1209

Original issue's description:
> Reland "[Interpreter] Add CreateClosure to BytecodeGraphBuilder."
>
> Original issue's description:
> > [Interpreter] Add CreateClosure to BytecodeGraphBuilder.
> >
> > Adds code and tests to support CreateClosure bytecode when building
> > graphs.
> >
> > Committed: https://crrev.com/4cceb11b0929abcbc82bf0854554a9b66003335d
> > Cr-Commit-Position: refs/heads/master@{#32224}
>
> BUG=v8:4280
> LOG=N
>
> Committed: https://crrev.com/6a8db006e1f0a08a43446b62765bba39fdc6af10
> Cr-Commit-Position: refs/heads/master@{#32257}

TBR=bmeurer@chromium.org,oth@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=v8:4280

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

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

src/compiler/bytecode-graph-builder.cc

@@ -643,20 +643,7 @@ void BytecodeGraphBuilder::VisitPopContext(
 
 void BytecodeGraphBuilder::VisitCreateClosure(
     const interpreter::BytecodeArrayIterator& iterator) {
-  Handle<SharedFunctionInfo> shared_info =
-      Handle<SharedFunctionInfo>::cast(iterator.GetConstantForIndexOperand(0));
-  PretenureFlag tenured =
-      iterator.GetImmediateOperand(1) ? TENURED : NOT_TENURED;
-  const Operator* op = javascript()->CreateClosure(shared_info, tenured);
-  Node* closure = NewNode(op);
-  AddEmptyFrameStateInputs(closure);
-  environment()->BindAccumulator(closure);
-}
-
-
-void BytecodeGraphBuilder::VisitCreateClosureWide(
-    const interpreter::BytecodeArrayIterator& iterator) {
-  VisitCreateClosure(iterator);
+  UNIMPLEMENTED();
 }
 
 

src/interpreter/bytecode-array-builder.cc

@@ -444,18 +444,9 @@ BytecodeArrayBuilder& BytecodeArrayBuilder::StoreKeyedProperty(
 
 
 BytecodeArrayBuilder& BytecodeArrayBuilder::CreateClosure(
-    Handle<SharedFunctionInfo> shared_info, PretenureFlag tenured) {
-  size_t entry = GetConstantPoolEntry(shared_info);
+    PretenureFlag tenured) {
   DCHECK(FitsInImm8Operand(tenured));
-  if (FitsInIdx8Operand(entry)) {
-    Output(Bytecode::kCreateClosure, static_cast<uint8_t>(entry),
-           static_cast<uint8_t>(tenured));
-  } else if (FitsInIdx16Operand(entry)) {
-    Output(Bytecode::kCreateClosureWide, static_cast<uint16_t>(entry),
-           static_cast<uint8_t>(tenured));
-  } else {
-    UNIMPLEMENTED();
-  }
+  Output(Bytecode::kCreateClosure, static_cast<uint8_t>(tenured));
   return *this;
 }
 

src/interpreter/bytecode-array-builder.h

@@ -116,9 +116,8 @@ class BytecodeArrayBuilder {
                                            int feedback_slot,
                                            LanguageMode language_mode);
 
-  // Create a new closure for the SharedFunctionInfo.
-  BytecodeArrayBuilder& CreateClosure(Handle<SharedFunctionInfo> shared_info,
-                                      PretenureFlag tenured);
+  // Create a new closure for the SharedFunctionInfo in the accumulator.
+  BytecodeArrayBuilder& CreateClosure(PretenureFlag tenured);
 
   // Create a new arguments object in the accumulator.
   BytecodeArrayBuilder& CreateArguments(CreateArgumentsType type);

src/interpreter/bytecode-generator.cc

@@ -971,8 +971,10 @@ void BytecodeGenerator::VisitFunctionLiteral(FunctionLiteral* expr) {
   Handle<SharedFunctionInfo> shared_info =
       Compiler::GetSharedFunctionInfo(expr, info()->script(), info());
   CHECK(!shared_info.is_null());  // TODO(rmcilroy): Set stack overflow?
-  builder()->CreateClosure(shared_info,
-                           expr->pretenure() ? TENURED : NOT_TENURED);
+
+  builder()
+      ->LoadLiteral(shared_info)
+      .CreateClosure(expr->pretenure() ? TENURED : NOT_TENURED);
   execution_result()->SetResultInAccumulator();
 }
 

src/interpreter/bytecodes.h

@@ -161,8 +161,7 @@ namespace interpreter {
   V(CreateObjectLiteral, OperandType::kIdx8, OperandType::kImm8)               \
                                                                                \
   /* Closure allocation */                                                     \
-  V(CreateClosure, OperandType::kIdx8, OperandType::kImm8)                     \
-  V(CreateClosureWide, OperandType::kIdx16, OperandType::kImm8)                \
+  V(CreateClosure, OperandType::kImm8)                                         \
                                                                                \
   /* Arguments allocation */                                                   \
   V(CreateMappedArguments, OperandType::kNone)                                 \

src/interpreter/interpreter.cc

@@ -1365,16 +1365,15 @@ void Interpreter::DoCreateObjectLiteral(
 }
 
 
-// CreateClosure <index> <tenured>
+// CreateClosure <tenured>
 //
-// Creates a new closure for SharedFunctionInfo at position |index| in the
-// constant pool and with the PretenureFlag <tenured>.
+// Creates a new closure for SharedFunctionInfo in the accumulator with the
+// PretenureFlag <tenured>.
 void Interpreter::DoCreateClosure(compiler::InterpreterAssembler* assembler) {
   // TODO(rmcilroy): Possibly call FastNewClosureStub when possible instead of
   // calling into the runtime.
-  Node* index = __ BytecodeOperandIdx(0);
-  Node* shared = __ LoadConstantPoolEntry(index);
-  Node* tenured_raw = __ BytecodeOperandImm(1);
+  Node* shared = __ GetAccumulator();
+  Node* tenured_raw = __ BytecodeOperandImm(0);
   Node* tenured = __ SmiTag(tenured_raw);
   Node* result =
       __ CallRuntime(Runtime::kInterpreterNewClosure, shared, tenured);
@@ -1383,16 +1382,6 @@ void Interpreter::DoCreateClosure(compiler::InterpreterAssembler* assembler) {
 }
 
 
-// CreateClosureWide <index> <tenured>
-//
-// Creates a new closure for SharedFunctionInfo at position |index| in the
-// constant pool and with the PretenureFlag <tenured>.
-void Interpreter::DoCreateClosureWide(
-    compiler::InterpreterAssembler* assembler) {
-  return DoCreateClosure(assembler);
-}
-
-
 // CreateMappedArguments
 //
 // Creates a new mapped arguments object.

test/cctest/compiler/test-run-bytecode-graph-builder.cc

@@ -575,45 +575,6 @@ TEST(BytecodeGraphBuilderCallNew) {
 }
 
 
-TEST(BytecodeGraphBuilderCreateClosure) {
-  HandleAndZoneScope scope;
-  Isolate* isolate = scope.main_isolate();
-  Zone* zone = scope.main_zone();
-  Factory* factory = isolate->factory();
-
-  ExpectedSnippet<0> snippets[] = {
-      {"function f() {\n"
-       "  function counter() { this.count = 20; }\n"
-       "  var c = new counter();\n"
-       "  return c.count;\n"
-       "}; f()",
-       {factory->NewNumberFromInt(20)}},
-      {"function f() {\n"
-       "  function counter(arg0) { this.count = 17; this.x = arg0; }\n"
-       "  var c = new counter(6);\n"
-       "  return c.count + c.x;\n"
-       "}; f()",
-       {factory->NewNumberFromInt(23)}},
-      {"function f() {\n"
-       "  function counter(arg0, arg1) {\n"
-       "    this.count = 17; this.x = arg0; this.y = arg1;\n"
-       "  }\n"
-       "  var c = new counter(3, 5);\n"
-       "  return c.count + c.x + c.y;\n"
-       "}; f()",
-       {factory->NewNumberFromInt(25)}},
-  };
-
-  size_t num_snippets = sizeof(snippets) / sizeof(snippets[0]);
-  for (size_t i = 0; i < num_snippets; i++) {
-    BytecodeGraphTester tester(isolate, zone, snippets[i].code_snippet);
-    auto callable = tester.GetCallable<>();
-    Handle<Object> return_value = callable().ToHandleChecked();
-    CHECK(return_value->SameValue(*snippets[i].return_value()));
-  }
-}
-
-
 TEST(BytecodeGraphBuilderCallRuntime) {
   HandleAndZoneScope scope;
   Isolate* isolate = scope.main_isolate();

test/unittests/compiler/bytecode-graph-builder-unittest.cc

@@ -40,12 +40,6 @@ class BytecodeGraphBuilderTest : public TestWithIsolateAndZone {
  public:
   BytecodeGraphBuilderTest() {}
 
-  std::pair<Graph*, Handle<SharedFunctionInfo>> GetCompletedGraphAndSharedInfo(
-      Handle<BytecodeArray> bytecode_array,
-      MaybeHandle<TypeFeedbackVector> feedback_vector =
-          MaybeHandle<TypeFeedbackVector>(),
-      LanguageMode language_mode = LanguageMode::SLOPPY);
-
   Graph* GetCompletedGraph(Handle<BytecodeArray> bytecode_array,
                            MaybeHandle<TypeFeedbackVector> feedback_vector =
                                MaybeHandle<TypeFeedbackVector>(),
@@ -69,8 +63,7 @@ class BytecodeGraphBuilderTest : public TestWithIsolateAndZone {
 };
 
 
-std::pair<Graph*, Handle<SharedFunctionInfo>>
-BytecodeGraphBuilderTest::GetCompletedGraphAndSharedInfo(
+Graph* BytecodeGraphBuilderTest::GetCompletedGraph(
     Handle<BytecodeArray> bytecode_array,
     MaybeHandle<TypeFeedbackVector> feedback_vector,
     LanguageMode language_mode) {
@@ -98,17 +91,7 @@ BytecodeGraphBuilderTest::GetCompletedGraphAndSharedInfo(
 
   BytecodeGraphBuilder graph_builder(zone(), &info, jsgraph);
   graph_builder.CreateGraph();
-  return std::make_pair(graph_builder.graph(), shared_info);
-}
-
-
-Graph* BytecodeGraphBuilderTest::GetCompletedGraph(
-    Handle<BytecodeArray> bytecode_array,
-    MaybeHandle<TypeFeedbackVector> feedback_vector,
-    LanguageMode language_mode) {
-  return GetCompletedGraphAndSharedInfo(bytecode_array, feedback_vector,
-                                        language_mode)
-      .first;
+  return graph;
 }
 
 
@@ -824,39 +807,6 @@ TEST_F(BytecodeGraphBuilderTest, New) {
   EXPECT_THAT(ret, IsReturn(call_construct, call_construct, IsIfSuccess(_)));
 }
 
-
-TEST_F(BytecodeGraphBuilderTest, CreateClosure) {
-  PretenureFlag kPretenureFlags[] = {NOT_TENURED, TENURED};
-  TRACED_FOREACH(PretenureFlag, pretenure_flag, kPretenureFlags) {
-    interpreter::BytecodeArrayBuilder inner_builder(isolate(), zone());
-    inner_builder.set_locals_count(0);
-    inner_builder.set_context_count(0);
-    inner_builder.set_parameter_count(3);
-    inner_builder.LoadAccumulatorWithRegister(inner_builder.Parameter(2))
-        .BinaryOperation(Token::Value::ADD, inner_builder.Parameter(1),
-                         Strength::WEAK)
-        .Return();
-
-    std::pair<Graph*, Handle<SharedFunctionInfo>> inner_graph_and_shared_info =
-        GetCompletedGraphAndSharedInfo(inner_builder.ToBytecodeArray());
-    Handle<SharedFunctionInfo> shared_info = inner_graph_and_shared_info.second;
-
-    interpreter::BytecodeArrayBuilder builder(isolate(), zone());
-    builder.set_locals_count(4);
-    builder.set_context_count(0);
-    builder.set_parameter_count(3);
-    builder.CreateClosure(shared_info, pretenure_flag).Return();
-
-    Graph* graph = GetCompletedGraph(builder.ToBytecodeArray());
-    Node* start = graph->start();
-    Node* ret = graph->end()->InputAt(0);
-
-    Matcher<Node*> create_closure =
-        IsCreateClosure(shared_info, pretenure_flag, start, start);
-    EXPECT_THAT(ret, IsReturn(create_closure, create_closure, start));
-  }
-}
-
 }  // namespace compiler
 }  // namespace internal
 }  // namespace v8

test/unittests/compiler/node-test-utils.cc

@@ -27,7 +27,6 @@ bool operator==(Handle<HeapObject> const& lhs, Handle<HeapObject> const& rhs) {
   return lhs.is_identical_to(rhs);
 }
 
-
 namespace compiler {
 
 namespace {
@@ -1859,56 +1858,6 @@ class IsJSCallMatcher final : public NodeMatcher {
   const Matcher<Node*> control_matcher_;
 };
 
-
-class IsCreateClosureMatcher final : public NodeMatcher {
- public:
-  IsCreateClosureMatcher(
-      const Matcher<Handle<SharedFunctionInfo>>& shared_info_matcher,
-      const Matcher<PretenureFlag>& pretenure_matcher,
-      const Matcher<Node*>& effect_matcher,
-      const Matcher<Node*>& control_matcher)
-      : NodeMatcher(IrOpcode::Value::kJSCreateClosure),
-        shared_info_matcher_(shared_info_matcher),
-        pretenure_matcher_(pretenure_matcher),
-        effect_matcher_(effect_matcher),
-        control_matcher_(control_matcher) {}
-
-  void DescribeTo(std::ostream* os) const final {
-    NodeMatcher::DescribeTo(os);
-    *os << " whose value (";
-    shared_info_matcher_.DescribeTo(os);
-    *os << ",";
-    pretenure_matcher_.DescribeTo(os);
-    *os << "), effect (";
-    effect_matcher_.DescribeTo(os);
-    *os << ") and control (";
-    control_matcher_.DescribeTo(os);
-    *os << ")";
-  }
-
-  bool MatchAndExplain(Node* node, MatchResultListener* listener) const final {
-    if (!NodeMatcher::MatchAndExplain(node, listener)) {
-      return false;
-    }
-    return (PrintMatchAndExplain(
-                OpParameter<const CreateClosureParameters>(node).shared_info(),
-                "value", shared_info_matcher_, listener) &&
-            PrintMatchAndExplain(
-                OpParameter<CreateClosureParameters>(node).pretenure(), "value",
-                pretenure_matcher_, listener) &&
-            PrintMatchAndExplain(NodeProperties::GetEffectInput(node), "effect",
-                                 effect_matcher_, listener) &&
-            PrintMatchAndExplain(NodeProperties::GetControlInput(node),
-                                 "control", control_matcher_, listener));
-  }
-
- private:
-  const Matcher<Handle<SharedFunctionInfo>> shared_info_matcher_;
-  const Matcher<PretenureFlag> pretenure_matcher_;
-  const Matcher<Node*> effect_matcher_;
-  const Matcher<Node*> control_matcher_;
-};
-
 }  // namespace
 
 
@@ -2586,15 +2535,6 @@ Matcher<Node*> IsJSCallRuntime(std::vector<Matcher<Node*>> value_matchers,
 }
 
 
-Matcher<Node*> IsCreateClosure(const Handle<SharedFunctionInfo> shared_info,
-                               PretenureFlag pretenure,
-                               const Matcher<Node*>& effect_matcher,
-                               const Matcher<Node*>& control_matcher) {
-  return MakeMatcher(new IsCreateClosureMatcher(
-      shared_info, pretenure, effect_matcher, control_matcher));
-}
-
-
 #define IS_BINOP_MATCHER(Name)                                            \
   Matcher<Node*> Is##Name(const Matcher<Node*>& lhs_matcher,              \
                           const Matcher<Node*>& rhs_matcher) {            \

test/unittests/compiler/node-test-utils.h

@@ -17,7 +17,6 @@ class ExternalReference;
 template <typename T>
 class Handle;
 class HeapObject;
-class SharedFunctionInfo;
 template <class>
 class TypeImpl;
 enum TypeofMode : int;
@@ -406,10 +405,6 @@ Matcher<Node*> IsJSDeleteProperty(const Matcher<Node*>& object_value_matcher,
                                   const Matcher<Node*>& key_matcher,
                                   const Matcher<Node*>& effect_matcher,
                                   const Matcher<Node*>& control_matcher);
-Matcher<Node*> IsCreateClosure(const Handle<SharedFunctionInfo> shared_info,
-                               PretenureFlag pretenure,
-                               const Matcher<Node*>& effect_matcher,
-                               const Matcher<Node*>& control_matcher);
 
 }  // namespace compiler
 }  // namespace internal

test/unittests/interpreter/bytecode-array-builder-unittest.cc

@@ -94,11 +94,7 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
       .StoreKeyedProperty(reg, reg, 2056, LanguageMode::STRICT);
 
   // Emit closure operations.
-  Factory* factory = isolate()->factory();
-  Handle<SharedFunctionInfo> shared_info = factory->NewSharedFunctionInfo(
-      factory->NewStringFromStaticChars("function_a"), MaybeHandle<Code>(),
-      false);
-  builder.CreateClosure(shared_info, NOT_TENURED);
+  builder.CreateClosure(NOT_TENURED);
 
   // Emit argument creation operations.
   builder.CreateArguments(CreateArgumentsType::kMappedArguments)
@@ -219,12 +215,6 @@ TEST_F(BytecodeArrayBuilderTest, AllBytecodesGenerated) {
   }
   builder.LoadLiteral(Smi::FromInt(20000000));
 
-  // CreateClosureWide
-  Handle<SharedFunctionInfo> shared_info2 = factory->NewSharedFunctionInfo(
-      factory->NewStringFromStaticChars("function_b"), MaybeHandle<Code>(),
-      false);
-  builder.CreateClosure(shared_info2, NOT_TENURED);
-
   builder.Return();
 
   // Generate BytecodeArray.