Revert "[codegen][frames] Generalize argument padding slot code"

This reverts commit 8cf4eec7.

Reason for revert: Rolling back to previous greedy slot allocator.

tbr=neis@chromium.org,jgruber@chromium.org

Original change's description:
> [codegen][frames] Generalize argument padding slot code
>
> - Removes kPadArguments boolean.
> - Changes ShouldPadArguments to ArgumentPaddingSlots to reflect
>   that on some architectures more than 1 padding slot may be needed.
> - Adds AddArgumentPaddingSlots and ShouldPadArguments convenience
>   functions.
>
> Bug: v8:9198
>
> Change-Id: Iba87518e071a75fb951b490d3f75a87ca715cc23
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2679109
> Commit-Queue: Bill Budge <bbudge@chromium.org>
> Reviewed-by: Georg Neis <neis@chromium.org>
> Reviewed-by: Jakob Gruber <jgruber@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#72605}

Bug: v8:9198
Change-Id: Ie93d32d4b93c67840e4792acb017f28a826bd030
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2713205
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72931}

BUILD.gn

@@ -2652,6 +2652,7 @@ v8_source_set("v8_base_without_compiler") {
     "src/codegen/register-arch.h",
     "src/codegen/register-configuration.cc",
     "src/codegen/register-configuration.h",
+    "src/codegen/register.cc",
     "src/codegen/register.h",
     "src/codegen/reglist.h",
     "src/codegen/reloc-info.cc",

src/codegen/arm/register-arm.h

@@ -119,12 +119,7 @@ GENERAL_REGISTERS(DECLARE_REGISTER)
 #undef DECLARE_REGISTER
 constexpr Register no_reg = Register::no_reg();
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = false;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/arm64/register-arm64.h

@@ -241,14 +241,7 @@ class Register : public CPURegister {
 
 ASSERT_TRIVIALLY_COPYABLE(Register);
 
-// Stack frame alignment and padding.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // Stack frames are aligned to 16 bytes.
-  constexpr int kStackFrameAlignment = 16;
-  constexpr int alignment_mask = kStackFrameAlignment / kSystemPointerSize - 1;
-  return argument_count & alignment_mask;
-}
-
+constexpr bool kPadArguments = true;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/ia32/register-ia32.h

@@ -76,12 +76,7 @@ GENERAL_REGISTERS(DEFINE_REGISTER)
 #undef DEFINE_REGISTER
 constexpr Register no_reg = Register::no_reg();
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/mips/register-mips.h

@@ -203,12 +203,7 @@ int ToNumber(Register reg);
 
 Register ToRegister(int num);
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/mips64/register-mips64.h

@@ -203,12 +203,7 @@ int ToNumber(Register reg);
 
 Register ToRegister(int num);
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/ppc/register-ppc.h

@@ -213,12 +213,7 @@ constexpr Register kConstantPoolRegister = r28;  // Constant pool.
 constexpr Register kRootRegister = r29;          // Roots array pointer.
 constexpr Register cp = r30;                     // JavaScript context pointer.
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/register-arch.h

@@ -30,18 +30,4 @@
 #error Unknown architecture.
 #endif
 
-namespace v8 {
-namespace internal {
-
-constexpr int AddArgumentPaddingSlots(int argument_count) {
-  return argument_count + ArgumentPaddingSlots(argument_count);
-}
-
-constexpr bool ShouldPadArguments(int argument_count) {
-  return ArgumentPaddingSlots(argument_count) != 0;
-}
-
-}  // namespace internal
-}  // namespace v8
-
 #endif  // V8_CODEGEN_REGISTER_ARCH_H_

src/codegen/register.cc

+// Copyright 2019 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/codegen/register.h"
+#include "src/codegen/register-arch.h"
+
+namespace v8 {
+namespace internal {
+
+bool ShouldPadArguments(int argument_count) {
+  return kPadArguments && (argument_count % 2 != 0);
+}
+
+}  // namespace internal
+}  // namespace v8

src/codegen/register.h

@@ -70,6 +70,9 @@ class RegisterBase {
   int reg_code_;
 };
 
+// Whether padding is needed for the given stack argument count.
+bool ShouldPadArguments(int argument_count);
+
 template <typename RegType,
           typename = decltype(RegisterName(std::declval<RegType>()))>
 inline std::ostream& operator<<(std::ostream& os, RegType reg) {

src/codegen/s390/register-s390.h

@@ -167,12 +167,7 @@ constexpr Register no_reg = Register::no_reg();
 constexpr Register kRootRegister = r10;  // Roots array pointer.
 constexpr Register cp = r13;             // JavaScript context pointer.
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/codegen/x64/register-x64.h

@@ -146,12 +146,7 @@ constexpr Register arg_reg_4 = rcx;
   V(xmm13)                              \
   V(xmm14)
 
-// Returns the number of padding slots needed for stack pointer alignment.
-constexpr int ArgumentPaddingSlots(int argument_count) {
-  // No argument padding required.
-  return 0;
-}
-
+constexpr bool kPadArguments = false;
 constexpr bool kSimpleFPAliasing = true;
 constexpr bool kSimdMaskRegisters = false;
 

src/compiler/linkage.cc

@@ -85,12 +85,22 @@ int CallDescriptor::GetStackParameterDelta(
   // inputs to the TailCall node, since they already exist on the stack.
   if (IsTailCallForTierUp()) return 0;
 
-  // Add padding if necessary before computing the stack parameter delta.
-  int callee_slots_above_sp = AddArgumentPaddingSlots(GetOffsetToReturns());
-  int tail_caller_slots_above_sp =
-      AddArgumentPaddingSlots(tail_caller->GetOffsetToReturns());
+  int callee_slots_above_sp = GetOffsetToReturns();
+  int tail_caller_slots_above_sp = tail_caller->GetOffsetToReturns();
   int stack_param_delta = callee_slots_above_sp - tail_caller_slots_above_sp;
-  DCHECK(!ShouldPadArguments(stack_param_delta));
+  if (ShouldPadArguments(stack_param_delta)) {
+    if (callee_slots_above_sp % 2 != 0) {
+      // The delta is odd due to the callee - we will need to add one slot
+      // of padding.
+      ++stack_param_delta;
+    } else {
+      DCHECK_NE(tail_caller_slots_above_sp % 2, 0);
+      // The delta is odd because of the caller. We already have one slot of
+      // padding that we can reuse for arguments, so we will need one fewer
+      // slot.
+      --stack_param_delta;
+    }
+  }
   return stack_param_delta;
 }
 
@@ -127,7 +137,7 @@ int CallDescriptor::GetOffsetToReturns() const {
   // Otherwise, return the first unused slot before the parameters, with any
   // additional padding slot if it exists.
   end_of_returns = GetFirstUnusedStackSlot();
-  end_of_returns += ArgumentPaddingSlots(end_of_returns);
+  if (ShouldPadArguments(end_of_returns)) end_of_returns++;
 
   DCHECK_EQ(end_of_returns == 0, StackParameterCount() == 0);
   return end_of_returns;

src/compiler/wasm-compiler.cc

@@ -8233,7 +8233,8 @@ CallDescriptor* GetWasmCallDescriptor(
         kJSFunctionRegister.code(), MachineType::TaggedPointer()));
   }
 
-  int parameter_slots = AddArgumentPaddingSlots(params.NumStackSlots());
+  int parameter_slots = params.NumStackSlots();
+  if (ShouldPadArguments(parameter_slots)) parameter_slots++;
 
   // Add return location(s).
   LinkageLocationAllocator rets(wasm::kGpReturnRegisters,
@@ -8334,7 +8335,8 @@ CallDescriptor* ReplaceTypeInCallDescriptorWith(
         kJSFunctionRegister.code(), MachineType::TaggedPointer()));
   }
 
