Introduce an OptimizingCompiler class, responsible for maintaining the state needed to run Cranksha…

src/compiler.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Redistribution and use in source and binary forms, with or without
 // modification, are permitted provided that the following conditions are
 // met:
 //
 //     * Redistributions of source code must retain the above copyright
 //       notice, this list of conditions and the following disclaimer.
 //     * Redistributions in binary form must reproduce the above
 //       copyright notice, this list of conditions and the following
 //       disclaimer in the documentation and/or other materials provided
@@ skipping 147 matching lines @@
   return false;
 #endif
 }
 
 
 static bool AlwaysFullCompiler(Isolate* isolate) {
   return FLAG_always_full_compiler || IsDebuggerActive(isolate);
 }
 
 
-static void FinishOptimization(Handle<JSFunction> function, int64_t start) {
-  int opt_count = function->shared()->opt_count();
-  function->shared()->set_opt_count(opt_count + 1);
-  double ms = static_cast<double>(OS::Ticks() - start) / 1000;
-  if (FLAG_trace_opt) {
-    PrintF("[optimizing: ");
-    function->PrintName();
-    PrintF(" / %" V8PRIxPTR, reinterpret_cast<intptr_t>(*function));
-    PrintF(" - took %0.3f ms]\n", ms);
-  }
-  if (FLAG_trace_opt_stats) {
-    static double compilation_time = 0.0;
-    static int compiled_functions = 0;
-    static int code_size = 0;
+OptimizingCompiler::Status OptimizingCompiler::InnerCreateHGraph() {
+  ASSERT(V8::UseCrankshaft());
+  ASSERT(info()->IsOptimizing());
+  ASSERT(!info()->IsCompilingForDebugging());
 
-    compilation_time += ms;
-    compiled_functions++;
-    code_size += function->shared()->SourceSize();
-    PrintF("Compiled: %d functions with %d byte source size in %fms.\n",
-           compiled_functions,
-           code_size,
-           compilation_time);
-  }
-}
-
-
-static bool MakeCrankshaftCode(CompilationInfo* info) {
-  ASSERT(V8::UseCrankshaft());
-  ASSERT(info->IsOptimizing());
-  ASSERT(!info->IsCompilingForDebugging());
-
-  // We should never arrive here if there is not code object on the
+  // We should never arrive here if there is no code object on the
   // shared function object.
-  Handle<Code> code(info->shared_info()->code());
+  Handle<Code> code(info()->shared_info()->code());
   ASSERT(code->kind() == Code::FUNCTION);
 
   // We should never arrive here if optimization has been disabled on the
   // shared function info.
-  ASSERT(!info->shared_info()->optimization_disabled());
+  ASSERT(!info()->shared_info()->optimization_disabled());
 
   // Fall back to using the full code generator if it's not possible
   // to use the Hydrogen-based optimizing compiler. We already have
   // generated code for this from the shared function object.
-  if (AlwaysFullCompiler(info->isolate())) {
-    info->SetCode(code);
-    return true;
+  if (AlwaysFullCompiler(info()->isolate())) {
+    info()->SetCode(code);
+    return BAILED_OUT;
   }
 
   // Limit the number of times we re-compile a functions with
   // the optimizing compiler.
   const int kMaxOptCount =
       FLAG_deopt_every_n_times == 0 ? Compiler::kDefaultMaxOptCount : 1000;
-  if (info->shared_info()->opt_count() > kMaxOptCount) {
-    info->AbortOptimization();
-    info->shared_info()->DisableOptimization();
-    // True indicates the compilation pipeline is still going, not
-    // necessarily that we optimized the code.
-    return true;
+  if (info()->shared_info()->opt_count() > kMaxOptCount) {
+    info()->AbortOptimization();
+    info()->shared_info()->DisableOptimization();
+    return BAILED_OUT;
   }
 
   // Due to an encoding limit on LUnallocated operands in the Lithium
   // language, we cannot optimize functions with too many formal parameters
   // or perform on-stack replacement for function with too many
   // stack-allocated local variables.
   //
   // The encoding is as a signed value, with parameters and receiver using
   // the negative indices and locals the non-negative ones.
   const int parameter_limit = -LUnallocated::kMinFixedIndex;
   const int locals_limit = LUnallocated::kMaxFixedIndex;
-  Scope* scope = info->scope();
+  Scope* scope = info()->scope();
   if ((scope->num_parameters() + 1) > parameter_limit ||
-      (info->osr_ast_id() != AstNode::kNoNumber &&
+      (info()->osr_ast_id() != AstNode::kNoNumber &&
        scope->num_parameters() + 1 + scope->num_stack_slots() > locals_limit)) {
-    info->AbortOptimization();
-    info->shared_info()->DisableOptimization();
-    // True indicates the compilation pipeline is still going, not
-    // necessarily that we optimized the code.
-    return true;
+    info()->AbortOptimization();
+    info()->shared_info()->DisableOptimization();
+    return BAILED_OUT;
   }
 
   // Take --hydrogen-filter into account.
-  Handle<String> name = info->function()->debug_name();
+  Handle<String> name = info()->function()->debug_name();
   if (*FLAG_hydrogen_filter != '\0') {
     Vector<const char> filter = CStrVector(FLAG_hydrogen_filter);
     if ((filter[0] == '-'
          && name->IsEqualTo(filter.SubVector(1, filter.length())))
         || (filter[0] != '-' && !name->IsEqualTo(filter))) {
-      info->SetCode(code);
-      return true;
+      info()->SetCode(code);
+      return BAILED_OUT;
     }
   }
 
+  Timer t(this);

[danno, 2012/07/16 12:19:09]

nit: Move this down after the comment so diff is clearer.

[sanjoy, 2012/07/16 13:46:46]

Done.

   // Recompile the unoptimized version of the code if the current version
   // doesn't have deoptimization support. Alternatively, we may decide to
   // run the full code generator to get a baseline for the compile-time
   // performance of the hydrogen-based compiler.
-  int64_t start = OS::Ticks();
-  bool should_recompile = !info->shared_info()->has_deoptimization_support();
+  bool should_recompile = !info()->shared_info()->has_deoptimization_support();
   if (should_recompile || FLAG_hydrogen_stats) {
     HPhase phase(HPhase::kFullCodeGen);
-    CompilationInfoWithZone unoptimized(info->shared_info());
+    CompilationInfoWithZone unoptimized(info()->shared_info());
     // Note that we use the same AST that we will use for generating the
     // optimized code.
-    unoptimized.SetFunction(info->function());
-    unoptimized.SetScope(info->scope());
+    unoptimized.SetFunction(info()->function());
+    unoptimized.SetScope(info()->scope());
     if (should_recompile) unoptimized.EnableDeoptimizationSupport();
     bool succeeded = FullCodeGenerator::MakeCode(&unoptimized);
     if (should_recompile) {
-      if (!succeeded) return false;
-      Handle<SharedFunctionInfo> shared = info->shared_info();
+      if (!succeeded) return FAILED;
+      Handle<SharedFunctionInfo> shared = info()->shared_info();
       shared->EnableDeoptimizationSupport(*unoptimized.code());
       // The existing unoptimized code was replaced with the new one.
       Compiler::RecordFunctionCompilation(
           Logger::LAZY_COMPILE_TAG, &unoptimized, shared);
     }
   }
 
   // Check that the unoptimized, shared code is ready for
   // optimizations.  When using the always_opt flag we disregard the
   // optimizable marker in the code object and optimize anyway. This
   // is safe as long as the unoptimized code has deoptimization
   // support.
   ASSERT(FLAG_always_opt || code->optimizable());
-  ASSERT(info->shared_info()->has_deoptimization_support());
+  ASSERT(info()->shared_info()->has_deoptimization_support());
 
   if (FLAG_trace_hydrogen) {
     PrintF("-----------------------------------------------------------\n");
     PrintF("Compiling method %s using hydrogen\n", *name->ToCString());
-    HTracer::Instance()->TraceCompilation(info->function());
+    HTracer::Instance()->TraceCompilation(info()->function());
+  }
+  Handle<Context> global_context(

[danno, 2012/07/16 12:19:09]

This fits on one line like in the original, no?

[sanjoy, 2012/07/16 13:46:46]

No, because I had to replace `info' with `info()'.

+      info()->closure()->context()->global_context());
+  oracle_ = new(info()->zone()) TypeFeedbackOracle(
+      code, global_context, info()->isolate(), info()->zone());
+  graph_builder_ = new(info()->zone()) HGraphBuilder(info(), oracle_);
+  HPhase phase(HPhase::kTotal);
+  graph_ = graph_builder_->CreateGraph();
+  is_inline_bailout_ = graph_builder_->inline_bailout();
+
+  if (info()->isolate()->has_pending_exception()) {
+    info()->SetCode(Handle<Code>::null());
+    return FAILED;
   }
 

[danno, 2012/07/16 12:19:09]

Don't add a line here, please keep original white space as much as possible to
help make diff clearer.

[sanjoy, 2012/07/16 13:46:46]

Done.

[sanjoy, 2012/07/16 13:46:46]

Done.

-  Handle<Context> global_context(info->closure()->context()->global_context());
-  TypeFeedbackOracle oracle(code, global_context, info->isolate(),
-                            info->zone());
-  HGraphBuilder builder(info, &oracle);
-  HPhase phase(HPhase::kTotal);
-  HGraph* graph = builder.CreateGraph();
-  if (info->isolate()->has_pending_exception()) {
-    info->SetCode(Handle<Code>::null());
-    return false;
+  if (graph_ == NULL) {
+    AbortCrankshaft();
+    return BAILED_OUT;
   }
 
-  if (graph != NULL) {
-    SmartArrayPointer<char> bailout_reason;
-    if (!graph->Optimize(&bailout_reason)) {
-      if (!bailout_reason.is_empty()) builder.Bailout(*bailout_reason);
-    } else {
-      LChunk* chunk = LChunk::NewChunk(graph);
-      if (chunk != NULL) {
-        Handle<Code> optimized_code = chunk->Codegen();
-        if (!optimized_code.is_null()) {
-          info->SetCode(optimized_code);
-          FinishOptimization(info->closure(), start);
-          return true;
-        }
-      }
-    }
+  return SUCCEEDED;
+}
+
+OptimizingCompiler::Status OptimizingCompiler::InnerOptimizeHGraph() {
+  Timer t(this);
+  ASSERT(graph_ != NULL);
+  SmartArrayPointer<char> bailout_reason;
+  if (!graph_->Optimize(&bailout_reason)) {
+    if (!bailout_reason.is_empty()) graph_builder_->Bailout(*bailout_reason);
+    AbortCrankshaft();
+    return BAILED_OUT;

[danno, 2012/07/16 12:19:09]

How about AbortCrankshaft just returns a OptimizingCompiler::Status so you can
write return AbortCrankshaft();

+  } else {
+    chunk_ = LChunk::NewChunk(graph_);
   }
-
-  // Keep using the shared code.
-  info->AbortOptimization();
-  if (!builder.inline_bailout()) {
-    // Mark the shared code as unoptimizable unless it was an inlined
-    // function that bailed out.
-    info->shared_info()->DisableOptimization();
+  if (chunk_ == NULL) {

[danno, 2012/07/16 12:19:09]

Why will chunk ever be null? It seems like the only path is in the else above,
why not put the test there and always return SUCCEEDED at the end of this
funciton?

[sanjoy, 2012/07/16 13:46:46]

Done.

+    AbortCrankshaft();
+    return BAILED_OUT;
+  } else {
+    return SUCCEEDED;
   }
-  // True indicates the compilation pipeline is still going, not necessarily
-  // that we optimized the code.
-  return true;
 }
 
 
+OptimizingCompiler::Status OptimizingCompiler::InnerGenerateAndInstallCode() {
+  Timer timer(this);
+  ASSERT(chunk_ != NULL);
+  ASSERT(graph_ != NULL);
+  Handle<Code> optimized_code = chunk_->Codegen();
+  if (optimized_code.is_null()) {
+    AbortCrankshaft();
+    return BAILED_OUT;
+  }
+  info()->SetCode(optimized_code);
+  return SUCCEEDED;
+}
+
+
+void OptimizingCompiler::RecordOptimizationStats() {

[danno, 2012/07/16 12:19:09]

Can you please move this back up to where FinishOptimization used to be? Makes
the diffs much simpler.

[sanjoy, 2012/07/16 13:46:46]

Done.

+  Handle<JSFunction> function = info()->closure();
+  int opt_count = function->shared()->opt_count();
+  function->shared()->set_opt_count(opt_count + 1);
+  double ms = static_cast<double>(total_time_taken_) / 1000;
+  if (FLAG_trace_opt) {
+    PrintF("[optimizing: ");
+    function->PrintName();
+    PrintF(" / %" V8PRIxPTR, reinterpret_cast<intptr_t>(*function));
+    PrintF(" - took %0.3f ms]\n", ms);
+  }
+  if (FLAG_trace_opt_stats) {
+    static double compilation_time = 0.0;
+    static int compiled_functions = 0;
+    static int code_size = 0;
+
+    compilation_time += ms;
+    compiled_functions++;
+    code_size += function->shared()->SourceSize();
+    PrintF("Compiled: %d functions with %d byte source size in %fms.\n",
+           compiled_functions,
+           code_size,
+           compilation_time);
+  }
+}
+
+// A return value of true indicates the compilation pipeline is still
+// going, not necessarily that we optimized the code.
+static bool MakeCrankshaftCode(CompilationInfo* info) {

[danno, 2012/07/16 12:19:09]

Can you please move this up where MakeCrankshaftCode used to be? Again, to
minimize the diffs if possible.

[sanjoy, 2012/07/16 13:46:46]

Done.

+  OptimizingCompiler compiler(info);
+  OptimizingCompiler::Status status = compiler.CreateHGraph();
+
+  if (status == OptimizingCompiler::SUCCEEDED) {
+    status = compiler.OptimizeHGraph();
+  } else {
+    return status != OptimizingCompiler::FAILED;
+  }
+
+  if (status == OptimizingCompiler::SUCCEEDED) {
+    status = compiler.GenerateAndInstallCode();
+  }
+  return status != OptimizingCompiler::FAILED;
+}
+
+
 static bool GenerateCode(CompilationInfo* info) {
   bool is_optimizing = V8::UseCrankshaft() &&
                        !info->IsCompilingForDebugging() &&
                        info->IsOptimizing();
   if (is_optimizing) {
     return MakeCrankshaftCode(info);
   } else {
     if (info->IsOptimizing()) {
       // Have the CompilationInfo decide if the compilation should be
       // BASE or NONOPT.
@@ skipping 528 matching lines @@
     }
   }
 
   GDBJIT(AddCode(Handle<String>(shared->DebugName()),
                  Handle<Script>(info->script()),
                  Handle<Code>(info->code()),
                  info));
 }
 
 } }  // namespace v8::internal