[TurboFan] Cache Parameter nodes in BytecodeGraphBuilder

Register allocator experienced some issues with multiple nodes for
the same parameter, which occurred in a few cases running turboprop.
This CL adds caching of Parameter nodes in BytecodeGraphBuilder such
that there exists only one node for each parameter index.

Bug: v8:11796
Change-Id: I90be5438f43368510ec4c317fa532c92a446e76a
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2910314
Commit-Queue: Nico Hartmann <nicohartmann@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Manos Koukoutos <manoskouk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74715}

src/compiler/bytecode-graph-builder.cc

@@ -11,6 +11,7 @@
 #include "src/common/assert-scope.h"
 #include "src/compiler/access-builder.h"
 #include "src/compiler/bytecode-analysis.h"
+#include "src/compiler/common-operator.h"
 #include "src/compiler/compiler-source-position-table.h"
 #include "src/compiler/js-heap-broker.h"
 #include "src/compiler/linkage.h"
@@ -70,6 +71,11 @@ class BytecodeGraphBuilder {
   // Get or create the node that represents the outer function closure.
   Node* GetFunctionClosure();
 
+  // Get or create the node for this parameter index. If such a node is
+  // already cached, it is returned directly and the {debug_name_hint} is
+  // ignored.
+  Node* GetParameter(int index, const char* debug_name_hint = nullptr);
+
   CodeKind code_kind() const { return code_kind_; }
   bool native_context_independent() const {
     // TODO(jgruber,v8:8888): Remove dependent code.
@@ -518,6 +524,8 @@ class BytecodeGraphBuilder {
   // the "resuming" ones. They are indexed by the suspend id of the resume.
   ZoneMap<int, Environment*> generator_merge_environments_;
 
+  ZoneVector<Node*> cached_parameters_;
+
   // Exception handlers currently entered by the iteration.
   ZoneStack<ExceptionHandler> exception_handlers_;
   int current_exception_handler_;
@@ -697,8 +705,7 @@ BytecodeGraphBuilder::Environment::Environment(
   // Parameters including the receiver
   for (int i = 0; i < parameter_count; i++) {
     const char* debug_name = (i == 0) ? "%this" : nullptr;
-    const Operator* op = common()->Parameter(i, debug_name);
-    Node* parameter = builder->graph()->NewNode(op, graph()->start());
+    Node* parameter = builder->GetParameter(i, debug_name);
     values()->push_back(parameter);
   }
 
@@ -713,15 +720,14 @@ BytecodeGraphBuilder::Environment::Environment(
 
   // Context
   int context_index = Linkage::GetJSCallContextParamIndex(parameter_count);
-  const Operator* op = common()->Parameter(context_index, "%context");
-  context_ = builder->graph()->NewNode(op, graph()->start());
+  context_ = builder->GetParameter(context_index, "%context");
 
   // Incoming new.target or generator register
   if (incoming_new_target_or_generator.is_valid()) {
     int new_target_index =
         Linkage::GetJSCallNewTargetParamIndex(parameter_count);
-    const Operator* op = common()->Parameter(new_target_index, "%new.target");
-    Node* new_target_node = builder->graph()->NewNode(op, graph()->start());
+    Node* new_target_node =
+        builder->GetParameter(new_target_index, "%new.target");
 
     int values_index = RegisterToValuesIndex(incoming_new_target_or_generator);
     values()->at(values_index) = new_target_node;
@@ -1101,6 +1107,7 @@ BytecodeGraphBuilder::BytecodeGraphBuilder(
                               BytecodeGraphBuilderFlag::kSkipFirstStackCheck),
       merge_environments_(local_zone),
       generator_merge_environments_(local_zone),
+      cached_parameters_(local_zone),
       exception_handlers_(local_zone),
       current_exception_handler_(0),
       input_buffer_size_(0),
@@ -1120,13 +1127,32 @@ BytecodeGraphBuilder::BytecodeGraphBuilder(
 Node* BytecodeGraphBuilder::GetFunctionClosure() {
   if (!function_closure_.is_set()) {
     int index = Linkage::kJSCallClosureParamIndex;
-    const Operator* op = common()->Parameter(index, "%closure");
-    Node* node = NewNode(op, graph()->start());
+    Node* node = GetParameter(index, "%closure");
     function_closure_.set(node);
   }
   return function_closure_.get();
 }
 
+Node* BytecodeGraphBuilder::GetParameter(int parameter_index,
+                                         const char* debug_name_hint) {
+  // We use negative indices for some parameters.
+  DCHECK_LE(ParameterInfo::kMinIndex, parameter_index);
+  const size_t index =
+      static_cast<size_t>(parameter_index - ParameterInfo::kMinIndex);
+
+  if (cached_parameters_.size() <= index) {
+    cached_parameters_.resize(index + 1, nullptr);
+  }
+
+  if (cached_parameters_[index] == nullptr) {
+    cached_parameters_[index] =
+        NewNode(common()->Parameter(parameter_index, debug_name_hint),
+                graph()->start());
+  }
+
+  return cached_parameters_[index];
+}
+
 void BytecodeGraphBuilder::CreateFeedbackCellNode() {
   DCHECK_NULL(feedback_cell_node_);
   if (native_context_independent()) {
@@ -1203,15 +1229,10 @@ void BytecodeGraphBuilder::MaybeBuildTierUpCheck() {
 
   int parameter_count = bytecode_array().parameter_count();
   Node* target = GetFunctionClosure();
-  Node* new_target = graph()->NewNode(
-      common()->Parameter(
-          Linkage::GetJSCallNewTargetParamIndex(parameter_count),
-          "%new.target"),
-      graph()->start());
-  Node* argc = graph()->NewNode(
-      common()->Parameter(Linkage::GetJSCallArgCountParamIndex(parameter_count),
-                          "%argc"),
-      graph()->start());
+  Node* new_target = GetParameter(
+      Linkage::GetJSCallNewTargetParamIndex(parameter_count), "%new.target");
+  Node* argc = GetParameter(
+      Linkage::GetJSCallArgCountParamIndex(parameter_count), "%argc");
   DCHECK_EQ(environment()->Context()->opcode(), IrOpcode::kParameter);
   Node* context = environment()->Context();
 
@@ -4355,6 +4376,11 @@ Node* BytecodeGraphBuilder::MakeNode(const Operator* op, int value_input_count,
                                      Node* const* value_inputs,
                                      bool incomplete) {
   DCHECK_EQ(op->ValueInputCount(), value_input_count);
+  // Parameter nodes must be created through GetParameter.
+  DCHECK_IMPLIES(
+      op->opcode() == IrOpcode::kParameter,
+      (nullptr == cached_parameters_[static_cast<std::size_t>(
+                      ParameterIndexOf(op) - ParameterInfo::kMinIndex)]));
 
   bool has_context = OperatorProperties::HasContextInput(op);
   bool has_frame_state = OperatorProperties::HasFrameStateInput(op);

src/compiler/common-operator.h

@@ -177,8 +177,12 @@ PhiRepresentationOf(const Operator* const) V8_WARN_UNUSED_RESULT;
 // function. This class bundles the index and a debug name for such operators.
 class ParameterInfo final {
  public:
+  static constexpr int kMinIndex = Linkage::kJSCallClosureParamIndex;
+
   ParameterInfo(int index, const char* debug_name)
-      : index_(index), debug_name_(debug_name) {}
+      : index_(index), debug_name_(debug_name) {
+    DCHECK_LE(kMinIndex, index);
+  }
 
   int index() const { return index_; }
   const char* debug_name() const { return debug_name_; }