From b37907ff7f866873ddfbfc97670b43c19a5fc7f9 Mon Sep 17 00:00:00 2001 From: bmeurer Date: Tue, 8 Sep 2015 22:01:01 -0700 Subject: [PATCH] [calls] Consistent call protocol for calls. The number of actual arguments should always be available, there's no point in trying to optimize away a simple assignment of an immediate to a register before some calls. The main motivation is to have a consistent state at the beginning of every function. Currently the arguments register (i.e. rax or eax) either contains the number of arguments or some random garbage depending on whether the callsite decided that the callee might need the information or not. This causes trouble with runtime implementations of functions that do not set internal_formal_parameter_count to the DontAdaptArguments sentinel (we don't have any of those yet), but also makes it impossible to sanity check the arguments in the callee, because the callee doesn't know whether the caller decided to pass the number of arguments or random garbage. BUG=v8:4413 LOG=n Review URL: https://codereview.chromium.org/1330033002 Cr-Commit-Position: refs/heads/master@{#30648} --- src/arm/lithium-codegen-arm.cc | 11 +++-------- src/arm/macro-assembler-arm.cc | 4 ++-- src/arm64/lithium-codegen-arm64.cc | 11 +++-------- src/arm64/macro-assembler-arm64.cc | 5 ++--- src/hydrogen-instructions.cc | 7 ++----- src/hydrogen-instructions.h | 8 +------- src/hydrogen.cc | 10 +++++----- src/hydrogen.h | 4 +--- src/ia32/lithium-codegen-ia32.cc | 11 +++-------- src/ia32/macro-assembler-ia32.cc | 6 ++++-- src/mips/lithium-codegen-mips.cc | 11 +++-------- src/mips/macro-assembler-mips.cc | 4 ++-- src/mips64/lithium-codegen-mips64.cc | 11 +++-------- src/mips64/macro-assembler-mips64.cc | 4 ++-- src/x64/lithium-codegen-x64.cc | 11 +++-------- src/x64/macro-assembler-x64.cc | 6 ++++-- 16 files changed, 43 insertions(+), 81 deletions(-) diff --git a/src/arm/lithium-codegen-arm.cc b/src/arm/lithium-codegen-arm.cc index 6e368de..74f527c 100644 --- a/src/arm/lithium-codegen-arm.cc +++ b/src/arm/lithium-codegen-arm.cc @@ -3464,11 +3464,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ ldr(cp, FieldMemOperand(function_reg, JSFunction::kContextOffset)); - // Set r0 to arguments count if adaption is not needed. Assumes that r0 - // is available to write to at this point. - if (dont_adapt_arguments) { - __ mov(r0, Operand(arity)); - } + // Always initialize r0 to the number of actual arguments. + __ mov(r0, Operand(arity)); // Invoke function. __ ldr(ip, FieldMemOperand(function_reg, JSFunction::kCodeEntryOffset)); @@ -3849,9 +3846,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(ToRegister(instr->function()).is(r1)); DCHECK(ToRegister(instr->result()).is(r0)); - if (instr->hydrogen()->pass_argument_count()) { - __ mov(r0, Operand(instr->arity())); - } + __ mov(r0, Operand(instr->arity())); // Change context. __ ldr(cp, FieldMemOperand(r1, JSFunction::kContextOffset)); diff --git a/src/arm/macro-assembler-arm.cc b/src/arm/macro-assembler-arm.cc index 3f0ff1a..c0b9773 100644 --- a/src/arm/macro-assembler-arm.cc +++ b/src/arm/macro-assembler-arm.cc @@ -1251,10 +1251,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + mov(r0, Operand(actual.immediate())); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - mov(r0, Operand(actual.immediate())); const int sentinel = SharedFunctionInfo::kDontAdaptArgumentsSentinel; if (expected.immediate() == sentinel) { // Don't worry about adapting arguments for builtins that @@ -1269,9 +1269,9 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, } } else { if (actual.is_immediate()) { + mov(r0, Operand(actual.immediate())); cmp(expected.reg(), Operand(actual.immediate())); b(eq, ®ular_invoke); - mov(r0, Operand(actual.immediate())); } else { cmp(expected.reg(), Operand(actual.reg())); b(eq, ®ular_invoke); diff --git a/src/arm64/lithium-codegen-arm64.cc b/src/arm64/lithium-codegen-arm64.cc index 529352d..fe16d06 100644 --- a/src/arm64/lithium-codegen-arm64.cc +++ b/src/arm64/lithium-codegen-arm64.cc @@ -1996,11 +1996,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ Ldr(cp, FieldMemOperand(function_reg, JSFunction::kContextOffset)); - // Set the arguments count if adaption is not needed. Assumes that x0 is - // available to write to at this point. - if (dont_adapt_arguments) { - __ Mov(arity_reg, arity); - } + // Always initialize x0 to the number of actual arguments. + __ Mov(arity_reg, arity); // Invoke function. __ Ldr(x10, FieldMemOperand(function_reg, JSFunction::kCodeEntryOffset)); @@ -2067,9 +2064,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(instr->IsMarkedAsCall()); DCHECK(ToRegister(instr->function()).is(x1)); - if (instr->hydrogen()->pass_argument_count()) { - __ Mov(x0, Operand(instr->arity())); - } + __ Mov(x0, Operand(instr->arity())); // Change context. __ Ldr(cp, FieldMemOperand(x1, JSFunction::kContextOffset)); diff --git a/src/arm64/macro-assembler-arm64.cc b/src/arm64/macro-assembler-arm64.cc index 75814e8..b4870df 100644 --- a/src/arm64/macro-assembler-arm64.cc +++ b/src/arm64/macro-assembler-arm64.cc @@ -2571,11 +2571,11 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + Mov(x0, actual.immediate()); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - Mov(x0, actual.immediate()); if (expected.immediate() == SharedFunctionInfo::kDontAdaptArgumentsSentinel) { // Don't worry about adapting arguments for builtins that @@ -2593,11 +2593,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, } else { // expected is a register. Operand actual_op = actual.is_immediate() ? Operand(actual.immediate()) : Operand(actual.reg()); + Mov(x0, actual_op); // If actual == expected perform a regular invocation. Cmp(expected.reg(), actual_op); B(eq, ®ular_invoke); - // Otherwise set up x0 for the argument adaptor. - Mov(x0, actual_op); } // If the argument counts may mismatch, generate a call to the argument diff --git a/src/hydrogen-instructions.cc b/src/hydrogen-instructions.cc index be0b71b..e9ca099 100644 --- a/src/hydrogen-instructions.cc +++ b/src/hydrogen-instructions.cc @@ -927,8 +927,7 @@ std::ostream& HCallJSFunction::PrintDataTo(std::ostream& os) const { // NOLINT HCallJSFunction* HCallJSFunction::New(Isolate* isolate, Zone* zone, HValue* context, HValue* function, - int argument_count, - bool pass_argument_count) { + int argument_count) { bool has_stack_check = false; if (function->IsConstant()) { HConstant* fun_const = HConstant::cast(function); @@ -939,9 +938,7 @@ HCallJSFunction* HCallJSFunction::New(Isolate* isolate, Zone* zone, jsfun->code()->kind() == Code::OPTIMIZED_FUNCTION); } - return new(zone) HCallJSFunction( - function, argument_count, pass_argument_count, - has_stack_check); + return new (zone) HCallJSFunction(function, argument_count, has_stack_check); } diff --git a/src/hydrogen-instructions.h b/src/hydrogen-instructions.h index e1bca99..9ff8079 100644 --- a/src/hydrogen-instructions.h +++ b/src/hydrogen-instructions.h @@ -2228,8 +2228,7 @@ class HBinaryCall : public HCall<2> { class HCallJSFunction final : public HCall<1> { public: static HCallJSFunction* New(Isolate* isolate, Zone* zone, HValue* context, - HValue* function, int argument_count, - bool pass_argument_count); + HValue* function, int argument_count); HValue* function() const { return OperandAt(0); } @@ -2240,8 +2239,6 @@ class HCallJSFunction final : public HCall<1> { return Representation::Tagged(); } - bool pass_argument_count() const { return pass_argument_count_; } - bool HasStackCheck() final { return has_stack_check_; } DECLARE_CONCRETE_INSTRUCTION(CallJSFunction) @@ -2250,15 +2247,12 @@ class HCallJSFunction final : public HCall<1> { // The argument count includes the receiver. HCallJSFunction(HValue* function, int argument_count, - bool pass_argument_count, bool has_stack_check) : HCall<1>(argument_count), - pass_argument_count_(pass_argument_count), has_stack_check_(has_stack_check) { SetOperandAt(0, function); } - bool pass_argument_count_; bool has_stack_check_; }; diff --git a/src/hydrogen.cc b/src/hydrogen.cc index 40d4c83..bf55f7c 100644 --- a/src/hydrogen.cc +++ b/src/hydrogen.cc @@ -7901,9 +7901,9 @@ void HOptimizedGraphBuilder::AddCheckPrototypeMaps(Handle holder, } -HInstruction* HOptimizedGraphBuilder::NewPlainFunctionCall( - HValue* fun, int argument_count, bool pass_argument_count) { - return New(fun, argument_count, pass_argument_count); +HInstruction* HOptimizedGraphBuilder::NewPlainFunctionCall(HValue* fun, + int argument_count) { + return New(fun, argument_count); } @@ -7941,7 +7941,7 @@ HInstruction* HOptimizedGraphBuilder::BuildCallConstantFunction( if (jsfun.is_identical_to(current_info()->closure())) { graph()->MarkRecursive(); } - return NewPlainFunctionCall(target, argument_count, dont_adapt_arguments); + return NewPlainFunctionCall(target, argument_count); } else { HValue* param_count_value = Add(formal_parameter_count); HValue* context = Add( @@ -8962,7 +8962,7 @@ bool HOptimizedGraphBuilder::TryInlineBuiltinMethodCall( if_inline.Else(); { Add(receiver); - result = Add(function, 1, true); + result = Add(function, 1); if (!ast_context()->IsEffect()) Push(result); } if_inline.End(); diff --git a/src/hydrogen.h b/src/hydrogen.h index 88a9fd8..f60f2cb 100644 --- a/src/hydrogen.h +++ b/src/hydrogen.h @@ -2865,9 +2865,7 @@ class HOptimizedGraphBuilder : public HGraphBuilder, public AstVisitor { void AddCheckPrototypeMaps(Handle holder, Handle receiver_map); - HInstruction* NewPlainFunctionCall(HValue* fun, - int argument_count, - bool pass_argument_count); + HInstruction* NewPlainFunctionCall(HValue* fun, int argument_count); HInstruction* NewArgumentAdaptorCall(HValue* fun, HValue* context, int argument_count, diff --git a/src/ia32/lithium-codegen-ia32.cc b/src/ia32/lithium-codegen-ia32.cc index 7f44b9e..24f6f42 100644 --- a/src/ia32/lithium-codegen-ia32.cc +++ b/src/ia32/lithium-codegen-ia32.cc @@ -3340,11 +3340,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ mov(esi, FieldOperand(function_reg, JSFunction::kContextOffset)); - // Set eax to arguments count if adaption is not needed. Assumes that eax - // is available to write to at this point. - if (dont_adapt_arguments) { - __ mov(eax, arity); - } + // Always initialize eax to the number of actual arguments. + __ mov(eax, arity); // Invoke function directly. if (function.is_identical_to(info()->closure())) { @@ -3406,9 +3403,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(ToRegister(instr->function()).is(edi)); DCHECK(ToRegister(instr->result()).is(eax)); - if (instr->hydrogen()->pass_argument_count()) { - __ mov(eax, instr->arity()); - } + __ mov(eax, instr->arity()); // Change context. __ mov(esi, FieldOperand(edi, JSFunction::kContextOffset)); diff --git a/src/ia32/macro-assembler-ia32.cc b/src/ia32/macro-assembler-ia32.cc index 96c8160..474bb77 100644 --- a/src/ia32/macro-assembler-ia32.cc +++ b/src/ia32/macro-assembler-ia32.cc @@ -1923,10 +1923,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, Label invoke; if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + mov(eax, actual.immediate()); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - mov(eax, actual.immediate()); const int sentinel = SharedFunctionInfo::kDontAdaptArgumentsSentinel; if (expected.immediate() == sentinel) { // Don't worry about adapting arguments for builtins that @@ -1944,10 +1944,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, // Expected is in register, actual is immediate. This is the // case when we invoke function values without going through the // IC mechanism. + mov(eax, actual.immediate()); cmp(expected.reg(), actual.immediate()); j(equal, &invoke); DCHECK(expected.reg().is(ebx)); - mov(eax, actual.immediate()); } else if (!expected.reg().is(actual.reg())) { // Both expected and actual are in (different) registers. This // is the case when we invoke functions using call and apply. @@ -1955,6 +1955,8 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, j(equal, &invoke); DCHECK(actual.reg().is(eax)); DCHECK(expected.reg().is(ebx)); + } else { + Move(eax, actual.reg()); } } diff --git a/src/mips/lithium-codegen-mips.cc b/src/mips/lithium-codegen-mips.cc index 5586ed1..bbe5092 100644 --- a/src/mips/lithium-codegen-mips.cc +++ b/src/mips/lithium-codegen-mips.cc @@ -3413,11 +3413,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ lw(cp, FieldMemOperand(function_reg, JSFunction::kContextOffset)); - // Set r0 to arguments count if adaption is not needed. Assumes that r0 - // is available to write to at this point. - if (dont_adapt_arguments) { - __ li(a0, Operand(arity)); - } + // Always initialize a0 to the number of actual arguments. + __ li(a0, Operand(arity)); // Invoke function. __ lw(at, FieldMemOperand(function_reg, JSFunction::kCodeEntryOffset)); @@ -3824,9 +3821,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(ToRegister(instr->function()).is(a1)); DCHECK(ToRegister(instr->result()).is(v0)); - if (instr->hydrogen()->pass_argument_count()) { - __ li(a0, Operand(instr->arity())); - } + __ li(a0, Operand(instr->arity())); // Change context. __ lw(cp, FieldMemOperand(a1, JSFunction::kContextOffset)); diff --git a/src/mips/macro-assembler-mips.cc b/src/mips/macro-assembler-mips.cc index 1e175f4..eb33c23 100644 --- a/src/mips/macro-assembler-mips.cc +++ b/src/mips/macro-assembler-mips.cc @@ -4081,10 +4081,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + li(a0, Operand(actual.immediate())); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - li(a0, Operand(actual.immediate())); const int sentinel = SharedFunctionInfo::kDontAdaptArgumentsSentinel; if (expected.immediate() == sentinel) { // Don't worry about adapting arguments for builtins that @@ -4098,8 +4098,8 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, } } } else if (actual.is_immediate()) { - Branch(®ular_invoke, eq, expected.reg(), Operand(actual.immediate())); li(a0, Operand(actual.immediate())); + Branch(®ular_invoke, eq, expected.reg(), Operand(a0)); } else { Branch(®ular_invoke, eq, expected.reg(), Operand(actual.reg())); } diff --git a/src/mips64/lithium-codegen-mips64.cc b/src/mips64/lithium-codegen-mips64.cc index a612681..4eeb53b 100644 --- a/src/mips64/lithium-codegen-mips64.cc +++ b/src/mips64/lithium-codegen-mips64.cc @@ -3582,11 +3582,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ ld(cp, FieldMemOperand(function_reg, JSFunction::kContextOffset)); - // Set r0 to arguments count if adaption is not needed. Assumes that r0 - // is available to write to at this point. - if (dont_adapt_arguments) { - __ li(a0, Operand(arity)); - } + // Always initialize a0 to the number of actual arguments. + __ li(a0, Operand(arity)); // Invoke function. __ ld(at, FieldMemOperand(function_reg, JSFunction::kCodeEntryOffset)); @@ -4012,9 +4009,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(ToRegister(instr->function()).is(a1)); DCHECK(ToRegister(instr->result()).is(v0)); - if (instr->hydrogen()->pass_argument_count()) { - __ li(a0, Operand(instr->arity())); - } + __ li(a0, Operand(instr->arity())); // Change context. __ ld(cp, FieldMemOperand(a1, JSFunction::kContextOffset)); diff --git a/src/mips64/macro-assembler-mips64.cc b/src/mips64/macro-assembler-mips64.cc index 0568cd6..0aa6785 100644 --- a/src/mips64/macro-assembler-mips64.cc +++ b/src/mips64/macro-assembler-mips64.cc @@ -4084,10 +4084,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + li(a0, Operand(actual.immediate())); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - li(a0, Operand(actual.immediate())); const int sentinel = SharedFunctionInfo::kDontAdaptArgumentsSentinel; if (expected.immediate() == sentinel) { // Don't worry about adapting arguments for builtins that @@ -4101,8 +4101,8 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, } } } else if (actual.is_immediate()) { - Branch(®ular_invoke, eq, expected.reg(), Operand(actual.immediate())); li(a0, Operand(actual.immediate())); + Branch(®ular_invoke, eq, expected.reg(), Operand(a0)); } else { Branch(®ular_invoke, eq, expected.reg(), Operand(actual.reg())); } diff --git a/src/x64/lithium-codegen-x64.cc b/src/x64/lithium-codegen-x64.cc index 2505890..4557ce1 100644 --- a/src/x64/lithium-codegen-x64.cc +++ b/src/x64/lithium-codegen-x64.cc @@ -3411,11 +3411,8 @@ void LCodeGen::CallKnownFunction(Handle function, // Change context. __ movp(rsi, FieldOperand(function_reg, JSFunction::kContextOffset)); - // Set rax to arguments count if adaption is not needed. Assumes that rax - // is available to write to at this point. - if (dont_adapt_arguments) { - __ Set(rax, arity); - } + // Always initialize rax to the number of actual arguments. + __ Set(rax, arity); // Invoke function. if (function.is_identical_to(info()->closure())) { @@ -3478,9 +3475,7 @@ void LCodeGen::DoCallJSFunction(LCallJSFunction* instr) { DCHECK(ToRegister(instr->function()).is(rdi)); DCHECK(ToRegister(instr->result()).is(rax)); - if (instr->hydrogen()->pass_argument_count()) { - __ Set(rax, instr->arity()); - } + __ Set(rax, instr->arity()); // Change context. __ movp(rsi, FieldOperand(rdi, JSFunction::kContextOffset)); diff --git a/src/x64/macro-assembler-x64.cc b/src/x64/macro-assembler-x64.cc index 884cbab..e3ab7c7 100644 --- a/src/x64/macro-assembler-x64.cc +++ b/src/x64/macro-assembler-x64.cc @@ -3632,10 +3632,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, Label invoke; if (expected.is_immediate()) { DCHECK(actual.is_immediate()); + Set(rax, actual.immediate()); if (expected.immediate() == actual.immediate()) { definitely_matches = true; } else { - Set(rax, actual.immediate()); if (expected.immediate() == SharedFunctionInfo::kDontAdaptArgumentsSentinel) { // Don't worry about adapting arguments for built-ins that @@ -3653,10 +3653,10 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, // Expected is in register, actual is immediate. This is the // case when we invoke function values without going through the // IC mechanism. + Set(rax, actual.immediate()); cmpp(expected.reg(), Immediate(actual.immediate())); j(equal, &invoke, Label::kNear); DCHECK(expected.reg().is(rbx)); - Set(rax, actual.immediate()); } else if (!expected.reg().is(actual.reg())) { // Both expected and actual are in (different) registers. This // is the case when we invoke functions using call and apply. @@ -3664,6 +3664,8 @@ void MacroAssembler::InvokePrologue(const ParameterCount& expected, j(equal, &invoke, Label::kNear); DCHECK(actual.reg().is(rax)); DCHECK(expected.reg().is(rbx)); + } else { + Move(rax, actual.reg()); } } -- 2.7.4