[turbofan] Reland "Add new JSFrameSpecialization reducer." and "Perform OSR...

[turbofan] Reland "Add new JSFrameSpecialization reducer." and "Perform OSR deconstruction early and remove type propagation.".

We have to reland these two commits at once, because the first breaks
some asm.js benchmarks without the second. The change was reverted
because of bogus checks in the verifier, which will not work in the
presence of OSR (and where hidden because of the type back propagation
hack in OSR so far). Original messages are below:

[turbofan] Add new JSFrameSpecialization reducer.

The JSFrameSpecialization specializes an OSR graph to the current
unoptimized frame on which we will perform the on-stack replacement.
This is used for asm.js functions, where we cannot reuse the OSR
code object anyway because of context specialization, and so we could as
well specialize to the max instead.

It works by replacing all OsrValues in the graph with their values
in the JavaScriptFrame.

The idea is that using this trick we get better performance without
doing the unsound backpropagation of types to OsrValues later. This
is the first step towards fixing OSR for TurboFan.

[turbofan] Perform OSR deconstruction early and remove type propagation.

This way we don't have to deal with dead pre-OSR code in the graph
and risk optimizing the wrong code, especially we don't make
optimistic assumptions in the dead code that leaks into the OSR code
(i.e. deopt guards are in dead code, but the types propagate to OSR
code via the OsrValue type back propagation).

BUG=v8:4273
LOG=n
R=jarin@chromium.org

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

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

BUILD.gn

