Changeset 187618 in webkit

Timestamp:

Jul 30, 2015 4:19:30 PM (9 years ago)

Author:

basile_clement@apple.com

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.

When doing a tail call, we overwrite an amount of stack space based on
the number of arguments in the call frame. If we entered the tail
caller by performing an arity fixup, this is incorrect and leads to
wasted stack space - we must use the CodeBlock's number of parameters
instead in that case.

This patch is also moving the prepareForTailCall() function from
jit/ThunkGenerators.h to the place where it should have always been,
namely jit/CCallHelpers.h

• jit/CCallHelpers.h:

(JSC::CCallHelpers::prepareForTailCallSlow):

• jit/JITCall.cpp:

(JSC::JIT::compileOpCall):

• jit/Repatch.cpp:

(JSC::linkPolymorphicCall):

• jit/ThunkGenerators.cpp:

(JSC::slowPathFor):
(JSC::virtualThunkFor):

• jit/ThunkGenerators.h:
• tests/stress/tail-call-no-stack-overflow.js:

(strictLoopArityFixup):

Location:

branches/jsc-tailcall/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• jit/CCallHelpers.h (modified) (2 diffs)
• jit/JITCall.cpp (modified) (1 diff)
• jit/JITCall32_64.cpp (modified) (1 diff)
• jit/Repatch.cpp (modified) (1 diff)
• jit/ThunkGenerators.cpp (modified) (3 diffs)
• jit/ThunkGenerators.h (modified) (1 diff)
• tests/stress/tail-call-no-stack-overflow.js (modified) (1 diff)

branches/jsc-tailcall/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2015-07-30  Basile Clement  <basile_clement@apple.com>
+
+        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.
+
+        When doing a tail call, we overwrite an amount of stack space based on
+        the number of arguments in the call frame. If we entered the tail
+        caller by performing an arity fixup, this is incorrect and leads to
+        wasted stack space - we must use the CodeBlock's number of parameters
+        instead in that case.
+
+        This patch is also moving the prepareForTailCall() function from
+        jit/ThunkGenerators.h to the place where it should have always been,
+        namely jit/CCallHelpers.h
+
+        * jit/CCallHelpers.h:
+        (JSC::CCallHelpers::prepareForTailCallSlow):
+        * jit/JITCall.cpp:
+        (JSC::JIT::compileOpCall):
+        * jit/Repatch.cpp:
+        (JSC::linkPolymorphicCall):
+        * jit/ThunkGenerators.cpp:
+        (JSC::slowPathFor):
+        (JSC::virtualThunkFor):
+        * jit/ThunkGenerators.h:
+        * tests/stress/tail-call-no-stack-overflow.js:
+        (strictLoopArityFixup):
+
 2015-07-30  Basile Clement  <basile_clement@apple.com>
 

branches/jsc-tailcall/Source/JavaScriptCore/jit/CCallHelpers.h

@@ -31 +31 @@
 #include "AssemblyHelpers.h"
 #include "GPRInfo.h"
+#include "StackAlignment.h"
 
 namespace JSC {
@@ -2021 +2022 @@
         jump(GPRInfo::regT1);
     }
