Reland "[wasm] Remove WasmInstructionBuffer"

This is a reland of ac654646.
Two constants defined in {AssemblerBase} were not defined anywhere,
which is fixed now.

Original change's description:
> [wasm] Remove WasmInstructionBuffer
>
> {WasmInstructionBuffer} was basically a wrapper around {AssemblerBuffer}
> which remembered the last {AssemblerBuffer} on {Grow()}. Since the
> {Assembler} itself already keeps track of the latest {AssemblerBuffer},
> this functionality is mostly redundant. All we need instead is a method
> to retrieve the {AssemblerBuffer} from the {Assembler}.
>
> This CL thus removes {WasmInstructionBuffer} and instead adds
> {AssemblerBase::ReleaseBuffer}.
>
> R=jkummerow@chromium.org, mslekova@chromium.org
> CC=dlehmann@google.com
>
> Bug: v8:11714
> Change-Id: Id07945b67992802a6177bf09e5f5c5be08f657b0
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2982013
> Commit-Queue: Clemens Backes <clemensb@chromium.org>
> Reviewed-by: Maya Lekova <mslekova@chromium.org>
> Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#75336}

Bug: v8:11714
Change-Id: I8797de1a7a78a93aaef936e46bfd1e73ec2cc9d5
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2982015Reviewed-by: Maya Lekova <mslekova@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75338}

src/codegen/assembler.cc

