Revert of [turbofan] Split CodeGenerator::GenerateCode into AssembleCode and...

Revert of [turbofan] Split CodeGenerator::GenerateCode into AssembleCode and FinishCodeObject. (patchset #3 id:40001 of https://codereview.chromium.org/2229243003/ )

Reason for revert:
There is a data race in the initialization of the Isolate::random_number_generator()

Original issue's description:
> [turbofan] Split CodeGenerator::GenerateCode into AssembleCode and FinishCodeObject.
>
> This CL splits CodeGenerator::GenerateCode into two new functions:
> AssembleCode and FinishCodeObject. AssembleCode does not access or
> modify the JS heap, which means that AssembleCode can be executed on
> background threads. FinishCodeObject allocates the generated code object
> on the JS heap and therefore has to be executed on the main thread.
>
> Implementation details:
> The GenerateCode function has been split just before out-of-line code is
> assembled. The reason is that code stubs may be generated when
> out-of-line code is assembled, which potentially allocates these code
> stubs on the heap.
>
> - Parts of initialization of the CodeGenerator has been moved from the
> constructor to an Initialize function so that we can instantiate an empty
> CodeGenerator object in PipelineData.
>
> R=bmeurer@chromium.org, mstarzinger@chromium.org, titzer@chromium.org
>
> Committed: https://crrev.com/03058a2187e32cc4080612181802086527c116a2
> Cr-Commit-Position: refs/heads/master@{#38604}

TBR=bmeurer@chromium.org,mstarzinger@chromium.org,titzer@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

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

src/compiler/code-generator.cc

@@ -32,48 +32,44 @@ class CodeGenerator::JumpTable final : public ZoneObject {
   size_t const target_count_;
 };
 
-CodeGenerator::CodeGenerator(Zone* zone, CompilationInfo* info)
+CodeGenerator::CodeGenerator(Frame* frame, Linkage* linkage,
+                             InstructionSequence* code, CompilationInfo* info)
     : frame_access_state_(nullptr),
-      linkage_(nullptr),
-      code_(nullptr),
-      unwinding_info_writer_(zone),
+      linkage_(linkage),
+      code_(code),
+      unwinding_info_writer_(zone()),
       info_(info),
-      labels_(nullptr),
+      labels_(zone()->NewArray<Label>(code->InstructionBlockCount())),
       current_block_(RpoNumber::Invalid()),
       current_source_position_(SourcePosition::Unknown()),
       masm_(info->isolate(), nullptr, 0, CodeObjectRequired::kNo),
       resolver_(this),
-      safepoints_(zone),
-      handlers_(zone),
-      deoptimization_exits_(zone),
-      deoptimization_states_(zone),
-      deoptimization_literals_(zone),
+      safepoints_(code->zone()),
+      handlers_(code->zone()),
+      deoptimization_exits_(code->zone()),
+      deoptimization_states_(code->zone()),
+      deoptimization_literals_(code->zone()),
       inlined_function_count_(0),
-      translations_(zone),
+      translations_(code->zone()),
       last_lazy_deopt_pc_(0),
       jump_tables_(nullptr),
       ools_(nullptr),
       osr_pc_offset_(-1),
-      source_position_table_builder_(info->isolate(), zone,
-                                     info->SourcePositionRecordingMode()),
-      assemble_code_successful_(false) {}
-
-void CodeGenerator::Initialize(Frame* frame, Linkage* linkage,
-                               InstructionSequence* code) {
-  linkage_ = linkage;
-  code_ = code;
-  labels_ = zone()->NewArray<Label>(code->InstructionBlockCount());
+      source_position_table_builder_(info->isolate(), code->zone(),
+                                     info->SourcePositionRecordingMode()) {
   for (int i = 0; i < code->InstructionBlockCount(); ++i) {
     new (&labels_[i]) Label;
   }
+  CreateFrameAccessState(frame);
+}
 
-  // Create the FrameAccessState object. The Frame is immutable from here on.
+void CodeGenerator::CreateFrameAccessState(Frame* frame) {
   FinishFrame(frame);
-  frame_access_state_ = new (code->zone()) FrameAccessState(frame);
+  frame_access_state_ = new (code()->zone()) FrameAccessState(frame);
 }
 
-bool CodeGenerator::AssembleCode() {
-  DCHECK(!assemble_code_successful());
+Handle<Code> CodeGenerator::GenerateCode() {
+  CompilationInfo* info = this->info();
 
   // Open a frame scope to indicate that there is a frame on the stack.  The
   // MANUAL indicates that the scope shouldn't actually generate code to set up
@@ -85,13 +81,13 @@ bool CodeGenerator::AssembleCode() {
     ProfileEntryHookStub::MaybeCallEntryHook(masm());
   }
   // Architecture-specific, linkage-specific prologue.
-  info()->set_prologue_offset(masm()->pc_offset());
+  info->set_prologue_offset(masm()->pc_offset());
 
   // Define deoptimization literals for all inlined functions.
   DCHECK_EQ(0u, deoptimization_literals_.size());
   for (const CompilationInfo::InlinedFunctionHolder& inlined :
-       info()->inlined_functions()) {
-    if (!inlined.shared_info.is_identical_to(info()->shared_info())) {
+       info->inlined_functions()) {
+    if (!inlined.shared_info.is_identical_to(info->shared_info())) {
       DefineDeoptimizationLiteral(inlined.shared_info);
     }
   }
@@ -100,8 +96,8 @@ bool CodeGenerator::AssembleCode() {
   // Define deoptimization literals for all unoptimized code objects of inlined
   // functions. This ensures unoptimized code is kept alive by optimized code.
   for (const CompilationInfo::InlinedFunctionHolder& inlined :
-       info()->inlined_functions()) {
-    if (!inlined.shared_info.is_identical_to(info()->shared_info())) {
+       info->inlined_functions()) {
+    if (!inlined.shared_info.is_identical_to(info->shared_info())) {
       DefineDeoptimizationLiteral(inlined.inlined_code_object_root);
     }
   }
@@ -171,23 +167,11 @@ bool CodeGenerator::AssembleCode() {
       } else {
         result = AssembleBlock(block);
       }
-      if (result != kSuccess) {
-        assemble_code_successful_ = false;
-        return false;
-      }
+      if (result != kSuccess) return Handle<Code>();
       unwinding_info_writer_.EndInstructionBlock(block);
     }
   }
 
-  assemble_code_successful_ = true;
-  return true;
-}
-
-Handle<Code> CodeGenerator::FinishCodeObject() {
-  if (!assemble_code_successful_) {
-    return Handle<Code>::null();
-  }
-
   // Assemble all out-of-line code.
   if (ools_) {
     masm()->RecordComment("-- Out of line code --");
@@ -205,7 +189,7 @@ Handle<Code> CodeGenerator::FinishCodeObject() {
   }
 
   // Ensure there is space for lazy deoptimization in the code.
-  if (info()->ShouldEnsureSpaceForLazyDeopt()) {
+  if (info->ShouldEnsureSpaceForLazyDeopt()) {
     int target_offset = masm()->pc_offset() + Deoptimizer::patch_size();
     while (masm()->pc_offset() < target_offset) {
       masm()->nop();
@@ -228,8 +212,7 @@ Handle<Code> CodeGenerator::FinishCodeObject() {
   unwinding_info_writer_.Finish(masm()->pc_offset());
 
   Handle<Code> result = v8::internal::CodeGenerator::MakeCodeEpilogue(
-      masm(), unwinding_info_writer_.eh_frame_writer(), info(),
-      Handle<Object>());
+      masm(), unwinding_info_writer_.eh_frame_writer(), info, Handle<Object>());
   result->set_is_turbofanned(true);
   result->set_stack_slots(frame()->GetTotalFrameSlotCount());
   result->set_safepoint_table_offset(safepoints()->GetCodeOffset());
@@ -254,13 +237,14 @@ Handle<Code> CodeGenerator::FinishCodeObject() {
   PopulateDeoptimizationData(result);
 
   // Ensure there is space for lazy deoptimization in the relocation info.
-  if (info()->ShouldEnsureSpaceForLazyDeopt()) {
+  if (info->ShouldEnsureSpaceForLazyDeopt()) {
     Deoptimizer::EnsureRelocSpaceForLazyDeoptimization(result);
   }
 
   return result;
 }
 
+
 bool CodeGenerator::IsNextInAssemblyOrder(RpoNumber block) const {
   return code()
       ->InstructionBlockAt(current_block_)

src/compiler/code-generator.h

@@ -48,24 +48,17 @@ class InstructionOperandIterator {
 // Generates native code for a sequence of instructions.
 class CodeGenerator final : public GapResolver::Assembler {
  public:
-  CodeGenerator(Zone* zone, CompilationInfo* info);
+  explicit CodeGenerator(Frame* frame, Linkage* linkage,
+                         InstructionSequence* code, CompilationInfo* info);
 
-  // Initialize before calling AssembleCode or FinishCodeObject.
-  void Initialize(Frame* frame, Linkage* linkage, InstructionSequence* code);
-
-  // Generate native code. This function can be run off the main thread.
-  bool AssembleCode();
-
-  // Finishes the code object generated in AssembleCode. This function returns
-  // an empty handle if AssembleCode() fails or has not been called.
-  Handle<Code> FinishCodeObject();
+  // Generate native code.
+  Handle<Code> GenerateCode();
 
   InstructionSequence* code() const { return code_; }
   FrameAccessState* frame_access_state() const { return frame_access_state_; }
   const Frame* frame() const { return frame_access_state_->frame(); }
   Isolate* isolate() const { return info_->isolate(); }
   Linkage* linkage() const { return linkage_; }
-  bool assemble_code_successful() { return assemble_code_successful_; }
 
   Label* GetLabel(RpoNumber rpo) { return &labels_[rpo.ToSize()]; }
 
@@ -76,6 +69,9 @@ class CodeGenerator final : public GapResolver::Assembler {
   Zone* zone() const { return code()->zone(); }
   CompilationInfo* info() const { return info_; }
 
+  // Create the FrameAccessState object. The Frame is immutable from here on.
+  void CreateFrameAccessState(Frame* frame);
+
   // Architecture - specific frame finalization.
   void FinishFrame(Frame* frame);
 
@@ -261,11 +257,11 @@ class CodeGenerator final : public GapResolver::Assembler {
   friend class OutOfLineCode;
 
   FrameAccessState* frame_access_state_;
-  Linkage* linkage_;
-  InstructionSequence* code_;
+  Linkage* const linkage_;
+  InstructionSequence* const code_;
   UnwindingInfoWriter unwinding_info_writer_;
   CompilationInfo* const info_;
-  Label* labels_;
+  Label* const labels_;
   Label return_label_;
   RpoNumber current_block_;
   SourcePosition current_source_position_;
@@ -283,7 +279,6 @@ class CodeGenerator final : public GapResolver::Assembler {
   OutOfLineCode* ools_;
   int osr_pc_offset_;
   SourcePositionTableBuilder source_position_table_builder_;
-  bool assemble_code_successful_;
 };
 
 }  // namespace compiler

src/compiler/pipeline.cc

@@ -89,13 +89,10 @@ class PipelineData {
         outer_zone_(info_->zone()),
         zone_pool_(zone_pool),
         pipeline_statistics_(pipeline_statistics),
-        code_generator_(info->zone(), info),
         graph_zone_scope_(zone_pool_),
         graph_zone_(graph_zone_scope_.zone()),
         instruction_zone_scope_(zone_pool_),
         instruction_zone_(instruction_zone_scope_.zone()),
-        sequence_(nullptr),
-        frame_(nullptr),
         register_allocation_zone_scope_(zone_pool_),
         register_allocation_zone_(register_allocation_zone_scope_.zone()) {
     PhaseScope scope(pipeline_statistics, "init pipeline data");
@@ -119,14 +116,11 @@ class PipelineData {
         info_(info),
         debug_name_(info_->GetDebugName()),
         zone_pool_(zone_pool),
-        code_generator_(info->zone(), info),
         graph_zone_scope_(zone_pool_),
         graph_(graph),
         source_positions_(source_positions),
         instruction_zone_scope_(zone_pool_),
         instruction_zone_(instruction_zone_scope_.zone()),
-        sequence_(nullptr),
-        frame_(nullptr),
         register_allocation_zone_scope_(zone_pool_),
         register_allocation_zone_(register_allocation_zone_scope_.zone()) {}
 
@@ -137,15 +131,12 @@ class PipelineData {
         info_(info),
         debug_name_(info_->GetDebugName()),
         zone_pool_(zone_pool),
-        code_generator_(info->zone(), info),
         graph_zone_scope_(zone_pool_),
         graph_(graph),
         source_positions_(new (info->zone()) SourcePositionTable(graph_)),
         schedule_(schedule),
         instruction_zone_scope_(zone_pool_),
         instruction_zone_(instruction_zone_scope_.zone()),
-        sequence_(nullptr),
-        frame_(nullptr),
         register_allocation_zone_scope_(zone_pool_),
         register_allocation_zone_(register_allocation_zone_scope_.zone()) {}
 
@@ -156,12 +147,10 @@ class PipelineData {
         info_(info),
         debug_name_(info_->GetDebugName()),
         zone_pool_(zone_pool),
-        code_generator_(info->zone(), info),
         graph_zone_scope_(zone_pool_),
         instruction_zone_scope_(zone_pool_),
         instruction_zone_(sequence->zone()),
         sequence_(sequence),
-        frame_(nullptr),
         register_allocation_zone_scope_(zone_pool_),
         register_allocation_zone_(register_allocation_zone_scope_.zone()) {}
 
@@ -222,10 +211,6 @@ class PipelineData {
   Zone* instruction_zone() const { return instruction_zone_; }
   InstructionSequence* sequence() const { return sequence_; }
   Frame* frame() const { return frame_; }
-  CodeGenerator* code_generator() { return &code_generator_; }
-  bool assemble_code_successful() {
-    return code_generator_.assemble_code_successful();
-  }
 
   Zone* register_allocation_zone() const { return register_allocation_zone_; }
   RegisterAllocationData* register_allocation_data() const {
@@ -328,7 +313,6 @@ class PipelineData {
   PipelineStatistics* pipeline_statistics_ = nullptr;
   bool compilation_failed_ = false;
   Handle<Code> code_ = Handle<Code>::null();
-  CodeGenerator code_generator_;
 
   // All objects in the following group of fields are allocated in graph_zone_.
   // They are all set to nullptr when the graph_zone_ is destroyed.
@@ -347,11 +331,12 @@ class PipelineData {
 
   // All objects in the following group of fields are allocated in
   // instruction_zone_.  They are all set to nullptr when the instruction_zone_
-  // is destroyed.
+  // is
+  // destroyed.
   ZonePool::Scope instruction_zone_scope_;
   Zone* instruction_zone_;
-  InstructionSequence* sequence_;
-  Frame* frame_;
+  InstructionSequence* sequence_ = nullptr;
+  Frame* frame_ = nullptr;
 
   // All objects in the following group of fields are allocated in
   // register_allocation_zone_.  They are all set to nullptr when the zone is
@@ -398,8 +383,7 @@ class PipelineImpl final {
   bool OptimizeGraph(Linkage* linkage);
 
   // Perform the actual code generation and return handle to a code object.
-  bool AssembleCode(Linkage* linkage);
-  Handle<Code> FinishCodeObject();
+  Handle<Code> GenerateCode(Linkage* linkage);
 
   bool ScheduleAndSelectInstructions(Linkage* linkage);
   void RunPrintAndVerify(const char* phase, bool untyped = false);
@@ -655,8 +639,7 @@ PipelineCompilationJob::Status PipelineCompilationJob::OptimizeGraphImpl() {
 }
 
 PipelineCompilationJob::Status PipelineCompilationJob::GenerateCodeImpl() {
-  pipeline_.AssembleCode(linkage_);
-  Handle<Code> code = pipeline_.FinishCodeObject();
+  Handle<Code> code = pipeline_.GenerateCode(linkage_);
   if (code.is_null()) {
     if (info()->bailout_reason() == kNoReason) {
       return AbortOptimization(kCodeGenerationFailed);
@@ -716,8 +699,7 @@ PipelineWasmCompilationJob::OptimizeGraphImpl() {
 
 PipelineWasmCompilationJob::Status
 PipelineWasmCompilationJob::GenerateCodeImpl() {
-  pipeline_.AssembleCode(&linkage_);
-  pipeline_.FinishCodeObject();
+  pipeline_.GenerateCode(&linkage_);
   return SUCCEEDED;
 }
 
@@ -1415,23 +1397,17 @@ struct JumpThreadingPhase {
   }
 };
 
-struct AssembleCodePhase {
+
+struct GenerateCodePhase {
   static const char* phase_name() { return "generate code"; }
 
   void Run(PipelineData* data, Zone* temp_zone, Linkage* linkage) {
-    data->code_generator()->Initialize(data->frame(), linkage,
-                                       data->sequence());
-    data->code_generator()->AssembleCode();
+    CodeGenerator generator(data->frame(), linkage, data->sequence(),
+                            data->info());
+    data->set_code(generator.GenerateCode());
   }
 };
 
-struct FinishCodeObjectPhase {
-  static const char* phase_name() { return "generate code"; }
-
-  void Run(PipelineData* data, Zone* temp_zone) {
-    data->set_code(data->code_generator()->FinishCodeObject());
-  }
-};
 
 struct PrintGraphPhase {
   static const char* phase_name() { return nullptr; }
@@ -1684,8 +1660,7 @@ Handle<Code> Pipeline::GenerateCodeForTesting(CompilationInfo* info) {
 
   if (!pipeline.CreateGraph()) return Handle<Code>::null();
   if (!pipeline.OptimizeGraph(&linkage)) return Handle<Code>::null();
-  pipeline.AssembleCode(&linkage);
-  return pipeline.FinishCodeObject();
+  return pipeline.GenerateCode(&linkage);
 }
 
 // static
@@ -1813,23 +1788,13 @@ bool PipelineImpl::ScheduleAndSelectInstructions(Linkage* linkage) {
   return true;
 }
 
-bool PipelineImpl::AssembleCode(Linkage* linkage) {
-  PipelineData* data = this->data_;
-
-  data->BeginPhaseKind("assemble code");
-
-  // Assemble machine code.
-  Run<AssembleCodePhase>(linkage);
-  return data->assemble_code_successful();
-}
-
-Handle<Code> PipelineImpl::FinishCodeObject() {
+Handle<Code> PipelineImpl::GenerateCode(Linkage* linkage) {
   PipelineData* data = this->data_;
 
-  data->BeginPhaseKind("finish code generation");
+  data->BeginPhaseKind("code generation");
 
-  // Generate final code object.
-  Run<FinishCodeObjectPhase>();
+  // Generate final machine code.
+  Run<GenerateCodePhase>(linkage);
 
   Handle<Code> code = data->code();
   if (data->profiler_data()) {
@@ -1876,8 +1841,7 @@ Handle<Code> PipelineImpl::ScheduleAndGenerateCode(
   if (!ScheduleAndSelectInstructions(&linkage)) return Handle<Code>();
 
   // Generate the final machine code.
-  AssembleCode(&linkage);
-  return FinishCodeObject();
+  return GenerateCode(&linkage);
 }
 
 void PipelineImpl::AllocateRegisters(const RegisterConfiguration* config,