+
+    void prepareForTailCallSlow(const TempRegisterSet& usedRegisters = { RegisterSet::specialRegisters() })
+    {
+        GPRReg temp1 = usedRegisters.getFreeGPR(0);
+        GPRReg temp2 = usedRegisters.getFreeGPR(1);
+        ASSERT(temp2 != InvalidGPRReg);
+
+        subPtr(TrustedImm32(sizeof(CallerFrameAndPC)), stackPointerRegister);
+        loadPtr(Address(GPRInfo::callFrameRegister), temp1);
+        storePtr(temp1, Address(stackPointerRegister));
+        loadPtr(Address(GPRInfo::callFrameRegister, sizeof(void*)), temp1);
+        storePtr(temp1, Address(stackPointerRegister, sizeof(void*)));
+
+        // Now stackPointerRegister points to a valid call frame for the callee
+        // and callFrameRegister points to our own call frame.
+        // We now slide the callee's call frame over our own call frame,
+        // starting with the top to avoid unwanted overwrites
+
+        // Move the callFrameRegister to the top of our (trashed) call frame
+        load32(Address(GPRInfo::callFrameRegister, JSStack::ArgumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset),
+            temp1);
+
+        // We need to use the number of parameters instead of the argument count in case of arity fixup
+        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);
+        argumentCountWasNotFixedUp.link(this);
+
+        add32(TrustedImm32(stackAlignmentRegisters() + JSStack::CallFrameHeaderSize - 1), temp1);
+        and32(TrustedImm32(-stackAlignmentRegisters()), temp1);
+        mul32(TrustedImm32(sizeof(Register)), temp1, temp1);
+        addPtr(temp1, GPRInfo::callFrameRegister);
+
+        // Compute the call frame size of the callee's call frame
+        load32(Address(stackPointerRegister, JSStack::ArgumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset),
+            temp2);
+        add32(TrustedImm32(stackAlignmentRegisters() + JSStack::CallFrameHeaderSize - 1), temp2);
+        and32(TrustedImm32(-stackAlignmentRegisters()), temp2);
+
+#if USE(JSVALUE32_64)
+        COMPILE_ASSERT(sizeof(void*) * 2 == sizeof(Register), Register_is_two_pointers_sized);
+        lshift32(TrustedImm32(1), temp2);
+#endif
+
+        // Do the sliding
+        MacroAssembler::Label copyLoop(label());
+
+        subPtr(TrustedImm32(sizeof(void*)), GPRInfo::callFrameRegister);
+        sub32(TrustedImm32(1), temp2);
+        loadPtr(BaseIndex(stackPointerRegister, temp2, MacroAssembler::timesPtr()), temp1);
+        storePtr(temp1, Address(GPRInfo::callFrameRegister));
+
+        branchTest32(MacroAssembler::NonZero, temp2).linkTo(copyLoop, this);
+
+        emitFunctionEpilogue();
+    }
+
+    void prepareForTailCallSlow(GPRReg calleeGPR)
+    {
+        TempRegisterSet usedRegisters { RegisterSet::specialRegisters() };
+        usedRegisters.set(calleeGPR);
+        prepareForTailCallSlow(usedRegisters);
+    }
 };
 

branches/jsc-tailcall/Source/JavaScriptCore/jit/JITCall.cpp

