- Revision
- 187676
- Author
- [email protected]
- Date
- 2015-07-31 13:31:39 -0700 (Fri, 31 Jul 2015)
Log Message
jsc-tailcall: Don't waste stack space when arity fixup was performed
https://bugs.webkit.org/show_bug.cgi?id=147447
Reviewed by Michael Saboff.
r187618 tried to handle this, but did not include the LLInt and did it
wrong anyway. When making a call, the caller first aligns the callee's
frame, then makes the call. If the callee later performs arity fixup,
it does not try to reuse the potential slot left empty by the caller's
frame alignment: instead, it aligns the parameters - arguments delta.
So when trying to find out where our frame starts, we should replicate
this behavior, which we were previously not doing and was the cause of
https://bugs.webkit.org/show_bug.cgi?id=147491.
While here, I also removed the extra slot that used to save the
returnPC for the arity fixup return thunk (that we got rid of in
https://bugs.webkit.org/show_bug.cgi?id=146847) and was now just
wasting stack space.
* jit/CCallHelpers.h:
* jit/ThunkGenerators.cpp:
(JSC::arityFixupGenerator):
* llint/LowLevelInterpreter.asm:
* runtime/CommonSlowPaths.h:
(JSC::CommonSlowPaths::arityCheckFor):
* tests/stress/tail-call-no-stack-overflow.js:
(strictLoopArityFixup1):
(strictLoopArityFixup2):
Modified Paths
Diff
Modified: branches/jsc-tailcall/Source/_javascript_Core/ChangeLog (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/ChangeLog 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/ChangeLog 2015-07-31 20:31:39 UTC (rev 187676)
@@ -1,3 +1,34 @@
+2015-07-31 Basile Clement <[email protected]>
+
+ jsc-tailcall: Don't waste stack space when arity fixup was performed
+ https://bugs.webkit.org/show_bug.cgi?id=147447
+
+ Reviewed by Michael Saboff.
+
+ r187618 tried to handle this, but did not include the LLInt and did it
+ wrong anyway. When making a call, the caller first aligns the callee's
+ frame, then makes the call. If the callee later performs arity fixup,
+ it does not try to reuse the potential slot left empty by the caller's
+ frame alignment: instead, it aligns the parameters - arguments delta.
+ So when trying to find out where our frame starts, we should replicate
+ this behavior, which we were previously not doing and was the cause of
+ https://bugs.webkit.org/show_bug.cgi?id=147491.
+
+ While here, I also removed the extra slot that used to save the
+ returnPC for the arity fixup return thunk (that we got rid of in
+ https://bugs.webkit.org/show_bug.cgi?id=146847) and was now just
+ wasting stack space.
+
+ * jit/CCallHelpers.h:
+ * jit/ThunkGenerators.cpp:
+ (JSC::arityFixupGenerator):
+ * llint/LowLevelInterpreter.asm:
+ * runtime/CommonSlowPaths.h:
+ (JSC::CommonSlowPaths::arityCheckFor):
+ * tests/stress/tail-call-no-stack-overflow.js:
+ (strictLoopArityFixup1):
+ (strictLoopArityFixup2):
+
2015-07-31 Michael Saboff <[email protected]>
jsc-tailcall: REGRESSION(r187639): Need to restore callee saves in LLInt before tail call
Modified: branches/jsc-tailcall/Source/_javascript_Core/jit/CCallHelpers.h (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/jit/CCallHelpers.h 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/jit/CCallHelpers.h 2015-07-31 20:31:39 UTC (rev 187676)
@@ -2047,7 +2047,10 @@
loadPtr(Address(GPRInfo::callFrameRegister, JSStack::CodeBlock * static_cast<int>(sizeof(Register))), temp2);
load32(Address(temp2, CodeBlock::offsetOfNumParameters()), temp2);
MacroAssembler::Jump argumentCountWasNotFixedUp = branch32(BelowOrEqual, temp2, temp1);
- move(temp2, temp1);
+ sub32(temp1, temp2);
+ add32(TrustedImm32(stackAlignmentRegisters() - 1), temp2);
+ and32(TrustedImm32(-stackAlignmentRegisters()), temp2);
+ add32(temp2, temp1);
argumentCountWasNotFixedUp.link(this);
add32(TrustedImm32(stackAlignmentRegisters() + JSStack::CallFrameHeaderSize - 1), temp1);
Modified: branches/jsc-tailcall/Source/_javascript_Core/jit/ThunkGenerators.cpp (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/jit/ThunkGenerators.cpp 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/jit/ThunkGenerators.cpp 2015-07-31 20:31:39 UTC (rev 187676)
@@ -388,10 +388,9 @@
jit.addPtr(JSInterfaceJIT::TrustedImm32(8), JSInterfaceJIT::regT3);
jit.branchSub32(MacroAssembler::NonZero, JSInterfaceJIT::TrustedImm32(1), JSInterfaceJIT::argumentGPR2).linkTo(copyLoop, &jit);
- // Fill in argumentGPR0 - 1 missing arg slots with undefined
+ // Fill in argumentGPR0 missing arg slots with undefined
jit.move(JSInterfaceJIT::argumentGPR0, JSInterfaceJIT::argumentGPR2);
jit.move(JSInterfaceJIT::TrustedImm64(ValueUndefined), extraTemp);
- jit.add32(JSInterfaceJIT::TrustedImm32(1), JSInterfaceJIT::argumentGPR2);
JSInterfaceJIT::Label fillUndefinedLoop(jit.label());
jit.store64(extraTemp, MacroAssembler::BaseIndex(JSInterfaceJIT::regT3, JSInterfaceJIT::argumentGPR0, JSInterfaceJIT::TimesEight));
jit.addPtr(JSInterfaceJIT::TrustedImm32(8), JSInterfaceJIT::regT3);
@@ -426,9 +425,8 @@
jit.addPtr(JSInterfaceJIT::TrustedImm32(8), JSInterfaceJIT::regT3);
jit.branchSub32(MacroAssembler::NonZero, JSInterfaceJIT::TrustedImm32(1), JSInterfaceJIT::argumentGPR2).linkTo(copyLoop, &jit);
- // Fill in argumentGPR0 - 1 missing arg slots with undefined
+ // Fill in argumentGPR0 missing arg slots with undefined
jit.move(JSInterfaceJIT::argumentGPR0, JSInterfaceJIT::argumentGPR2);
- jit.add32(JSInterfaceJIT::TrustedImm32(1), JSInterfaceJIT::argumentGPR2);
JSInterfaceJIT::Label fillUndefinedLoop(jit.label());
jit.move(JSInterfaceJIT::TrustedImm32(0), JSInterfaceJIT::regT5);
jit.store32(JSInterfaceJIT::regT5, MacroAssembler::BaseIndex(JSInterfaceJIT::regT3, JSInterfaceJIT::argumentGPR0, JSInterfaceJIT::TimesEight));
Modified: branches/jsc-tailcall/Source/_javascript_Core/llint/LowLevelInterpreter.asm (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/llint/LowLevelInterpreter.asm 2015-07-31 20:31:39 UTC (rev 187676)
@@ -666,13 +666,32 @@
addp CallerFrameAndPCSize, sp
end
-macro getFrameSize(frame, dst)
- loadi PayloadOffset + ArgumentCount[frame], dst
- mulp SlotSize, dst
- addp StackAlignment - 1 + CallFrameHeaderSize, dst
- andp ~StackAlignmentMask, dst
+macro alignFrameSize(argCount)
+ addp StackAlignment - 1 + CallFrameHeaderSize, argCount
+ andp ~StackAlignmentMask, argCount
end
+macro popFrame(temp1, temp2)
+ const argCount = temp1
+ const numParameters = temp2
+ const missingArgCount = temp2
+
+ loadi PayloadOffset + ArgumentCount[cfr], argCount
+ mulp SlotSize, argCount
+ loadp CodeBlock[cfr], numParameters
+ loadi CodeBlock::m_numParameters[numParameters], numParameters
+ mulp SlotSize, numParameters
+ bilteq numParameters, argCount, .noArityFixup
+
+ subi numParameters, argCount, missingArgCount
+ addp StackAlignmentMask, missingArgCount
+ andp ~StackAlignmentMask, missingArgCount
+ addi missingArgCount, argCount
+.noArityFixup:
+ alignFrameSize(argCount)
+ addp argCount, cfr
+end
+
# sp points to the new frame
macro prepareForTailCall(callee, temp1, temp2)
restoreCalleeSavesUsedByLLInt()
@@ -683,10 +702,12 @@
storep temp2, ReturnPC[sp]
# Slide the new call frame over the old one
- getFrameSize(cfr, temp1)
- addp temp1, cfr
- getFrameSize(sp, temp2)
+ popFrame(temp1, temp2)
+ loadi PayloadOffset + ArgumentCount[sp], temp2
+ mulp SlotSize, temp2
+ alignFrameSize(temp2)
+
.copyLoop:
subp PtrSize, cfr
subp PtrSize, temp2
Modified: branches/jsc-tailcall/Source/_javascript_Core/runtime/CommonSlowPaths.h (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/runtime/CommonSlowPaths.h 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/runtime/CommonSlowPaths.h 2015-07-31 20:31:39 UTC (rev 187676)
@@ -60,8 +60,7 @@
ASSERT(argumentCountIncludingThis < newCodeBlock->numParameters());
int missingArgumentCount = newCodeBlock->numParameters() - argumentCountIncludingThis;
- int neededStackSpace = missingArgumentCount + 1; // Allow space to save the original return PC.
- int paddedStackSpace = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), neededStackSpace);
+ int paddedStackSpace = WTF::roundUpToMultipleOf(stackAlignmentRegisters(), missingArgumentCount);
if (!stack->ensureCapacityFor(exec->registers() - paddedStackSpace))
return -1;
Modified: branches/jsc-tailcall/Source/_javascript_Core/tests/stress/tail-call-no-stack-overflow.js (187675 => 187676)
--- branches/jsc-tailcall/Source/_javascript_Core/tests/stress/tail-call-no-stack-overflow.js 2015-07-31 20:21:15 UTC (rev 187675)
+++ branches/jsc-tailcall/Source/_javascript_Core/tests/stress/tail-call-no-stack-overflow.js 2015-07-31 20:31:39 UTC (rev 187676)
@@ -24,14 +24,22 @@
return strictLoop(n - 1);
}
-function strictLoopArityFixup(n, dummy) {
+// We have two of these so that we can test different stack alignments
+function strictLoopArityFixup1(n, dummy) {
"use strict";
if (n > 0)
return strictLoopArityFixup(n - 1);
}
+function strictLoopArityFixup2(n, dummy1, dummy2) {
+ "use strict";
+ if (n > 0)
+ return strictLoopArityFixup(n - 1);
+}
+
shouldThrow(function () { sloppyLoop(100000); }, 'RangeError: Maximum call stack size exceeded.');
// These should not throw
strictLoop(100000);
-strictLoopArityFixup(100000);
+strictLoopArityFixup1(1000000);
+strictLoopArityFixup2(1000000);