@@ -99,9 +99,10 @@ namespace {
 class DefaultAssemblerBuffer : public AssemblerBuffer {
  public:
   explicit DefaultAssemblerBuffer(int size)
-      : buffer_(base::OwnedVector<uint8_t>::NewForOverwrite(size)) {
+      : buffer_(base::OwnedVector<uint8_t>::NewForOverwrite(
+            std::max(AssemblerBase::kMinimalBufferSize, size))) {
 #ifdef DEBUG
-    ZapCode(reinterpret_cast<Address>(buffer_.start()), size);
+    ZapCode(reinterpret_cast<Address>(buffer_.start()), buffer_.size());
 #endif
   }
 
@@ -219,6 +220,12 @@ std::unique_ptr<AssemblerBuffer> NewOnHeapAssemblerBuffer(Isolate* isolate,
 // -----------------------------------------------------------------------------
 // Implementation of AssemblerBase
 
+// static
+constexpr int AssemblerBase::kMinimalBufferSize;
+
+// static
+constexpr int AssemblerBase::kDefaultBufferSize;
+
 AssemblerBase::AssemblerBase(const AssemblerOptions& options,
                              std::unique_ptr<AssemblerBuffer> buffer)
     : buffer_(std::move(buffer)),

src/codegen/assembler.h

@@ -292,6 +292,15 @@ class V8_EXPORT_PRIVATE AssemblerBase : public Malloced {
   int buffer_size() const { return buffer_->size(); }
   int instruction_size() const { return pc_offset(); }
 
+  std::unique_ptr<AssemblerBuffer> ReleaseBuffer() {
+    std::unique_ptr<AssemblerBuffer> buffer = std::move(buffer_);
+    DCHECK_NULL(buffer_);
+    // Reset fields to prevent accidental further modifications of the buffer.
+    buffer_start_ = nullptr;
+    pc_ = nullptr;
+    return buffer;
+  }
+
   // This function is called when code generation is aborted, so that
   // the assembler could clean up internal data structures.
   virtual void AbortedCodeGeneration() {}

src/compiler/backend/code-generator.cc

@@ -48,8 +48,7 @@ CodeGenerator::CodeGenerator(
     int start_source_position, JumpOptimizationInfo* jump_opt,
     PoisoningMitigationLevel poisoning_level, const AssemblerOptions& options,
     Builtin builtin, size_t max_unoptimized_frame_height,
-    size_t max_pushed_argument_count, std::unique_ptr<AssemblerBuffer> buffer,
-    const char* debug_name)
+    size_t max_pushed_argument_count, const char* debug_name)
     : zone_(codegen_zone),
       isolate_(isolate),
       frame_access_state_(nullptr),
@@ -62,7 +61,7 @@ CodeGenerator::CodeGenerator(
       current_block_(RpoNumber::Invalid()),
       start_source_position_(start_source_position),
       current_source_position_(SourcePosition::Unknown()),
-      tasm_(isolate, options, CodeObjectRequired::kNo, std::move(buffer)),
+      tasm_(isolate, options, CodeObjectRequired::kNo),
       resolver_(this),
       safepoints_(codegen_zone),
       handlers_(codegen_zone),

src/compiler/backend/code-generator.h

@@ -127,8 +127,7 @@ class V8_EXPORT_PRIVATE CodeGenerator final : public GapResolver::Assembler {
       int start_source_position, JumpOptimizationInfo* jump_opt,
       PoisoningMitigationLevel poisoning_level, const AssemblerOptions& options,
       Builtin builtin, size_t max_unoptimized_frame_height,
-      size_t max_pushed_argument_count, std::unique_ptr<AssemblerBuffer> = {},
-      const char* debug_name = nullptr);
+      size_t max_pushed_argument_count, const char* debug_name = nullptr);
 
   // Generate native code. After calling AssembleCode, call FinalizeCode to
   // produce the actual code object. If an error occurs during either phase,

src/compiler/pipeline.cc

@@ -546,15 +546,14 @@ class PipelineData {
     start_source_position_ = position;
   }
 
-  void InitializeCodeGenerator(Linkage* linkage,
-                               std::unique_ptr<AssemblerBuffer> buffer) {
+  void InitializeCodeGenerator(Linkage* linkage) {
     DCHECK_NULL(code_generator_);
     code_generator_ = new CodeGenerator(
         codegen_zone(), frame(), linkage, sequence(), info(), isolate(),
         osr_helper_, start_source_position_, jump_optimization_info_,
         info()->GetPoisoningMitigationLevel(), assembler_options_,
         info_->builtin(), max_unoptimized_frame_height(),
-        max_pushed_argument_count(), std::move(buffer),
+        max_pushed_argument_count(),
         FLAG_trace_turbo_stack_accesses ? debug_name_.get() : nullptr);
   }
 
@@ -704,8 +703,7 @@ class PipelineImpl final {
   bool SelectInstructions(Linkage* linkage);
 
   // Step C. Run the code assembly pass.
-  void AssembleCode(Linkage* linkage,
-                    std::unique_ptr<AssemblerBuffer> buffer = {});
+  void AssembleCode(Linkage* linkage);
 
   // Step D. Run the code finalization pass.
   MaybeHandle<Code> FinalizeCode(bool retire_broker = true);
@@ -3155,11 +3153,6 @@ wasm::WasmCompilationResult Pipeline::GenerateCodeForWasmNativeStub(
   wasm::WasmEngine* wasm_engine = wasm::GetWasmEngine();
   ZoneStats zone_stats(wasm_engine->allocator());
   NodeOriginTable* node_positions = graph->zone()->New<NodeOriginTable>(graph);
-  // {instruction_buffer} must live longer than {PipelineData}, since
-  // {PipelineData} will reference the {instruction_buffer} via the
-  // {AssemblerBuffer} of the {Assembler} contained in the {CodeGenerator}.
-  std::unique_ptr<wasm::WasmInstructionBuffer> instruction_buffer =
-      wasm::WasmInstructionBuffer::New();
   PipelineData data(&zone_stats, wasm_engine, &info, mcgraph, nullptr,
                     source_positions, node_positions, options);
   std::unique_ptr<PipelineStatistics> pipeline_statistics;
@@ -3200,14 +3193,14 @@ wasm::WasmCompilationResult Pipeline::GenerateCodeForWasmNativeStub(
 
   Linkage linkage(call_descriptor);
   CHECK(pipeline.SelectInstructions(&linkage));
-  pipeline.AssembleCode(&linkage, instruction_buffer->CreateView());
+  pipeline.AssembleCode(&linkage);
 
   CodeGenerator* code_generator = pipeline.code_generator();
   wasm::WasmCompilationResult result;
   code_generator->tasm()->GetCode(
       nullptr, &result.code_desc, code_generator->safepoint_table_builder(),
       static_cast<int>(code_generator->GetHandlerTableOffset()));
-  result.instr_buffer = instruction_buffer->ReleaseBuffer();
+  result.instr_buffer = code_generator->tasm()->ReleaseBuffer();
   result.source_positions = code_generator->GetSourcePositionTable();
   result.protected_instructions_data =
       code_generator->GetProtectedInstructionsData();
@@ -3258,11 +3251,6 @@ void Pipeline::GenerateCodeForWasmFunction(
   ZoneStats zone_stats(wasm_engine->allocator());
   std::unique_ptr<PipelineStatistics> pipeline_statistics(
       CreatePipelineStatistics(function_body, module, info, &zone_stats));
-  // {instruction_buffer} must live longer than {PipelineData}, since
-  // {PipelineData} will reference the {instruction_buffer} via the
-  // {AssemblerBuffer} of the {Assembler} contained in the {CodeGenerator}.
-  std::unique_ptr<wasm::WasmInstructionBuffer> instruction_buffer =
-      wasm::WasmInstructionBuffer::New();
   PipelineData data(&zone_stats, wasm_engine, info, mcgraph,
                     pipeline_statistics.get(), source_positions, node_origins,
                     WasmAssemblerOptions());
@@ -3310,7 +3298,7 @@ void Pipeline::GenerateCodeForWasmFunction(
 
   Linkage linkage(call_descriptor);
   if (!pipeline.SelectInstructions(&linkage)) return;
-  pipeline.AssembleCode(&linkage, instruction_buffer->CreateView());
+  pipeline.AssembleCode(&linkage);
 
   auto result = std::make_unique<wasm::WasmCompilationResult>();
   CodeGenerator* code_generator = pipeline.code_generator();
@@ -3318,7 +3306,7 @@ void Pipeline::GenerateCodeForWasmFunction(
       nullptr, &result->code_desc, code_generator->safepoint_table_builder(),
       static_cast<int>(code_generator->GetHandlerTableOffset()));
 
-  result->instr_buffer = instruction_buffer->ReleaseBuffer();
+  result->instr_buffer = code_generator->tasm()->ReleaseBuffer();
   result->frame_slot_count = code_generator->frame()->GetTotalFrameSlotCount();
   result->tagged_parameter_slots = call_descriptor->GetTaggedParameterSlots();
   result->source_positions = code_generator->GetSourcePositionTable();
@@ -3713,11 +3701,10 @@ std::ostream& operator<<(std::ostream& out,
   return out;
 }
 
-void PipelineImpl::AssembleCode(Linkage* linkage,
-                                std::unique_ptr<AssemblerBuffer> buffer) {
+void PipelineImpl::AssembleCode(Linkage* linkage) {
   PipelineData* data = this->data_;
   data->BeginPhaseKind("V8.TFCodeGeneration");
-  data->InitializeCodeGenerator(linkage, std::move(buffer));
+  data->InitializeCodeGenerator(linkage);
 
   UnparkedScopeIfNeeded unparked_scope(data->broker(), FLAG_code_comments);
 

src/wasm/baseline/liftoff-compiler.cc

@@ -491,6 +491,10 @@ class LiftoffCompiler {
                  handler_table_offset_);
   }
 
+  std::unique_ptr<AssemblerBuffer> ReleaseBuffer() {
+    return asm_.ReleaseBuffer();
+  }
+
   base::OwnedVector<uint8_t> GetSourcePositionTable() {
     return source_position_table_builder_.ToSourcePositionTableVector();
   }
@@ -6226,9 +6230,10 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   size_t code_size_estimate =
       WasmCodeManager::EstimateLiftoffCodeSize(func_body_size);
   // Allocate the initial buffer a bit bigger to avoid reallocation during code
-  // generation.
-  std::unique_ptr<wasm::WasmInstructionBuffer> instruction_buffer =
-      wasm::WasmInstructionBuffer::New(128 + code_size_estimate * 4 / 3);
+  // generation. Overflows when casting to int are fine, as we will allocate at
+  // least {AssemblerBase::kMinimalBufferSize} anyway, so in the worst case we
+  // have to grow more often.
+  int initial_buffer_size = static_cast<int>(128 + code_size_estimate * 4 / 3);
   std::unique_ptr<DebugSideTableBuilder> debug_sidetable_builder;
   if (debug_sidetable) {
     debug_sidetable_builder = std::make_unique<DebugSideTableBuilder>();
@@ -6236,7 +6241,7 @@ WasmCompilationResult ExecuteLiftoffCompilation(
   DCHECK_IMPLIES(max_steps, for_debugging == kForDebugging);
   WasmFullDecoder<Decoder::kBooleanValidation, LiftoffCompiler> decoder(
       &zone, env->module, env->enabled_features, detected, func_body,
-      call_descriptor, env, &zone, instruction_buffer->CreateView(),
+      call_descriptor, env, &zone, NewAssemblerBuffer(initial_buffer_size),
       debug_sidetable_builder.get(), for_debugging, func_index, breakpoints,
       dead_breakpoint, max_steps, nondeterminism);
   decoder.Decode();
@@ -6259,7 +6264,7 @@ WasmCompilationResult ExecuteLiftoffCompilation(
 
   WasmCompilationResult result;
   compiler->GetCode(&result.code_desc);
-  result.instr_buffer = instruction_buffer->ReleaseBuffer();
+  result.instr_buffer = compiler->ReleaseBuffer();
   result.source_positions = compiler->GetSourcePositionTable();
   result.protected_instructions_data = compiler->GetProtectedInstructionsData();
   result.frame_slot_count = compiler->GetTotalFrameSlotCountForGC();

src/wasm/function-compiler.cc

@@ -22,96 +22,6 @@ namespace v8 {
 namespace internal {
 namespace wasm {
 
-namespace {
-
-class WasmInstructionBufferImpl {
- public:
-  class View : public AssemblerBuffer {
-   public:
-    View(base::Vector<uint8_t> buffer, WasmInstructionBufferImpl* holder)
-        : buffer_(buffer), holder_(holder) {}
-
-    ~View() override {
-      if (buffer_.begin() == holder_->old_buffer_.start()) {
-        DCHECK_EQ(buffer_.size(), holder_->old_buffer_.size());
-        holder_->old_buffer_ = {};
-      }
-    }
-
-    byte* start() const override { return buffer_.begin(); }
-
-    int size() const override { return static_cast<int>(buffer_.size()); }
-
-    std::unique_ptr<AssemblerBuffer> Grow(int new_size) override {
-      // If we grow, we must be the current buffer of {holder_}.
-      DCHECK_EQ(buffer_.begin(), holder_->buffer_.start());
-      DCHECK_EQ(buffer_.size(), holder_->buffer_.size());
-      DCHECK_NULL(holder_->old_buffer_);
-
-      DCHECK_LT(size(), new_size);
-
-      holder_->old_buffer_ = std::move(holder_->buffer_);
-      holder_->buffer_ = base::OwnedVector<uint8_t>::NewForOverwrite(new_size);
-      return std::make_unique<View>(holder_->buffer_.as_vector(), holder_);
-    }
-
-   private:
-    const base::Vector<uint8_t> buffer_;
-    WasmInstructionBufferImpl* const holder_;
-  };
-
-  explicit WasmInstructionBufferImpl(size_t size)
-      : buffer_(base::OwnedVector<uint8_t>::NewForOverwrite(size)) {}
-
-  std::unique_ptr<AssemblerBuffer> CreateView() {
-    DCHECK_NOT_NULL(buffer_);
-    return std::make_unique<View>(buffer_.as_vector(), this);
-  }
-
-  std::unique_ptr<uint8_t[]> ReleaseBuffer() {
-    DCHECK_NULL(old_buffer_);
-    DCHECK_NOT_NULL(buffer_);
-    return buffer_.ReleaseData();
-  }
-
-  bool released() const { return buffer_ == nullptr; }
-
- private:
-  // The current buffer used to emit code.
-  base::OwnedVector<uint8_t> buffer_;
-
-  // While the buffer is grown, we need to temporarily also keep the old buffer
-  // alive.
-  base::OwnedVector<uint8_t> old_buffer_;
-};
-
-WasmInstructionBufferImpl* Impl(WasmInstructionBuffer* buf) {
-  return reinterpret_cast<WasmInstructionBufferImpl*>(buf);
-}
-
-}  // namespace
-
-// PIMPL interface WasmInstructionBuffer for WasmInstBufferImpl
-WasmInstructionBuffer::~WasmInstructionBuffer() {
-  Impl(this)->~WasmInstructionBufferImpl();
-}
-
-std::unique_ptr<AssemblerBuffer> WasmInstructionBuffer::CreateView() {
-  return Impl(this)->CreateView();
-}
-
-std::unique_ptr<uint8_t[]> WasmInstructionBuffer::ReleaseBuffer() {
-  return Impl(this)->ReleaseBuffer();
-}
-
-// static
-std::unique_ptr<WasmInstructionBuffer> WasmInstructionBuffer::New(size_t size) {
-  return std::unique_ptr<WasmInstructionBuffer>{
-      reinterpret_cast<WasmInstructionBuffer*>(new WasmInstructionBufferImpl(
-          std::max(size_t{AssemblerBase::kMinimalBufferSize}, size)))};
-}
-// End of PIMPL interface WasmInstructionBuffer for WasmInstBufferImpl
-
 // static
 ExecutionTier WasmCompilationUnit::GetBaselineExecutionTier(
     const WasmModule* module) {

src/wasm/function-compiler.h

@@ -11,6 +11,7 @@
 
 #include <memory>
 
+#include "src/codegen/assembler.h"
 #include "src/codegen/code-desc.h"
 #include "src/trap-handler/trap-handler.h"
 #include "src/wasm/compilation-environment.h"
@@ -22,7 +23,6 @@
 namespace v8 {
 namespace internal {
 
-class AssemblerBuffer;
 class Counters;
 class OptimizedCompilationJob;
 
@@ -33,24 +33,6 @@ class WasmCode;
 class WasmEngine;
 struct WasmFunction;
 
-class WasmInstructionBuffer final {
- public:
-  WasmInstructionBuffer() = delete;
-  WasmInstructionBuffer(const WasmInstructionBuffer&) = delete;
-  WasmInstructionBuffer& operator=(const WasmInstructionBuffer&) = delete;
-  ~WasmInstructionBuffer();
-  std::unique_ptr<AssemblerBuffer> CreateView();
-  std::unique_ptr<uint8_t[]> ReleaseBuffer();
-
-  // Allocate a new {WasmInstructionBuffer}. The size is the maximum of {size}
-  // and {AssemblerBase::kMinimalSize}.
-  static std::unique_ptr<WasmInstructionBuffer> New(size_t size = 0);
-
-  // Override {operator delete} to avoid implicit instantiation of {operator
-  // delete} with {size_t} argument. The {size_t} argument would be incorrect.
-  void operator delete(void* ptr) { ::operator delete(ptr); }
-};
-
 struct WasmCompilationResult {
  public:
   MOVE_ONLY_WITH_DEFAULT_CONSTRUCTORS(WasmCompilationResult);
@@ -65,7 +47,7 @@ struct WasmCompilationResult {
   operator bool() const { return succeeded(); }
 
   CodeDesc code_desc;
-  std::unique_ptr<uint8_t[]> instr_buffer;
+  std::unique_ptr<AssemblerBuffer> instr_buffer;
   uint32_t frame_slot_count = 0;
   uint32_t tagged_parameter_slots = 0;
   base::OwnedVector<byte> source_positions;

src/wasm/wasm-code-manager.cc

@@ -2099,7 +2099,7 @@ std::vector<std::unique_ptr<WasmCode>> NativeModule::AddCompiledCode(
   // {allocator_mutex_} if we try to switch for each code individually.
   CodeSpaceWriteScope code_space_write_scope(this);
   for (auto& result : results) {
-    DCHECK_EQ(result.code_desc.buffer, result.instr_buffer.get());
+    DCHECK_EQ(result.code_desc.buffer, result.instr_buffer->start());
     size_t code_size = RoundUp<kCodeAlignment>(result.code_desc.instr_size);
     base::Vector<byte> this_code_space = code_space.SubVector(0, code_size);
     code_space += code_size;