@@ -193 +193 @@
 
     if (opcodeID == op_tail_call || opcodeID == op_tail_call_varargs) {
-        prepareForTailCall(*this);
+        prepareForTailCallSlow();
         m_callCompilationInfo[callLinkInfoIndex].hotPathOther = emitNakedTailCall();
         // We must never come back here

branches/jsc-tailcall/Source/JavaScriptCore/jit/JITCall32_64.cpp

@@ -277 +277 @@
     checkStackPointerAlignment();
     if (opcodeID == op_tail_call || opcodeID == op_tail_call_varargs) {
-        prepareForTailCall(*this);
+        prepareForTailCallSlow();
         m_callCompilationInfo[callLinkInfoIndex].hotPathOther = emitNakedTailCall();
         // We must never come back here

branches/jsc-tailcall/Source/JavaScriptCore/jit/Repatch.cpp

@@ -1882 +1882 @@
         }
         if (CallLinkInfo::isTailCallType(callLinkInfo.callType())) {
-            prepareForTailCall(stubJit);
+            stubJit.prepareForTailCallSlow();
             calls[caseIndex].call = stubJit.nearTailCall();
         } else

branches/jsc-tailcall/Source/JavaScriptCore/jit/ThunkGenerators.cpp

@@ -103 +103 @@
 
     jit.preserveReturnAddressAfterCall(GPRInfo::nonPreservedNonReturnGPR);
-    jit.move(GPRInfo::returnValueGPR, GPRInfo::regT4); // FIXME
-    prepareForTailCall(jit);
-    jit.jump(GPRInfo::regT4);
+    jit.prepareForTailCallSlow(GPRInfo::returnValueGPR);
 
     doNotTrash.link(&jit);
@@ -196 +194 @@
     if (CallLinkInfo::isTailCallType(callLinkInfo.callType())) {
         jit.preserveReturnAddressAfterCall(GPRInfo::regT0);
-        prepareForTailCall(jit);
+        jit.prepareForTailCallSlow(GPRInfo::regT4);
     }
     jit.jump(GPRInfo::regT4);
@@ -460 +458 @@
     LinkBuffer patchBuffer(*vm, jit, GLOBAL_THUNK_ID);
     return FINALIZE_CODE(patchBuffer, ("unreachable thunk"));
-}
-
-void prepareForTailCall(CCallHelpers& jit)
-{
-    jit.subPtr(CCallHelpers::TrustedImm32(sizeof(CallerFrameAndPC)), JSInterfaceJIT::stackPointerRegister);
-    jit.loadPtr(CCallHelpers::Address(JSInterfaceJIT::callFrameRegister), JSInterfaceJIT::regT1);
-    jit.storePtr(JSInterfaceJIT::regT1, CCallHelpers::Address(JSInterfaceJIT::stackPointerRegister));
-    jit.loadPtr(CCallHelpers::Address(JSInterfaceJIT::callFrameRegister, sizeof(void*)), JSInterfaceJIT::regT1);
-    jit.storePtr(JSInterfaceJIT::regT1, CCallHelpers::Address(JSInterfaceJIT::stackPointerRegister, sizeof(void*)));
-
-    // Now stackPointerRegister points to a valid call frame for the callee
-    // and callFrameRegister points to our own call frame.
-    // We now slide the callee's call frame over our own call frame,
-    // starting with the top to avoid unwanted overwrites
-
-    // Move the callFrameRegister to the top of our (trashed) call frame
-    jit.load32(CCallHelpers::Address(JSInterfaceJIT::callFrameRegister, JSStack::ArgumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset),
-        JSInterfaceJIT::regT1);
-    jit.add32(CCallHelpers::TrustedImm32(stackAlignmentRegisters() + JSStack::CallFrameHeaderSize - 1), JSInterfaceJIT::regT1);
-    jit.and32(CCallHelpers::TrustedImm32(-stackAlignmentRegisters()), JSInterfaceJIT::regT1);
-    jit.mul32(CCallHelpers::TrustedImm32(sizeof(Register)), JSInterfaceJIT::regT1, JSInterfaceJIT::regT1);
-    jit.addPtr(JSInterfaceJIT::regT1, JSInterfaceJIT::callFrameRegister);
-
-    // Compute the call frame size of the callee's call frame
-    jit.load32(CCallHelpers::Address(JSInterfaceJIT::stackPointerRegister, JSStack::ArgumentCount * static_cast<int>(sizeof(Register)) + PayloadOffset),
-        JSInterfaceJIT::regT2);
-    jit.add32(CCallHelpers::TrustedImm32(stackAlignmentRegisters() + JSStack::CallFrameHeaderSize - 1), JSInterfaceJIT::regT2);
-    jit.and32(CCallHelpers::TrustedImm32(-stackAlignmentRegisters()), JSInterfaceJIT::regT2);
-
-#if USE(JSVALUE32_64)
-    COMPILE_ASSERT(sizeof(void*) * 2 == sizeof(Register), Register_is_two_pointers_sized);
-    jit.lshift32(CCallHelpers::TrustedImm32(1), JSInterfaceJIT::regT2);
-#endif
-
-    // Do the sliding
-    MacroAssembler::Label copyLoop(jit.label());
-
-    jit.subPtr(CCallHelpers::TrustedImm32(sizeof(void*)), JSInterfaceJIT::callFrameRegister);
-    jit.sub32(CCallHelpers::TrustedImm32(1), JSInterfaceJIT::regT2);
-    jit.loadPtr(CCallHelpers::BaseIndex(JSInterfaceJIT::stackPointerRegister, JSInterfaceJIT::regT2, MacroAssembler::timesPtr()), JSInterfaceJIT::regT1);
-    jit.storePtr(JSInterfaceJIT::regT1, CCallHelpers::Address(JSInterfaceJIT::callFrameRegister));
-
-    jit.branchTest32(MacroAssembler::NonZero, JSInterfaceJIT::regT2).linkTo(copyLoop, &jit);
-
-    jit.emitFunctionEpilogue();
 }
 

branches/jsc-tailcall/Source/JavaScriptCore/jit/ThunkGenerators.h

@@ -50 +50 @@
 MacroAssemblerCodeRef unreachableGenerator(VM*);
 
-void prepareForTailCall(CCallHelpers& jit);
-
 MacroAssemblerCodeRef baselineGetterReturnThunkGenerator(VM* vm);
 MacroAssemblerCodeRef baselineSetterReturnThunkGenerator(VM* vm);

branches/jsc-tailcall/Source/JavaScriptCore/tests/stress/tail-call-no-stack-overflow.js

@@ -25 +25 @@
 }
 
+function strictLoopArityFixup(n, dummy) {
+    "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);