-  int parameter_slots = AddArgumentPaddingSlots(params.NumStackSlots());
+  int parameter_slots = params.NumStackSlots();
+  if (ShouldPadArguments(parameter_slots)) parameter_slots++;
 
   LinkageLocationAllocator rets(wasm::kGpReturnRegisters,
                                 wasm::kFpReturnRegisters, parameter_slots);

src/deoptimizer/deoptimizer.cc

@@ -1038,10 +1038,8 @@ void Deoptimizer::DoComputeUnoptimizedFrame(TranslatedFrame* translated_frame,
 
   // Compute the incoming parameter translation.
   ReadOnlyRoots roots(isolate());
-  if (should_pad_arguments) {
-    for (int i = 0; i < ArgumentPaddingSlots(parameters_count); ++i) {
-      frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
-    }
+  if (should_pad_arguments && ShouldPadArguments(parameters_count)) {
+    frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
   }
 
   // Note: parameters_count includes the receiver.
@@ -1193,7 +1191,7 @@ void Deoptimizer::DoComputeUnoptimizedFrame(TranslatedFrame* translated_frame,
 
   // Translate the accumulator register (depending on frame position).
   if (is_topmost) {
-    for (int i = 0; i < ArgumentPaddingSlots(1); ++i) {
+    if (kPadArguments) {
       frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
     }
     // For topmost frame, put the accumulator on the stack. The
@@ -1297,10 +1295,11 @@ void Deoptimizer::DoComputeArgumentsAdaptorFrame(
       argument_count_without_receiver - formal_parameter_count;
   // The number of pushed arguments is the maximum of the actual argument count
   // and the formal parameter count + the receiver.
-  const int padding = ArgumentPaddingSlots(
+  const bool should_pad_args = ShouldPadArguments(
       std::max(argument_count_without_receiver, formal_parameter_count) + 1);
   const int output_frame_size =
-      (std::max(0, extra_argument_count) + padding) * kSystemPointerSize;
+      std::max(0, extra_argument_count * kSystemPointerSize) +
+      (should_pad_args ? kSystemPointerSize : 0);
   if (verbose_tracing_enabled()) {
     PrintF(trace_scope_->file(),
            "  translating arguments adaptor => variable_size=%d\n",
@@ -1323,7 +1322,7 @@ void Deoptimizer::DoComputeArgumentsAdaptorFrame(
   FrameWriter frame_writer(this, output_frame, verbose_trace_scope());
 
   ReadOnlyRoots roots(isolate());
-  for (int i = 0; i < padding; ++i) {
+  if (should_pad_args) {
     frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
   }
 
@@ -1385,7 +1384,7 @@ void Deoptimizer::DoComputeConstructStubFrame(TranslatedFrame* translated_frame,
   output_frame->SetTop(top_address);
 
   ReadOnlyRoots roots(isolate());
-  for (int i = 0; i < ArgumentPaddingSlots(parameters_count); ++i) {
+  if (ShouldPadArguments(parameters_count)) {
     frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
   }
 
@@ -1451,7 +1450,7 @@ void Deoptimizer::DoComputeConstructStubFrame(TranslatedFrame* translated_frame,
   frame_writer.PushTranslatedValue(receiver_iterator, debug_hint);
 
   if (is_topmost) {
-    for (int i = 0; i < ArgumentPaddingSlots(1); ++i) {
+    if (kPadArguments) {
       frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
     }
     // Ensure the result is restored back when we return to the stub.
@@ -1735,8 +1734,7 @@ void Deoptimizer::DoComputeBuiltinContinuation(
   ++value_iterator;
 
   ReadOnlyRoots roots(isolate());
-  const int padding = ArgumentPaddingSlots(frame_info.stack_parameter_count());
-  for (int i = 0; i < padding; ++i) {
+  if (ShouldPadArguments(frame_info.stack_parameter_count())) {
     frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
   }
 
@@ -1891,7 +1889,7 @@ void Deoptimizer::DoComputeBuiltinContinuation(
   }
 
   if (is_topmost) {
-    for (int i = 0; i < ArgumentPaddingSlots(1); ++i) {
+    if (kPadArguments) {
       frame_writer.PushRawObject(roots.the_hole_value(), "padding\n");
     }
 

src/deoptimizer/frame-description.h

@@ -112,9 +112,7 @@ class FrameDescription {
 
   unsigned GetLastArgumentSlotOffset(bool pad_arguments = true) {
     int parameter_slots = parameter_count();
-    if (pad_arguments) {
-      parameter_slots = AddArgumentPaddingSlots(parameter_slots);
-    }
+    if (pad_arguments && ShouldPadArguments(parameter_slots)) parameter_slots++;
     return GetFrameSize() - parameter_slots * kSystemPointerSize;
   }
 

src/execution/frames.cc

@@ -2195,11 +2195,13 @@ InnerPointerToCodeCache::GetCacheEntry(Address inner_pointer) {
 
 namespace {
 
+int ArgumentPaddingSlots(int arg_count) {
+  return ShouldPadArguments(arg_count) ? 1 : 0;
+}
+
 // Some architectures need to push padding together with the TOS register
 // in order to maintain stack alignment.
-constexpr int TopOfStackRegisterPaddingSlots() {
-  return ArgumentPaddingSlots(1);
-}
+constexpr int TopOfStackRegisterPaddingSlots() { return kPadArguments ? 1 : 0; }
 
 bool BuiltinContinuationModeIsWithCatch(BuiltinContinuationMode mode) {
   switch (mode) {

test/unittests/compiler/linkage-tail-call-unittest.cc

@@ -169,8 +169,8 @@ TEST_F(LinkageTailCall, MoreRegisterAndStackParametersCallee) {
   Node* const node = Node::New(zone(), 1, op, 0, nullptr, false);
   EXPECT_TRUE(desc1->CanTailCall(CallDescriptorOf(node->op())));
   int stack_param_delta = desc2->GetStackParameterDelta(desc1);
-  // We might need to add padding slots to the callee arguments.
-  int expected = 1 + ArgumentPaddingSlots(1);
+  // We might need to add one slot of padding to the callee arguments.
+  int expected = kPadArguments ? 2 : 1;
   EXPECT_EQ(expected, stack_param_delta);
 }
 
@@ -192,8 +192,8 @@ TEST_F(LinkageTailCall, MoreRegisterAndStackParametersCaller) {
   Node* const node = Node::New(zone(), 1, op, 0, nullptr, false);
   EXPECT_TRUE(desc1->CanTailCall(CallDescriptorOf(node->op())));
   int stack_param_delta = desc2->GetStackParameterDelta(desc1);
-  // We might need to drop padding slots from the caller's arguments.
-  int expected = -1 - ArgumentPaddingSlots(1);
+  // We might need to drop one slot of padding from the caller's arguments.
+  int expected = kPadArguments ? -2 : -1;
   EXPECT_EQ(expected, stack_param_delta);
 }
 
@@ -329,8 +329,8 @@ TEST_F(LinkageTailCall, MatchingStackParametersExtraCallerRegistersAndStack) {
       Node::New(zone(), 1, op, arraysize(parameters), parameters, false);
   EXPECT_TRUE(desc1->CanTailCall(CallDescriptorOf(node->op())));
   int stack_param_delta = desc2->GetStackParameterDelta(desc1);
-  // We might need to add padding slots to the callee arguments.
-  int expected = ArgumentPaddingSlots(1) - 1;
+  // We might need to add one slot of padding to the callee arguments.
+  int expected = kPadArguments ? 0 : -1;
   EXPECT_EQ(expected, stack_param_delta);
 }
 
@@ -359,8 +359,8 @@ TEST_F(LinkageTailCall, MatchingStackParametersExtraCalleeRegistersAndStack) {
       Node::New(zone(), 1, op, arraysize(parameters), parameters, false);
   EXPECT_TRUE(desc1->CanTailCall(CallDescriptorOf(node->op())));
   int stack_param_delta = desc2->GetStackParameterDelta(desc1);
-  // We might need to drop padding slots from the caller's arguments.
-  int expected = 1 - ArgumentPaddingSlots(1);
+  // We might need to drop one slot of padding from the caller's arguments.
+  int expected = kPadArguments ? 0 : 1;
   EXPECT_EQ(expected, stack_param_delta);
 }
 

test/unittests/wasm/wasm-compiler-unittest.cc

@@ -97,7 +97,7 @@ TEST_F(WasmCallDescriptorTest, Regress_1174500) {
 
   // The stack return slot should be right above our last parameter, and any
   // argument padding slots. The return slot itself should not be padded.
-  const int padding = ShouldPadArguments(1);
+  const int padding = kPadArguments ? 1 : 0;
   const int first_return_slot = -1 - (padding + 1);
   compiler::LinkageLocation return_location =
       desc->GetReturnLocation(kReturns - 1);