@@ -672,6 +672,8 @@ source_set("v8_base") {
     "src/compiler/js-builtin-reducer.h",
     "src/compiler/js-context-specialization.cc",
     "src/compiler/js-context-specialization.h",
+    "src/compiler/js-frame-specialization.cc",
+    "src/compiler/js-frame-specialization.h",
     "src/compiler/js-generic-lowering.cc",
     "src/compiler/js-generic-lowering.h",
     "src/compiler/js-graph.cc",

src/compiler.cc

@@ -394,6 +394,7 @@ OptimizedCompileJob::Status OptimizedCompileJob::CreateGraph() {
     }
 
     if (info()->shared_info()->asm_function()) {
+      if (info()->osr_frame()) info()->MarkAsFrameSpecializing();
       info()->MarkAsContextSpecializing();
     } else if (FLAG_turbo_type_feedback) {
       info()->MarkAsTypeFeedbackEnabled();
@@ -712,7 +713,9 @@ static void InsertCodeIntoOptimizedCodeMap(CompilationInfo* info) {
   if (code->kind() != Code::OPTIMIZED_FUNCTION) return;  // Nothing to do.
 
   // Context specialization folds-in the context, so no sharing can occur.
-  if (code->is_turbofanned() && info->is_context_specializing()) return;
+  if (info->is_context_specializing()) return;
+  // Frame specialization implies context specialization.
+  DCHECK(!info->is_frame_specializing());
 
   // Do not cache bound functions.
   Handle<JSFunction> function = info->closure();
@@ -1469,7 +1472,8 @@ Handle<SharedFunctionInfo> Compiler::GetSharedFunctionInfo(
 MaybeHandle<Code> Compiler::GetOptimizedCode(Handle<JSFunction> function,
                                              Handle<Code> current_code,
                                              ConcurrencyMode mode,
-                                             BailoutId osr_ast_id) {
+                                             BailoutId osr_ast_id,
+                                             JavaScriptFrame* osr_frame) {
   Handle<Code> cached_code;
   if (GetCodeFromOptimizedCodeMap(
           function, osr_ast_id).ToHandle(&cached_code)) {
@@ -1527,6 +1531,7 @@ MaybeHandle<Code> Compiler::GetOptimizedCode(Handle<JSFunction> function,
       return isolate->builtins()->InOptimizationQueue();
     }
   } else {
+    info->set_osr_frame(osr_frame);
     if (GetOptimizedCodeNow(info.get())) return info->code();
   }
 

src/compiler.h

@@ -17,6 +17,7 @@ namespace internal {
 
 class AstValueFactory;
 class HydrogenCodeStub;
+class JavaScriptFrame;
 class ParseInfo;
 class ScriptData;
 
@@ -122,14 +123,15 @@ class CompilationInfo {
     kCompilingForDebugging = 1 << 7,
     kSerializing = 1 << 8,
     kContextSpecializing = 1 << 9,
-    kInliningEnabled = 1 << 10,
-    kTypingEnabled = 1 << 11,
-    kDisableFutureOptimization = 1 << 12,
-    kSplittingEnabled = 1 << 13,
-    kTypeFeedbackEnabled = 1 << 14,
-    kDeoptimizationEnabled = 1 << 15,
-    kSourcePositionsEnabled = 1 << 16,
-    kFirstCompile = 1 << 17,
+    kFrameSpecializing = 1 << 10,
+    kInliningEnabled = 1 << 11,
+    kTypingEnabled = 1 << 12,
+    kDisableFutureOptimization = 1 << 13,
+    kSplittingEnabled = 1 << 14,
+    kTypeFeedbackEnabled = 1 << 15,
+    kDeoptimizationEnabled = 1 << 16,
+    kSourcePositionsEnabled = 1 << 17,
+    kFirstCompile = 1 << 18,
   };
 
   explicit CompilationInfo(ParseInfo* parse_info);
@@ -217,6 +219,10 @@ class CompilationInfo {
 
   bool is_context_specializing() const { return GetFlag(kContextSpecializing); }
 
+  void MarkAsFrameSpecializing() { SetFlag(kFrameSpecializing); }
+
+  bool is_frame_specializing() const { return GetFlag(kFrameSpecializing); }
+
   void MarkAsTypeFeedbackEnabled() { SetFlag(kTypeFeedbackEnabled); }
 
   bool is_type_feedback_enabled() const {
@@ -388,6 +394,8 @@ class CompilationInfo {
     DCHECK(height >= 0);
     osr_expr_stack_height_ = height;
   }
+  JavaScriptFrame* osr_frame() const { return osr_frame_; }
+  void set_osr_frame(JavaScriptFrame* osr_frame) { osr_frame_ = osr_frame; }
 
 #if DEBUG
   void PrintAstForTesting();
@@ -492,6 +500,9 @@ class CompilationInfo {
 
   int osr_expr_stack_height_;
 
+  // The current OSR frame for specialization or {nullptr}.
+  JavaScriptFrame* osr_frame_ = nullptr;
+
   Type::FunctionType* function_type_;
 
   DISALLOW_COPY_AND_ASSIGN(CompilationInfo);
@@ -662,10 +673,9 @@ class Compiler : public AllStatic {
   // In the latter case, return the InOptimizationQueue builtin.  On failure,
   // return the empty handle.
   MUST_USE_RESULT static MaybeHandle<Code> GetOptimizedCode(
-      Handle<JSFunction> function,
-      Handle<Code> current_code,
-      ConcurrencyMode mode,
-      BailoutId osr_ast_id = BailoutId::None());
+      Handle<JSFunction> function, Handle<Code> current_code,
+      ConcurrencyMode mode, BailoutId osr_ast_id = BailoutId::None(),
+      JavaScriptFrame* osr_frame = nullptr);
 
   // Generate and return code from previously queued optimization job.
   // On failure, return the empty handle.

src/compiler/js-frame-specialization.cc

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/compiler/js-frame-specialization.h"
+
+#include "src/compiler/js-graph.h"
+#include "src/compiler/linkage.h"
+#include "src/frames-inl.h"
+
+namespace v8 {
+namespace internal {
+namespace compiler {
+
+Reduction JSFrameSpecialization::Reduce(Node* node) {
+  switch (node->opcode()) {
+    case IrOpcode::kOsrValue:
+      return ReduceOsrValue(node);
+    case IrOpcode::kParameter:
+      return ReduceParameter(node);
+    default:
+      break;
+  }
+  return NoChange();
+}
+
+
+Reduction JSFrameSpecialization::ReduceOsrValue(Node* node) {
+  DCHECK_EQ(IrOpcode::kOsrValue, node->opcode());
+  DisallowHeapAllocation no_gc;
+  Object* object;
+  int const index = OpParameter<int>(node);
+  int const parameters_count = frame()->ComputeParametersCount() + 1;
+  if (index == Linkage::kOsrContextSpillSlotIndex) {
+    object = frame()->context();
+  } else if (index >= parameters_count) {
+    object = frame()->GetExpression(index - parameters_count);
+  } else {
+    // The OsrValue index 0 is the receiver.
+    object = index ? frame()->GetParameter(index - 1) : frame()->receiver();
+  }
+  return Replace(jsgraph()->Constant(handle(object, isolate())));
+}
+
+
+Reduction JSFrameSpecialization::ReduceParameter(Node* node) {
+  DCHECK_EQ(IrOpcode::kParameter, node->opcode());
+  DisallowHeapAllocation no_gc;
+  Object* object;
+  int const index = ParameterIndexOf(node->op());
+  int const parameters_count = frame()->ComputeParametersCount() + 1;
+  if (index == Linkage::kJSFunctionCallClosureParamIndex) {
+    object = frame()->function();
+  } else if (index == parameters_count) {
+    // The Parameter index (arity + 1) is the context.
+    object = frame()->context();
+  } else {
+    // The Parameter index 0 is the receiver.
+    object = index ? frame()->GetParameter(index - 1) : frame()->receiver();
+  }
+  return Replace(jsgraph()->Constant(handle(object, isolate())));
+}
+
+
+Isolate* JSFrameSpecialization::isolate() const { return jsgraph()->isolate(); }
+
+}  // namespace compiler
+}  // namespace internal
+}  // namespace v8

src/compiler/js-frame-specialization.h

+// Copyright 2015 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_COMPILER_JS_FRAME_SPECIALIZATION_H_
+#define V8_COMPILER_JS_FRAME_SPECIALIZATION_H_
+
+#include "src/compiler/graph-reducer.h"
+
+namespace v8 {
+namespace internal {
+namespace compiler {
+
+// Forward declarations.
+class JSGraph;
+
+
+class JSFrameSpecialization final : public Reducer {
+ public:
+  JSFrameSpecialization(JavaScriptFrame const* frame, JSGraph* jsgraph)
+      : frame_(frame), jsgraph_(jsgraph) {}
+  ~JSFrameSpecialization() final {}
+
+  Reduction Reduce(Node* node) final;
+
+ private:
+  Reduction ReduceOsrValue(Node* node);
+  Reduction ReduceParameter(Node* node);
+
+  Isolate* isolate() const;
+  JavaScriptFrame const* frame() const { return frame_; }
+  JSGraph* jsgraph() const { return jsgraph_; }
+
+  JavaScriptFrame const* const frame_;
+  JSGraph* const jsgraph_;
+
+  DISALLOW_COPY_AND_ASSIGN(JSFrameSpecialization);
+};
+
+}  // namespace compiler
+}  // namespace internal
+}  // namespace v8
+
+#endif  // V8_COMPILER_JS_FRAME_SPECIALIZATION_H_

src/compiler/osr.cc

@@ -249,40 +249,6 @@ static void PeelOuterLoopsForOsr(Graph* graph, CommonOperatorBuilder* common,
 }
 
 
-static void TransferOsrValueTypesFromLoopPhis(Zone* zone, Node* osr_loop_entry,
-                                              Node* osr_loop) {
-  // Find the index of the osr loop entry into the loop.
-  int index = 0;
-  for (index = 0; index < osr_loop->InputCount(); index++) {
-    if (osr_loop->InputAt(index) == osr_loop_entry) break;
-  }
-  if (index == osr_loop->InputCount()) return;
-
-  for (Node* osr_value : osr_loop_entry->uses()) {
-    if (osr_value->opcode() != IrOpcode::kOsrValue) continue;
-    bool unknown = true;
-    for (Node* phi : osr_value->uses()) {
-      if (phi->opcode() != IrOpcode::kPhi) continue;
-      if (NodeProperties::GetControlInput(phi) != osr_loop) continue;
-      if (phi->InputAt(index) != osr_value) continue;
-      if (NodeProperties::IsTyped(phi)) {
-        // Transfer the type from the phi to the OSR value itself.
-        Bounds phi_bounds = NodeProperties::GetBounds(phi);
-        if (unknown) {
-          NodeProperties::SetBounds(osr_value, phi_bounds);
-        } else {
-          Bounds osr_bounds = NodeProperties::GetBounds(osr_value);
-          NodeProperties::SetBounds(osr_value,
-                                    Bounds::Both(phi_bounds, osr_bounds, zone));
-        }
-        unknown = false;
-      }
-    }
-    if (unknown) NodeProperties::SetBounds(osr_value, Bounds::Unbounded(zone));
-  }
-}
-
-
 void OsrHelper::Deconstruct(JSGraph* jsgraph, CommonOperatorBuilder* common,
                             Zone* tmp_zone) {
   Graph* graph = jsgraph->graph();
@@ -313,9 +279,6 @@ void OsrHelper::Deconstruct(JSGraph* jsgraph, CommonOperatorBuilder* common,
 
   CHECK(osr_loop);  // Should have found the OSR loop.
 
-  // Transfer the types from loop phis to the OSR values which flow into them.
-  TransferOsrValueTypesFromLoopPhis(graph->zone(), osr_loop_entry, osr_loop);
-
   // Analyze the graph to determine how deeply nested the OSR loop is.
   LoopTree* loop_tree = LoopFinder::BuildLoopTree(graph, tmp_zone);
 

src/compiler/pipeline.cc

@@ -26,6 +26,7 @@
 #include "src/compiler/instruction-selector.h"
 #include "src/compiler/js-builtin-reducer.h"
 #include "src/compiler/js-context-specialization.h"
+#include "src/compiler/js-frame-specialization.h"
 #include "src/compiler/js-generic-lowering.h"
 #include "src/compiler/js-inlining.h"
 #include "src/compiler/js-intrinsic-lowering.h"
@@ -496,12 +497,17 @@ struct InliningPhase {
     CommonOperatorReducer common_reducer(&graph_reducer, data->graph(),
                                          data->common(), data->machine());
     JSContextSpecializer context_specializer(&graph_reducer, data->jsgraph());
+    JSFrameSpecialization frame_specialization(data->info()->osr_frame(),
+                                               data->jsgraph());
     JSInliner inliner(&graph_reducer, data->info()->is_inlining_enabled()
                                           ? JSInliner::kGeneralInlining
                                           : JSInliner::kRestrictedInlining,
                       temp_zone, data->info(), data->jsgraph());
     AddReducer(data, &graph_reducer, &dead_code_elimination);
     AddReducer(data, &graph_reducer, &common_reducer);
+    if (data->info()->is_frame_specializing()) {
+      AddReducer(data, &graph_reducer, &frame_specialization);
+    }
     if (data->info()->is_context_specializing()) {
       AddReducer(data, &graph_reducer, &context_specializer);
     }
@@ -1035,6 +1041,12 @@ Handle<Code> Pipeline::GenerateCode() {
   if (data.compilation_failed()) return Handle<Code>::null();
   RunPrintAndVerify("Initial untyped", true);
 
+  // Perform OSR deconstruction.
+  if (info()->is_osr()) {
+    Run<OsrDeconstructionPhase>();
+    RunPrintAndVerify("OSR deconstruction", true);
+  }
+
   // Perform context specialization and inlining (if enabled).
   Run<InliningPhase>();
   RunPrintAndVerify("Inlined", true);
@@ -1071,11 +1083,6 @@ Handle<Code> Pipeline::GenerateCode() {
       RunPrintAndVerify("Loop peeled");
     }
 
-    if (info()->is_osr()) {
-      Run<OsrDeconstructionPhase>();
-      RunPrintAndVerify("OSR deconstruction");
-    }
-
     if (info()->is_type_feedback_enabled()) {
       Run<JSTypeFeedbackPhase>();
       RunPrintAndVerify("JSType feedback");
@@ -1095,12 +1102,6 @@ Handle<Code> Pipeline::GenerateCode() {
     Run<ChangeLoweringPhase>();
     // TODO(jarin, rossberg): Remove UNTYPED once machine typing works.
     RunPrintAndVerify("Lowered changes", true);
-  } else {
-    if (info()->is_osr()) {
-      Run<OsrDeconstructionPhase>();
-      if (info()->bailout_reason() != kNoReason) return Handle<Code>::null();
-      RunPrintAndVerify("OSR deconstruction", true);
-    }
   }
 
   // Lower any remaining generic JSOperators.

src/compiler/typer.cc

@@ -582,16 +582,6 @@ Bounds Typer::Visitor::TypeParameter(Node* node) {
 
 
 Bounds Typer::Visitor::TypeOsrValue(Node* node) {
-  if (node->InputAt(0)->opcode() == IrOpcode::kOsrLoopEntry) {
-    // Before deconstruction, OSR values have type {None} to avoid polluting
-    // the types of phis and other nodes in the graph.
-    return Bounds(Type::None(), Type::None());
-  }
-  if (NodeProperties::IsTyped(node)) {
-    // After deconstruction, OSR values may have had a type explicitly set.
-    return NodeProperties::GetBounds(node);
-  }
-  // Otherwise, be conservative.
   return Bounds::Unbounded(zone());
 }
 

src/compiler/verifier.cc

@@ -584,7 +584,9 @@ void Verifier::Visitor::Check(Node* node) {
       break;
     }
     case IrOpcode::kJSForInDone: {
-      CheckValueInputIs(node, 0, Type::UnsignedSmall());
+      // TODO(bmeurer): OSR breaks this invariant, although the node is not user
+      // visible, so we know it is safe (fullcodegen has an unsigned smi there).
+      // CheckValueInputIs(node, 0, Type::UnsignedSmall());
       break;
     }
     case IrOpcode::kJSForInNext: {
@@ -592,7 +594,9 @@ void Verifier::Visitor::Check(Node* node) {
       break;
     }
     case IrOpcode::kJSForInStep: {
-      CheckValueInputIs(node, 0, Type::UnsignedSmall());
+      // TODO(bmeurer): OSR breaks this invariant, although the node is not user
+      // visible, so we know it is safe (fullcodegen has an unsigned smi there).
+      // CheckValueInputIs(node, 0, Type::UnsignedSmall());
       CheckUpperIs(node, Type::UnsignedSmall());
       break;
     }

src/runtime/runtime-compiler.cc

@@ -219,11 +219,12 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
   BailoutId ast_id = caller_code->TranslatePcOffsetToAstId(pc_offset);
   DCHECK(!ast_id.IsNone());
 
-  Compiler::ConcurrencyMode mode =
-      isolate->concurrent_osr_enabled() &&
-              (function->shared()->ast_node_count() > 512)
-          ? Compiler::CONCURRENT
-          : Compiler::NOT_CONCURRENT;
+  // Disable concurrent OSR for asm.js, to enable frame specialization.
+  Compiler::ConcurrencyMode mode = (isolate->concurrent_osr_enabled() &&
+                                    !function->shared()->asm_function() &&
+                                    function->shared()->ast_node_count() > 512)
+                                       ? Compiler::CONCURRENT
+                                       : Compiler::NOT_CONCURRENT;
   Handle<Code> result = Handle<Code>::null();
 
   OptimizedCompileJob* job = NULL;
@@ -258,8 +259,9 @@ RUNTIME_FUNCTION(Runtime_CompileForOnStackReplacement) {
       function->PrintName();
       PrintF(" at AST id %d]\n", ast_id.ToInt());
     }
-    MaybeHandle<Code> maybe_result =
-        Compiler::GetOptimizedCode(function, caller_code, mode, ast_id);
+    MaybeHandle<Code> maybe_result = Compiler::GetOptimizedCode(
+        function, caller_code, mode, ast_id,
+        (mode == Compiler::NOT_CONCURRENT) ? frame : nullptr);
     if (maybe_result.ToHandle(&result) &&
         result.is_identical_to(isolate->builtins()->InOptimizationQueue())) {
       // Optimization is queued.  Return to check later.

test/cctest/compiler/test-osr.cc

@@ -165,30 +165,6 @@ TEST(Deconstruct_osr1) {
 }
 
 
-TEST(Deconstruct_osr1_type) {
-  OsrDeconstructorTester T(1);
-
-  Node* loop = T.NewOsrLoop(1);
-  Node* osr_phi =
-      T.NewOsrPhi(loop, T.jsgraph.OneConstant(), 0, T.jsgraph.ZeroConstant());
-  Type* type = Type::Signed32();
-  NodeProperties::SetBounds(osr_phi, Bounds(type, type));
-
-  Node* ret = T.graph.NewNode(T.common.Return(), osr_phi, T.start, loop);
-  T.graph.SetEnd(ret);
-
-  OsrHelper helper(0, 0);
-  helper.Deconstruct(&T.jsgraph, &T.common, T.main_zone());
-
-  CHECK_EQ(type, NodeProperties::GetBounds(T.osr_values[0]).lower);
-  CHECK_EQ(type, NodeProperties::GetBounds(T.osr_values[0]).upper);
-
-  CheckInputs(loop, T.start, loop);
-  CheckInputs(osr_phi, T.osr_values[0], T.jsgraph.ZeroConstant(), loop);
-  CheckInputs(ret, osr_phi, T.start, loop);
-}
-
-
 TEST(Deconstruct_osr_remove_prologue) {
   OsrDeconstructorTester T(1);
   Diamond d(&T.graph, &T.common, T.p0);

tools/gyp/v8.gyp

@@ -505,6 +505,8 @@
         '../../src/compiler/js-builtin-reducer.h',
         '../../src/compiler/js-context-specialization.cc',
         '../../src/compiler/js-context-specialization.h',
+        '../../src/compiler/js-frame-specialization.cc',
+        '../../src/compiler/js-frame-specialization.h',
         '../../src/compiler/js-generic-lowering.cc',
         '../../src/compiler/js-generic-lowering.h',
         '../../src/compiler/js-graph.cc',