Bug 1533890: Don't push callee unnecessarily for jit calls r=mgaudet

When pushing arguments for a JIT call, we push the callee value onto the stack, and then immediately pop it off. This is a remnant of the old implementation.

This patch removes that wart, with a small detour to rewrite/recomment alignJitStackBasedOnNArgs for clarity.

Differential Revision: https://phabricator.services.mozilla.com/D29532

--HG--
extra : moz-landing-system : lando
This commit is contained in:
Iain Ireland 2019-05-01 19:50:41 +00:00
parent 1f0e04063a
commit 1cbb4acb72
4 changed files with 131 additions and 159 deletions

View File

@ -72,8 +72,11 @@ class MOZ_RAII BaselineCacheIRCompiler : public CacheIRCompiler {
bool updateArgc(CallFlags flags, Register argcReg, Register scratch);
void loadStackObject(ArgumentKind kind, CallFlags flags, size_t stackPushed,
Register argcReg, Register dest);
void pushCallArguments(Register argcReg, Register scratch, Register scratch2,
bool isJitCall, bool isConstructing);
void pushArguments(Register argcReg, Register calleeReg, Register scratch,
Register scratch2, CallFlags flags, bool isJitCall);
void pushStandardArguments(Register argcReg, Register scratch,
Register scratch2, bool isJitCall,
bool isConstructing);
void pushArrayArguments(Register argcReg, Register scratch, Register scratch2,
bool isJitCall, bool isConstructing);
void pushFunCallArguments(Register argcReg, Register calleeReg,
@ -81,8 +84,6 @@ class MOZ_RAII BaselineCacheIRCompiler : public CacheIRCompiler {
bool isJitCall);
void pushFunApplyArgs(Register argcReg, Register calleeReg, Register scratch,
Register scratch2, bool isJitCall);
void pushFunApplyArray(Register argcReg, Register scratch, Register scratch2,
bool isJitCall);
void createThis(Register argcReg, Register calleeReg, Register scratch,
CallFlags flags);
void updateReturnValue();
@ -2606,23 +2607,52 @@ bool BaselineCacheIRCompiler::emitGuardFunApply() {
return true;
}
void BaselineCacheIRCompiler::pushCallArguments(Register argcReg,
Register scratch,
Register scratch2,
bool isJitCall,
bool isConstructing) {
void BaselineCacheIRCompiler::pushArguments(Register argcReg,
Register calleeReg,
Register scratch, Register scratch2,
CallFlags flags, bool isJitCall) {
switch (flags.getArgFormat()) {
case CallFlags::Standard:
pushStandardArguments(argcReg, scratch, scratch2, isJitCall,
flags.isConstructing());
break;
case CallFlags::Spread:
pushArrayArguments(argcReg, scratch, scratch2, isJitCall,
flags.isConstructing());
break;
case CallFlags::FunCall:
pushFunCallArguments(argcReg, calleeReg, scratch, scratch2, isJitCall);
break;
case CallFlags::FunApplyArgs:
pushFunApplyArgs(argcReg, calleeReg, scratch, scratch2, isJitCall);
break;
case CallFlags::FunApplyArray:
pushArrayArguments(argcReg, scratch, scratch2, isJitCall,
/*isConstructing =*/false);
break;
default:
MOZ_CRASH("Invalid arg format");
}
}
void BaselineCacheIRCompiler::pushStandardArguments(Register argcReg,
Register scratch,
Register scratch2,
bool isJitCall,
bool isConstructing) {
// The arguments to the call IC are pushed on the stack left-to-right.
// Our calling conventions want them right-to-left in the callee, so
// we duplicate them on the stack in reverse order.
// |this| and callee are pushed last.
// countReg contains the total number of arguments to copy.
// In addition to the actual arguments, we have to copy the callee and |this|.
// If we are constructing, we also have to copy newTarget.
// In addition to the actual arguments, we have to copy hidden arguments.
// We always have to copy |this|.
// If we are constructing, we have to copy |newTarget|.
// If we are not a jit call, we have to copy |callee|.
// We use a scratch register to avoid clobbering argc, which is an input reg.
Register countReg = scratch;
masm.move32(argcReg, countReg);
masm.add32(Imm32(2 + isConstructing), countReg);
masm.add32(Imm32(1 + !isJitCall + isConstructing), countReg);
// argPtr initially points to the last argument. Skip the stub frame.
Register argPtr = scratch2;
@ -2632,7 +2662,7 @@ void BaselineCacheIRCompiler::pushCallArguments(Register argcReg,
// Align the stack such that the JitFrameLayout is aligned on the
// JitStackAlignment.
if (isJitCall) {
masm.alignJitStackBasedOnNArgs(countReg);
masm.alignJitStackBasedOnNArgs(countReg, /*countIncludesThis = */ true);
}
// Push all values, starting at the last one.
@ -2670,7 +2700,7 @@ void BaselineCacheIRCompiler::pushArrayArguments(Register argcReg,
alignReg = scratch2;
masm.computeEffectiveAddress(Address(argcReg, 1), alignReg);
}
masm.alignJitStackBasedOnNArgs(alignReg);
masm.alignJitStackBasedOnNArgs(alignReg, /*countIncludesThis =*/false);
}
// Push newTarget, if necessary
@ -2693,13 +2723,17 @@ void BaselineCacheIRCompiler::pushArrayArguments(Register argcReg,
masm.jump(&copyStart);
masm.bind(&copyDone);
// Push the callee and |this|.
// Push |this|.
masm.pushValue(
Address(BaselineFrameReg,
STUB_FRAME_SIZE + (1 + isConstructing) * sizeof(Value)));
masm.pushValue(
Address(BaselineFrameReg,
STUB_FRAME_SIZE + (2 + isConstructing) * sizeof(Value)));
// Push |callee| if needed.
if (!isJitCall) {
masm.pushValue(
Address(BaselineFrameReg,
STUB_FRAME_SIZE + (2 + isConstructing) * sizeof(Value)));
}
}
void BaselineCacheIRCompiler::pushFunCallArguments(Register argcReg,
@ -2722,13 +2756,13 @@ void BaselineCacheIRCompiler::pushFunCallArguments(Register argcReg,
// argN (argN-1 of target) -----> arg1
//
// As demonstrated in the right column, this is exactly what we need
// the stack to look like when calling pushCallArguments for target,
// the stack to look like when calling pushStandardArguments for target,
// except with one more argument. If we subtract 1 from argc,
// everything works out correctly.
masm.sub32(Imm32(1), argcReg);
pushCallArguments(argcReg, scratch, scratch2, isJitCall,
/*isConstructing =*/false);
pushStandardArguments(argcReg, scratch, scratch2, isJitCall,
/*isConstructing =*/false);
masm.jump(&done);
masm.bind(&zeroArgs);
@ -2750,8 +2784,10 @@ void BaselineCacheIRCompiler::pushFunCallArguments(Register argcReg,
// Store the new |this|.
masm.pushValue(UndefinedValue());
// Store |callee|.
masm.Push(TypedOrValueRegister(MIRType::Object, AnyRegister(calleeReg)));
// Store |callee| if needed.
if (!isJitCall) {
masm.Push(TypedOrValueRegister(MIRType::Object, AnyRegister(calleeReg)));
}
masm.bind(&done);
}
@ -2769,7 +2805,7 @@ void BaselineCacheIRCompiler::pushFunApplyArgs(Register argcReg,
masm.addPtr(Imm32(BaselineFrame::offsetOfArg(0)), startReg);
if (isJitCall) {
masm.alignJitStackBasedOnNArgs(argcReg);
masm.alignJitStackBasedOnNArgs(argcReg, /*countIncludesThis =*/false);
}
Register endReg = scratch2;
@ -2789,19 +2825,10 @@ void BaselineCacheIRCompiler::pushFunApplyArgs(Register argcReg,
// Push arg0 as |this| for call
masm.pushValue(Address(BaselineFrameReg, STUB_FRAME_SIZE + sizeof(Value)));
// Push |callee|.
masm.Push(TypedOrValueRegister(MIRType::Object, AnyRegister(calleeReg)));
}
void BaselineCacheIRCompiler::pushFunApplyArray(Register argcReg,
Register scratch,
Register scratch2,
bool isJitCall) {
// Push the contents of the array onto the stack.
// We have already ensured that the array is packed and has no holes.
pushArrayArguments(argcReg, scratch, scratch2, isJitCall,
/*isConstructing =*/false);
// Push |callee| if needed.
if (!isJitCall) {
masm.Push(TypedOrValueRegister(MIRType::Object, AnyRegister(calleeReg)));
}
}
bool BaselineCacheIRCompiler::emitCallNativeShared(NativeCallType callType) {
@ -2831,29 +2858,8 @@ bool BaselineCacheIRCompiler::emitCallNativeShared(NativeCallType callType) {
masm.switchToObjectRealm(calleeReg, scratch);
}
switch (flags.getArgFormat()) {
case CallFlags::Standard:
pushCallArguments(argcReg, scratch, scratch2, /*isJitCall =*/false,
isConstructing);
break;
case CallFlags::Spread:
pushArrayArguments(argcReg, scratch, scratch2, /*isJitCall =*/false,
isConstructing);
break;
case CallFlags::FunCall:
pushFunCallArguments(argcReg, calleeReg, scratch, scratch2,
/*isJitCall = */ false);
break;
case CallFlags::FunApplyArgs:
pushFunApplyArgs(argcReg, calleeReg, scratch, scratch2,
/*isJitCall = */ false);
break;
case CallFlags::FunApplyArray:
pushFunApplyArray(argcReg, scratch, scratch2, /*isJitCall = */ false);
break;
default:
MOZ_CRASH("Invalid arg format");
}
pushArguments(argcReg, calleeReg, scratch, scratch2, flags,
/*isJitCall =*/false);
// Native functions have the signature:
//
@ -3095,37 +3101,8 @@ bool BaselineCacheIRCompiler::emitCallScriptedFunction() {
createThis(argcReg, calleeReg, scratch, flags);
}
switch (flags.getArgFormat()) {
case CallFlags::Standard:
pushCallArguments(argcReg, scratch, scratch2, /*isJitCall = */ true,
isConstructing);
break;
case CallFlags::Spread:
pushArrayArguments(argcReg, scratch, scratch2, /*isJitCall = */ true,
isConstructing);
break;
case CallFlags::FunCall:
pushFunCallArguments(argcReg, calleeReg, scratch, scratch2,
/*isJitCall = */ true);
break;
case CallFlags::FunApplyArgs:
pushFunApplyArgs(argcReg, calleeReg, scratch, scratch2,
/*isJitCall = */ true);
break;
case CallFlags::FunApplyArray:
pushFunApplyArray(argcReg, scratch, scratch2, /*isJitCall = */ true);
break;
default:
MOZ_CRASH("Invalid arg format");
}
// TODO: The callee is currently on top of the stack. The old
// implementation popped it at this point, but I'm not sure why,
// because it is still in a register along both paths. For now we
// just free that stack slot to make things line up. This should
// probably be rewritten to avoid pushing callee at all if we don't
// have to.
masm.freeStack(sizeof(Value));
pushArguments(argcReg, calleeReg, scratch, scratch2, flags,
/*isJitCall =*/true);
// Load the start of the target JitCode.
Register code = scratch2;

View File

@ -4059,7 +4059,7 @@ void ICStubCompilerBase::pushCallArguments(MacroAssembler& masm,
// Align the stack such that the JitFrameLayout is aligned on the
// JitStackAlignment.
if (isJitCall) {
masm.alignJitStackBasedOnNArgs(count);
masm.alignJitStackBasedOnNArgs(count, /*countIncludesThis =*/false);
// Account for callee and |this|, skipped earlier
masm.add32(Imm32(2), count);
@ -4115,7 +4115,7 @@ void ICCallStubCompiler::pushSpreadCallArguments(
masm.movePtr(argcReg, alignReg);
masm.addPtr(Imm32(1), alignReg);
}
masm.alignJitStackBasedOnNArgs(alignReg);
masm.alignJitStackBasedOnNArgs(alignReg, /*countIncludesThis =*/false);
if (isConstructing) {
MOZ_ASSERT(alignReg != argcReg);
regs.add(alignReg);
@ -4291,7 +4291,7 @@ void ICCallStubCompiler::pushCallerArguments(
masm.loadPtr(Address(startReg, BaselineFrame::offsetOfNumActualArgs()),
endReg);
masm.addPtr(Imm32(BaselineFrame::offsetOfArg(0)), startReg);
masm.alignJitStackBasedOnNArgs(endReg);
masm.alignJitStackBasedOnNArgs(endReg, /*countIncludesThis =*/false);
masm.lshiftPtr(Imm32(ValueShift), endReg);
masm.addPtr(startReg, endReg);
@ -4318,7 +4318,7 @@ void ICCallStubCompiler::pushArrayArguments(
masm.loadPtr(Address(startReg, NativeObject::offsetOfElements()), startReg);
masm.load32(Address(startReg, ObjectElements::offsetOfInitializedLength()),
endReg);
masm.alignJitStackBasedOnNArgs(endReg);
masm.alignJitStackBasedOnNArgs(endReg, /*countIncludesThis =*/false);
masm.lshiftPtr(Imm32(ValueShift), endReg);
masm.addPtr(startReg, endReg);

View File

@ -2464,87 +2464,82 @@ void MacroAssembler::linkProfilerCallSites(JitCode* code) {
}
}
void MacroAssembler::alignJitStackBasedOnNArgs(Register nargs) {
void MacroAssembler::alignJitStackBasedOnNArgs(Register nargs,
bool countIncludesThis) {
// The stack should already be aligned to the size of a value.
assertStackAlignment(sizeof(Value), 0);
static_assert(JitStackValueAlignment == 1 || JitStackValueAlignment == 2,
"JitStackValueAlignment is either 1 or 2.");
if (JitStackValueAlignment == 1) {
return;
}
// A JitFrameLayout is composed of the following:
// [padding?] [argN] .. [arg1] [this] [[argc] [callee] [descr] [raddr]]
// A jit frame is composed of the following:
//
// We want to ensure that the |raddr| address is aligned.
// Which implies that we want to ensure that |this| is aligned.
static_assert(
sizeof(JitFrameLayout) % JitStackAlignment == 0,
"No need to consider the JitFrameLayout for aligning the stack");
// [padding?] [argN] .. [arg1] [this] [[argc] [callee] [descr] [raddr]]
// \________JitFrameLayout_________/
// (The stack grows this way --->)
//
// We want to ensure that |raddr|, the return address, is 16-byte aligned.
// (Note: if 8-byte alignment was sufficient, we would have already
// returned above.)
// Which implies that |argN| is aligned if |nargs| is even, and offset by
// |sizeof(Value)| if |nargs| is odd.
MOZ_ASSERT(JitStackValueAlignment == 2);
// JitFrameLayout does not affect the alignment, so we can ignore it.
static_assert(sizeof(JitFrameLayout) % JitStackAlignment == 0,
"JitFrameLayout doesn't affect stack alignment");
// Thus the |padding| is offset by |sizeof(Value)| if |nargs| is even, and
// aligned if |nargs| is odd.
// Therefore, we need to ensure that |this| is aligned.
// This implies that |argN| must be aligned if N is even,
// and offset by |sizeof(Value)| if N is odd.
// if (nargs % 2 == 0) {
// if (sp % JitStackAlignment == 0) {
// sp -= sizeof(Value);
// }
// MOZ_ASSERT(sp % JitStackAlignment == JitStackAlignment -
// sizeof(Value));
// } else {
// sp = sp & ~(JitStackAlignment - 1);
// }
Label odd, end;
Label* maybeAssert = &end;
#ifdef DEBUG
Label assert;
maybeAssert = &assert;
#endif
assertStackAlignment(sizeof(Value), 0);
branchTestPtr(Assembler::NonZero, nargs, Imm32(1), &odd);
branchTestStackPtr(Assembler::NonZero, Imm32(JitStackAlignment - 1),
maybeAssert);
subFromStackPtr(Imm32(sizeof(Value)));
#ifdef DEBUG
bind(&assert);
#endif
assertStackAlignment(JitStackAlignment, sizeof(Value));
jump(&end);
bind(&odd);
// Depending on the context of the caller, it may be easier to pass in a
// register that has already been modified to include |this|. If that is the
// case, we want to flip the direction of the test.
Assembler::Condition condition =
countIncludesThis ? Assembler::NonZero : Assembler::Zero;
Label alignmentIsOffset, end;
branchTestPtr(condition, nargs, Imm32(1), &alignmentIsOffset);
// |argN| should be aligned to 16 bytes.
andToStackPtr(Imm32(~(JitStackAlignment - 1)));
jump(&end);
// |argN| should be offset by 8 bytes from 16-byte alignment.
// We already know that it is 8-byte aligned, so the only possibilities are:
// a) It is 16-byte aligned, and we must offset it by 8 bytes.
// b) It is not 16-byte aligned, and therefore already has the right offset.
// Therefore, we test to see if it is 16-byte aligned, and adjust it if it is.
bind(&alignmentIsOffset);
branchTestStackPtr(Assembler::NonZero, Imm32(JitStackAlignment - 1), &end);
subFromStackPtr(Imm32(sizeof(Value)));
bind(&end);
}
void MacroAssembler::alignJitStackBasedOnNArgs(uint32_t nargs) {
void MacroAssembler::alignJitStackBasedOnNArgs(uint32_t argc) {
// The stack should already be aligned to the size of a value.
assertStackAlignment(sizeof(Value), 0);
static_assert(JitStackValueAlignment == 1 || JitStackValueAlignment == 2,
"JitStackValueAlignment is either 1 or 2.");
if (JitStackValueAlignment == 1) {
return;
}
// A JitFrameLayout is composed of the following:
// [padding?] [argN] .. [arg1] [this] [[argc] [callee] [descr] [raddr]]
//
// We want to ensure that the |raddr| address is aligned.
// Which implies that we want to ensure that |this| is aligned.
static_assert(
sizeof(JitFrameLayout) % JitStackAlignment == 0,
"No need to consider the JitFrameLayout for aligning the stack");
// Which implies that |argN| is aligned if |nargs| is even, and offset by
// |sizeof(Value)| if |nargs| is odd.
MOZ_ASSERT(JitStackValueAlignment == 2);
// Thus the |padding| is offset by |sizeof(Value)| if |nargs| is even, and
// aligned if |nargs| is odd.
assertStackAlignment(sizeof(Value), 0);
if (nargs % 2 == 0) {
// See above for full explanation.
uint32_t nArgs = argc + 1;
if (nArgs % 2 == 0) {
// |argN| should be 16-byte aligned
andToStackPtr(Imm32(~(JitStackAlignment - 1)));
} else {
// |argN| must be 16-byte aligned if argc is even,
// and offset by 8 if argc is odd.
Label end;
branchTestStackPtr(Assembler::NonZero, Imm32(JitStackAlignment - 1), &end);
subFromStackPtr(Imm32(sizeof(Value)));
bind(&end);
assertStackAlignment(JitStackAlignment, sizeof(Value));
} else {
andToStackPtr(Imm32(~(JitStackAlignment - 1)));
}
}

View File

@ -3177,8 +3177,8 @@ class MacroAssembler : public MacroAssemblerSpecific {
// Align the stack pointer based on the number of arguments which are pushed
// on the stack, such that the JitFrameLayout would be correctly aligned on
// the JitStackAlignment.
void alignJitStackBasedOnNArgs(Register nargs);
void alignJitStackBasedOnNArgs(uint32_t nargs);
void alignJitStackBasedOnNArgs(Register nargs, bool countIncludesThis);
void alignJitStackBasedOnNArgs(uint32_t argc);
inline void assertStackAlignment(uint32_t alignment, int32_t offset = 0);