[deoptimizer] More consistent semantics of height for all frame types

Information required for deoptimization is passed from codegen to the
deoptimizer through so-called translations. Translations contain,
among many other things, a 'height' field. It is used during deopts to
calculate the unoptimized frame height (but note that it does not
correspond exactly to the frame height itself - further calculations
on the deopt side are needed to get to the real frame height).

The height field has roughly the following data flow:

1. During codegen, we serialize whatever
FrameStateDescriptor::GetHeight() returns.
2. During deopts, serialized translations are converted into
TranslatedFrame objects in TranslatedState::CreateNextTranslatedFrame.
3. These are later used to arrive at the real frame height in multiple
spots, e.g. in DoComputeInterpretedFrame and friends.

Prior to this CL, we were adding and subtracting 1 in basically random
spots. For example, for interpreted and construct stub frames we added
1 in step 1 and subtracted 1 in step 3. For continuation frames, we
added 1 in step 2 and subtracted it in step 3. Argument adaptor frames
were left untouched.

This CL removes all these +-1's. The height field now contains
locals_count() for interpreted frames, and parameters_count() for
everything else. I also tried to make the meaning of adds/subs clearer
through use of named constants like kTheReceiver.

Bug: v8:9534
Change-Id: I6fd26886ff5aa63930f413d879d5480578d9dc7e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1751724Reviewed-by: Sigurd Schneider <sigurds@chromium.org>
Reviewed-by: Michael Stanton <mvstanton@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Auto-Submit: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#63238}

src/compiler/backend/instruction.cc

@@ -1019,23 +1019,22 @@ FrameStateDescriptor::FrameStateDescriptor(
       outer_state_(outer_state) {}
 
 size_t FrameStateDescriptor::GetHeight() const {
-  // TODO(jgruber): Unify how the height is handled. Currently, we seem to add
-  // and subtract 1 at random spots. For example, for interpreted functions we
-  // add 1 here, and subtract it in DoComputeInterpretedFrame. For builtin
-  // continuation frames, we add 1 in CreateNextTranslatedFrame, and subtract
-  // it in DoComputeBuiltinContinuation. Arguments adaptor frames thread through
-  // the height unmodified.
-  // Handling in all these cases should ideally be consistent and use named
-  // constants.
   switch (type()) {
     case FrameStateType::kInterpretedFunction:
-      return locals_count() + 1;  // +1 for the accumulator.
-    case FrameStateType::kConstructStub:
-      return parameters_count() + 1;  // +1 for the context.
-    case FrameStateType::kArgumentsAdaptor:
+      return locals_count();  // The accumulator is *not* included.
     case FrameStateType::kBuiltinContinuation:
+      // Custom, non-JS calling convention (that does not have a notion of
+      // a receiver or context).
+      return parameters_count();
+    case FrameStateType::kArgumentsAdaptor:
+    case FrameStateType::kConstructStub:
     case FrameStateType::kJavaScriptBuiltinContinuation:
     case FrameStateType::kJavaScriptBuiltinContinuationWithCatch:
+      // JS linkage. The parameters count
+      // - includes the receiver (input 1 in CreateArtificialFrameState, and
+      //   passed as part of stack parameters to
+      //   CreateJavaScriptBuiltinContinuationFrameState), and
+      // - does *not* include the context.
       return parameters_count();
   }
   UNREACHABLE();

src/compiler/frame-states.cc

@@ -190,11 +190,13 @@ Node* CreateJavaScriptBuiltinContinuationFrameState(
     actual_parameters.push_back(stack_parameters[i]);
   }
 
-  // Register parameters follow stack paraemters. The context will be added by
+  Node* new_target = jsgraph->UndefinedConstant();
+
+  // Register parameters follow stack parameters. The context will be added by
   // instruction selector during FrameState translation.
-  actual_parameters.push_back(target);
-  actual_parameters.push_back(jsgraph->UndefinedConstant());
-  actual_parameters.push_back(argc);
+  actual_parameters.push_back(target);      // kJavaScriptCallTargetRegister
+  actual_parameters.push_back(new_target);  // kJavaScriptCallNewTargetRegister
+  actual_parameters.push_back(argc);        // kJavaScriptCallArgCountRegister
 
   return CreateBuiltinContinuationFrameStateCommon(
       jsgraph,

src/compiler/js-call-reducer.cc

@@ -5655,10 +5655,14 @@ Node* JSCallReducer::CreateArtificialFrameState(
       bailout_id, OutputFrameStateCombine::Ignore(), state_info);
   const Operator* op0 = common()->StateValues(0, SparseInputMask::Dense());
   Node* node0 = graph()->NewNode(op0);
+
+  static constexpr int kTargetInputIndex = 0;
+  static constexpr int kReceiverInputIndex = 1;
+  const int parameter_count_with_receiver = parameter_count + 1;
   std::vector<Node*> params;
-  params.reserve(parameter_count + 1);
-  for (int parameter = 0; parameter < parameter_count + 1; ++parameter) {
-    params.push_back(node->InputAt(1 + parameter));
+  params.reserve(parameter_count_with_receiver);
+  for (int i = 0; i < parameter_count_with_receiver; i++) {
+    params.push_back(node->InputAt(kReceiverInputIndex + i));
   }
   const Operator* op_param = common()->StateValues(
       static_cast<int>(params.size()), SparseInputMask::Dense());
@@ -5668,7 +5672,7 @@ Node* JSCallReducer::CreateArtificialFrameState(
     context = jsgraph()->UndefinedConstant();
   }
   return graph()->NewNode(op, params_node, node0, node0, context,
-                          node->InputAt(0), outer_frame_state);
+                          node->InputAt(kTargetInputIndex), outer_frame_state);
 }
 
 Reduction JSCallReducer::ReducePromiseConstructor(Node* node) {

src/compiler/js-inlining.cc

@@ -247,9 +247,13 @@ Node* JSInliner::CreateArtificialFrameState(Node* node, Node* outer_frame_state,
       bailout_id, OutputFrameStateCombine::Ignore(), state_info);
   const Operator* op0 = common()->StateValues(0, SparseInputMask::Dense());
   Node* node0 = graph()->NewNode(op0);
+
+  static constexpr int kTargetInputIndex = 0;
+  static constexpr int kReceiverInputIndex = 1;
+  const int parameter_count_with_receiver = parameter_count + 1;
   NodeVector params(local_zone_);
-  for (int parameter = 0; parameter < parameter_count + 1; ++parameter) {
-    params.push_back(node->InputAt(1 + parameter));
+  for (int i = 0; i < parameter_count_with_receiver; i++) {
+    params.push_back(node->InputAt(kReceiverInputIndex + i));
   }
   const Operator* op_param = common()->StateValues(
       static_cast<int>(params.size()), SparseInputMask::Dense());
@@ -259,7 +263,7 @@ Node* JSInliner::CreateArtificialFrameState(Node* node, Node* outer_frame_state,
     context = jsgraph()->UndefinedConstant();
   }
   return graph()->NewNode(op, params_node, node0, node0, context,
-                          node->InputAt(0), outer_frame_state);
+                          node->InputAt(kTargetInputIndex), outer_frame_state);
 }
 
 namespace {

src/deoptimizer/deoptimizer.cc

@@ -452,6 +452,15 @@ const char* Deoptimizer::MessageFor(DeoptimizeKind kind) {
   return nullptr;
 }
 
+namespace {
+
+uint16_t InternalFormalParameterCountWithReceiver(SharedFunctionInfo sfi) {
+  static constexpr int kTheReceiver = 1;
+  return sfi.internal_formal_parameter_count() + kTheReceiver;
+}
+
+}  // namespace
+
 Deoptimizer::Deoptimizer(Isolate* isolate, JSFunction function,
                          DeoptimizeKind kind, unsigned bailout_id, Address from,
                          int fp_to_sp_delta)
@@ -503,7 +512,8 @@ Deoptimizer::Deoptimizer(Isolate* isolate, JSFunction function,
             CodeDeoptEvent(compiled_code_, kind, from_, fp_to_sp_delta_));
   }
   unsigned size = ComputeInputFrameSize();
-  int parameter_count = function.shared().internal_formal_parameter_count() + 1;
+  int parameter_count =
+      InternalFormalParameterCountWithReceiver(function.shared());
   input_ = new (size) FrameDescription(size, parameter_count);
 
   if (kSupportsFixedDeoptExitSize) {
@@ -804,9 +814,9 @@ void Deoptimizer::DoComputeInterpretedFrame(TranslatedFrame* translated_frame,
   bool is_topmost = (output_count_ - 1 == frame_index);
 
   int bytecode_offset = translated_frame->node_id().ToInt();
-  int height = translated_frame->height();
-  int register_count = height - 1;  // Exclude accumulator.
-  int register_stack_slot_count =
+  const int height = translated_frame->height();
+  const int register_count = height;  // The accumulator is *not* included.
+  const int register_stack_slot_count =
       InterpreterFrameConstants::RegisterStackSlotCount(register_count);
   int height_in_bytes = register_stack_slot_count * kSystemPointerSize;
 
@@ -836,7 +846,7 @@ void Deoptimizer::DoComputeInterpretedFrame(TranslatedFrame* translated_frame,
   unsigned output_frame_size = height_in_bytes + fixed_frame_size;
 
   // Allocate and store the output frame description.
-  int parameter_count = shared.internal_formal_parameter_count() + 1;
+  const int parameter_count = InternalFormalParameterCountWithReceiver(shared);
   FrameDescription* output_frame = new (output_frame_size)
       FrameDescription(output_frame_size, parameter_count);
   FrameWriter frame_writer(this, output_frame, trace_scope_);
@@ -1072,8 +1082,9 @@ void Deoptimizer::DoComputeArgumentsAdaptorFrame(
   unsigned height = translated_frame->height();
   unsigned height_in_bytes = height * kSystemPointerSize;
   int parameter_count = height;
-  if (ShouldPadArguments(parameter_count))
+  if (ShouldPadArguments(parameter_count)) {
     height_in_bytes += kSystemPointerSize;
+  }
 
   TranslatedFrame::iterator function_iterator = value_iterator++;
   if (trace_scope_ != nullptr) {
@@ -1180,7 +1191,7 @@ void Deoptimizer::DoComputeConstructStubFrame(TranslatedFrame* translated_frame,
   Code construct_stub = builtins->builtin(Builtins::kJSConstructStubGeneric);
   BailoutId bailout_id = translated_frame->node_id();
   unsigned height = translated_frame->height();
-  unsigned parameter_count = height - 1;  // Exclude the context.
+  unsigned parameter_count = height;  // The context is *not* included.
   unsigned height_in_bytes = parameter_count * kSystemPointerSize;
 
   // If the construct frame appears to be topmost we should ensure that the
@@ -1193,8 +1204,9 @@ void Deoptimizer::DoComputeConstructStubFrame(TranslatedFrame* translated_frame,
     if (PadTopOfStackRegister()) height_in_bytes += kSystemPointerSize;
   }
 
-  if (ShouldPadArguments(parameter_count))
+  if (ShouldPadArguments(parameter_count)) {
     height_in_bytes += kSystemPointerSize;
+  }
 
   TranslatedFrame::iterator function_iterator = value_iterator++;
   if (trace_scope_ != nullptr) {
@@ -1469,7 +1481,7 @@ void Deoptimizer::DoComputeBuiltinContinuation(
 
   // The output frame must have room for all of the parameters that need to be
   // passed to the builtin continuation.
-  const int height_in_words = translated_frame->height();
+  const int height = translated_frame->height();  // Context *not* included.
 
   BailoutId bailout_id = translated_frame->node_id();
   Builtins::Name builtin_name = Builtins::GetBuiltinFromBailoutId(bailout_id);
@@ -1493,10 +1505,7 @@ void Deoptimizer::DoComputeBuiltinContinuation(
 
   const int register_parameter_count =
       continuation_descriptor.GetRegisterParameterCount();
-  // Make sure to account for the context by removing it from the register
-  // parameter count.
-  const int translated_stack_parameters =
-      height_in_words - register_parameter_count - 1;
+  const int translated_stack_parameters = height - register_parameter_count;
   const int stack_param_count =
       translated_stack_parameters + (must_handle_result ? 1 : 0) +
       (BuiltinContinuationModeIsWithCatch(mode) ? 1 : 0);
@@ -1823,7 +1832,7 @@ unsigned Deoptimizer::ComputeInterpretedFixedSize(SharedFunctionInfo shared) {
 
 // static
 unsigned Deoptimizer::ComputeIncomingArgumentSize(SharedFunctionInfo shared) {
-  int parameter_slots = shared.internal_formal_parameter_count() + 1;
+  int parameter_slots = InternalFormalParameterCountWithReceiver(shared);
   if (kPadArguments) parameter_slots = RoundUp(parameter_slots, 2);
   return parameter_slots * kSystemPointerSize;
 }
@@ -2267,12 +2276,9 @@ DeoptimizedFrameInfo::DeoptimizedFrameInfo(TranslatedState* state,
   stack_it++;
 
   // Get the expression stack.
-  int stack_height = frame_it->height();
-  if (frame_it->kind() == TranslatedFrame::kInterpretedFunction) {
-    // For interpreter frames, we should not count the accumulator.
-    // TODO(jarin): Clean up the indexing in translated frames.
-    stack_height--;
-  }
+  DCHECK_EQ(TranslatedFrame::kInterpretedFunction, frame_it->kind());
+  const int stack_height = frame_it->height();  // Accumulator *not* included.
+
   expression_stack_.resize(static_cast<size_t>(stack_height));
   for (int i = 0; i < stack_height; i++) {
     Handle<Object> expression = GetValueForDebugger(stack_it, isolate);
@@ -2280,10 +2286,9 @@ DeoptimizedFrameInfo::DeoptimizedFrameInfo(TranslatedState* state,
     stack_it++;
   }
 
-  // For interpreter frame, skip the accumulator.
-  if (frame_it->kind() == TranslatedFrame::kInterpretedFunction) {
-    stack_it++;
-  }
+  DCHECK_EQ(TranslatedFrame::kInterpretedFunction, frame_it->kind());
+  stack_it++;  // Skip the accumulator.
+
   CHECK(stack_it == frame_it->end());
 }
 
@@ -2701,20 +2706,30 @@ TranslatedFrame TranslatedFrame::JavaScriptBuiltinContinuationWithCatchFrame(
 }
 
 int TranslatedFrame::GetValueCount() {
+  // The function is added to all frame state descriptors in
+  // InstructionSelector::AddInputsToFrameStateDescriptor.
+  static constexpr int kTheFunction = 1;
+
   switch (kind()) {
     case kInterpretedFunction: {
       int parameter_count =
-          raw_shared_info_.internal_formal_parameter_count() + 1;
-      // + 2 for function and context.
-      return height_ + parameter_count + 2;
+          InternalFormalParameterCountWithReceiver(raw_shared_info_);
+      static constexpr int kTheContext = 1;
+      static constexpr int kTheAccumulator = 1;
+      return height() + parameter_count + kTheContext + kTheFunction +
+             kTheAccumulator;
     }
 
     case kArgumentsAdaptor:
+      return height() + kTheFunction;
+
     case kConstructStub:
     case kBuiltinContinuation:
     case kJavaScriptBuiltinContinuation:
-    case kJavaScriptBuiltinContinuationWithCatch:
-      return 1 + height_;
+    case kJavaScriptBuiltinContinuationWithCatch: {
+      static constexpr int kTheContext = 1;
+      return height() + kTheContext + kTheFunction;
+    }
 
     case kInvalid:
       UNREACHABLE();
@@ -2749,7 +2764,7 @@ TranslatedFrame TranslatedState::CreateNextTranslatedFrame(
       if (trace_file != nullptr) {
         std::unique_ptr<char[]> name = shared_info.DebugName().ToCString();
         PrintF(trace_file, "  reading input frame %s", name.get());
-        int arg_count = shared_info.internal_formal_parameter_count() + 1;
+        int arg_count = InternalFormalParameterCountWithReceiver(shared_info);
         PrintF(trace_file,
                " => bytecode_offset=%d, args=%d, height=%d, retval=%i(#%i); "
                "inputs:\n",
@@ -2800,11 +2815,8 @@ TranslatedFrame TranslatedState::CreateNextTranslatedFrame(
         PrintF(trace_file, " => bailout_id=%d, height=%d; inputs:\n",
                bailout_id.ToInt(), height);
       }
-      // Add one to the height to account for the context which was implicitly
-      // added to the translation during code generation.
-      int height_with_context = height + 1;
       return TranslatedFrame::BuiltinContinuationFrame(bailout_id, shared_info,
-                                                       height_with_context);
+                                                       height);
     }
 
     case Translation::JAVA_SCRIPT_BUILTIN_CONTINUATION_FRAME: {
@@ -2819,11 +2831,8 @@ TranslatedFrame TranslatedState::CreateNextTranslatedFrame(
         PrintF(trace_file, " => bailout_id=%d, height=%d; inputs:\n",
                bailout_id.ToInt(), height);
       }
-      // Add one to the height to account for the context which was implicitly
-      // added to the translation during code generation.
-      int height_with_context = height + 1;
       return TranslatedFrame::JavaScriptBuiltinContinuationFrame(
-          bailout_id, shared_info, height_with_context);
+          bailout_id, shared_info, height);
     }
     case Translation::JAVA_SCRIPT_BUILTIN_CONTINUATION_WITH_CATCH_FRAME: {
       BailoutId bailout_id = BailoutId(iterator->Next());
@@ -2838,11 +2847,8 @@ TranslatedFrame TranslatedState::CreateNextTranslatedFrame(
         PrintF(trace_file, " => bailout_id=%d, height=%d; inputs:\n",
                bailout_id.ToInt(), height);
       }
-      // Add one to the height to account for the context which was implicitly
-      // added to the translation during code generation.
-      int height_with_context = height + 1;
       return TranslatedFrame::JavaScriptBuiltinContinuationWithCatchFrame(
-          bailout_id, shared_info, height_with_context);
+          bailout_id, shared_info, height);
     }
     case Translation::UPDATE_FEEDBACK:
     case Translation::BEGIN:
@@ -3931,15 +3937,16 @@ TranslatedFrame* TranslatedState::GetArgumentsInfoFromJSFrameIndex(
           // to last value in the TranslatedFrame. It should also always be
           // {1}, as the GenericLazyDeoptContinuation builtin only has one
           // argument (the receiver).
-          const int height = frames_[i].height();
+          static constexpr int kTheContext = 1;
+          const int height = frames_[i].height() + kTheContext;
           Object argc_object = frames_[i].ValueAt(height - 1)->GetRawValue();
           CHECK(argc_object.IsSmi());
           *args_count = Smi::ToInt(argc_object);
 
           DCHECK_EQ(*args_count, 1);
         } else {
-          *args_count =
-              frames_[i].shared_info()->internal_formal_parameter_count() + 1;
+          *args_count = InternalFormalParameterCountWithReceiver(
+              *frames_[i].shared_info());
         }
         return &(frames_[i]);
       }

src/deoptimizer/deoptimizer.h

@@ -172,7 +172,14 @@ class TranslatedFrame {
   Kind kind() const { return kind_; }
   BailoutId node_id() const { return node_id_; }
   Handle<SharedFunctionInfo> shared_info() const { return shared_info_; }
+
+  // TODO(jgruber): Simplify/clarify the semantics of this field. The name
+  // `height` is slightly misleading. Yes, this value is related to stack frame
+  // height, but must undergo additional mutations to arrive at the real stack
+  // frame height (e.g.: addition/subtraction of context, accumulator, fixed
+  // frame sizes, padding).
   int height() const { return height_; }
+
   int return_value_offset() const { return return_value_offset_; }
   int return_value_count() const { return return_value_count_; }