Changeset 209725 in webkit for trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

Timestamp:

Dec 12, 2016, 1:46:45 PM (9 years ago)

Author:

[email protected]

Message:

REGRESSION(r209653): speedometer crashes making virtual slow path tailcalls
​https://bugs.webkit.org/show_bug.cgi?id=165748

Reviewed by Filip Pizlo.

JSTests:

New regression test.

• stress/regress-165748.js: Added.

(sum1):
(sum2):
(sum3):
(sum4):
(sum5):
(sum6):
(tailCaller):
(test):

Source/JavaScriptCore:

The virtual slow path for tailcalls always passes arguments on the stack.
The fix here is to link to the stack argument entrypoint instead of a register
argument entrypoint.

While fixing this bug, I found that we weren't clearing the code origin when
shuffling the call frame for a register argument tailcall.

Also rolling back in r209653, r209654, r209663, and r209673.

• jit/CallFrameShuffler.cpp:

(JSC::CallFrameShuffler::prepareAny):

• jit/ThunkGenerators.cpp:

(JSC::virtualThunkFor):

Source/WTF:

Rolling back in r209653, r209654, r209663, and r209673.

• wtf/Platform.h:

File:

• trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp (modified) (19 diffs)

trunk/Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp

@@ -81 +81 @@
 }
 
-GPRReg SpeculativeJIT::fillJSValue(Edge edge)
+GPRReg SpeculativeJIT::fillJSValue(Edge edge, GPRReg gprToUse)
 {
     VirtualRegister virtualRegister = edge->virtualRegister();
@@ -88 +88 @@
     switch (info.registerFormat()) {
     case DataFormatNone: {
-        GPRReg gpr = allocate();
+        GPRReg gpr = allocate(gprToUse);
 
         if (edge->hasConstant()) {
@@ -121 +121 @@
         // If not, we'll zero extend in place, so mark on the info that this is now type DataFormatInt32, not DataFormatJSInt32.
         if (m_gprs.isLocked(gpr)) {
-            GPRReg result = allocate();
+            GPRReg result = allocate(gprToUse);
+            m_jit.or64(GPRInfo::tagTypeNumberRegister, gpr, result);
+            return result;
+        }
+        if (gprToUse != InvalidGPRReg && gpr != gprToUse) {
+            GPRReg result = allocate(gprToUse);
             m_jit.or64(GPRInfo::tagTypeNumberRegister, gpr, result);
             return result;
@@ -139 +144 @@
     case DataFormatJSBoolean: {
         GPRReg gpr = info.gpr();
+        if (gprToUse != InvalidGPRReg && gpr != gprToUse) {
+            GPRReg result = allocate(gprToUse);
+            m_jit.move(gpr, result);
+            return result;
+        }
         m_gprs.lock(gpr);
         return gpr;
@@ -633 +643 @@
 {
     CallLinkInfo::CallType callType;
+    ArgumentsLocation argumentsLocation = StackArgs;
     bool isVarargs = false;
     bool isForwardVarargs = false;
@@ -715 +726 @@
     GPRReg calleeGPR = InvalidGPRReg;
     CallFrameShuffleData shuffleData;
-    
+    std::optional<JSValueOperand> tailCallee;
+    std::optional<GPRTemporary> calleeGPRTemporary;
+
+    incrementCounter(&m_jit, VM::DFGCaller);
+
     ExecutableBase* executable = nullptr;
     FunctionExecutable* functionExecutable = nullptr;
@@ -734 +749 @@
         unsigned numUsedStackSlots = m_jit.graph().m_nextMachineLocal;
         
+        incrementCounter(&m_jit, VM::CallVarargs);
         if (isForwardVarargs) {
             flushRegisters();
@@ -842 +858 @@
 
         if (isTail) {
+            incrementCounter(&m_jit, VM::TailCall);
             Edge calleeEdge = m_jit.graph().child(node, 0);
-            JSValueOperand callee(this, calleeEdge);
-            calleeGPR = callee.gpr();
+            // We can't get the a specific register for the callee, since that will just move
+            // from any current register.  When we silent fill in the slow path we'll fill
+            // the original register and won't have the callee in the right register.
+            // Therefore we allocate a temp register for the callee and move ourselves.
+            tailCallee.emplace(this, calleeEdge);
+            GPRReg tailCalleeGPR = tailCallee->gpr();
+            calleeGPR = argumentRegisterForCallee();
+            if (tailCalleeGPR != calleeGPR)
+                calleeGPRTemporary = GPRTemporary(this, calleeGPR);
             if (!isDirect)
-                callee.use();
-
+                tailCallee->use();
+
+            argumentsLocation = argumentsLocationFor(numAllocatedArgs);
+            shuffleData.argumentsInRegisters = argumentsLocation != StackArgs;
             shuffleData.tagTypeNumber = GPRInfo::tagTypeNumberRegister;
             shuffleData.numLocals = m_jit.graph().frameRegisterCount();
-            shuffleData.callee = ValueRecovery::inGPR(calleeGPR, DataFormatJS);
+            shuffleData.callee = ValueRecovery::inGPR(tailCalleeGPR, DataFormatJS);
             shuffleData.args.resize(numAllocatedArgs);
 
@@ -865 +891 @@
 
             shuffleData.setupCalleeSaveRegisters(m_jit.codeBlock());
-        } else {
+        } else if (node->op() == CallEval) {
+            // CallEval is handled with the arguments in the stack
             m_jit.store32(MacroAssembler::TrustedImm32(numPassedArgs), JITCompiler::calleeFramePayloadSlot(CallFrameSlot::argumentCount));
 
@@ -879 +906 @@
             for (unsigned i = numPassedArgs; i < numAllocatedArgs; ++i)
                 m_jit.storeTrustedValue(jsUndefined(), JITCompiler::calleeArgumentSlot(i));
+
+            incrementCounter(&m_jit, VM::CallEval);
+        } else {
+            for (unsigned i = numPassedArgs; i-- > 0;) {
+                GPRReg platformArgGPR = argumentRegisterForFunctionArgument(i);
+                Edge argEdge = m_jit.graph().m_varArgChildren[node->firstChild() + 1 + i];
+                JSValueOperand arg(this, argEdge, platformArgGPR);
+                GPRReg argGPR = arg.gpr();
+                ASSERT(argGPR == platformArgGPR || platformArgGPR == InvalidGPRReg);
+
+                // Only free the non-argument registers at this point.
+                if (platformArgGPR == InvalidGPRReg) {
+                    use(argEdge);
+                    m_jit.store64(argGPR, JITCompiler::calleeArgumentSlot(i));
+                }
+            }
+
+            // Use the argument edges for arguments passed in registers.
+            for (unsigned i = numPassedArgs; i-- > 0;) {
+                GPRReg argGPR = argumentRegisterForFunctionArgument(i);
+                if (argGPR != InvalidGPRReg) {
+                    Edge argEdge = m_jit.graph().m_varArgChildren[node->firstChild() + 1 + i];
+                    use(argEdge);
+                }
+            }
+
+            GPRTemporary argCount(this, argumentRegisterForArgumentCount());
+            GPRReg argCountGPR = argCount.gpr();
+            m_jit.move(TrustedImm32(numPassedArgs), argCountGPR);
+            argumentsLocation = argumentsLocationFor(numAllocatedArgs);
+
+            for (unsigned i = numPassedArgs; i < numAllocatedArgs; ++i) {
+                GPRReg platformArgGPR = argumentRegisterForFunctionArgument(i);
+
+                // FIXME: https://bugs.webkit.org/show_bug.cgi?id=165769
+                // Eliminate filling extra stack slots with undefined JSValues for register arguments.
+                // The LLInt thunks aren't smart enough to know what register arguments to spill therefore
+                // we put undefined in both the extra argument register and stack slot.
+                if (platformArgGPR != InvalidGPRReg) {
+                    GPRTemporary argumentTemp(this, platformArgGPR);
+                    m_jit.move(TrustedImm64(JSValue::encode(jsUndefined())), argumentTemp.gpr());
+                }
+                m_jit.storeTrustedValue(jsUndefined(), JITCompiler::calleeArgumentSlot(i));
+            }
         }
     }
@@ -884 +955 @@
     if (!isTail || isVarargs || isForwardVarargs) {
         Edge calleeEdge = m_jit.graph().child(node, 0);
-        JSValueOperand callee(this, calleeEdge);
+        JSValueOperand callee(this, calleeEdge, argumentRegisterForCallee());
         calleeGPR = callee.gpr();
         callee.use();
-        m_jit.store64(calleeGPR, JITCompiler::calleeFrameSlot(CallFrameSlot::callee));
+        if (argumentsLocation == StackArgs)
+            m_jit.store64(calleeGPR, JITCompiler::calleeFrameSlot(CallFrameSlot::callee));
 
         flushRegisters();
@@ -914 +986 @@
     
     CallLinkInfo* callLinkInfo = m_jit.codeBlock()->addCallLinkInfo();
-    callLinkInfo->setUpCall(callType, m_currentNode->origin.semantic, calleeGPR);
+    callLinkInfo->setUpCall(callType, argumentsLocation, m_currentNode->origin.semantic, calleeGPR);
 
     if (node->op() == CallEval) {
@@ -955 +1027 @@
             RELEASE_ASSERT(node->op() == DirectTailCall);
             
+            if (calleeGPRTemporary != std::nullopt)
+                m_jit.move(tailCallee->gpr(), calleeGPRTemporary->gpr());
+
             JITCompiler::PatchableJump patchableJump = m_jit.patchableJump();
             JITCompiler::Label mainPath = m_jit.label();
+
+            incrementCounter(&m_jit, VM::TailCall);
+            incrementCounter(&m_jit, VM::DirectCall);
             
             m_jit.emitStoreCallSiteIndex(callSite);
@@ -972 +1050 @@
             silentFillAllRegisters(InvalidGPRReg);
             m_jit.exceptionCheck();
+            if (calleeGPRTemporary != std::nullopt)
+                m_jit.move(tailCallee->gpr(), calleeGPRTemporary->gpr());
             m_jit.jump().linkTo(mainPath, &m_jit);
             
@@ -982 +1062 @@
         JITCompiler::Label mainPath = m_jit.label();
         
+        incrementCounter(&m_jit, VM::DirectCall);
+
         m_jit.emitStoreCallSiteIndex(callSite);
         
@@ -989 +1071 @@
         JITCompiler::Label slowPath = m_jit.label();
         if (isX86())
-            m_jit.pop(JITCompiler::selectScratchGPR(calleeGPR));
-
-        callOperation(operationLinkDirectCall, callLinkInfo, calleeGPR);
+            m_jit.pop(GPRInfo::nonArgGPR0);
+
+        m_jit.move(MacroAssembler::TrustedImmPtr(callLinkInfo), GPRInfo::nonArgGPR0); // Link info needs to be in nonArgGPR0
+        JITCompiler::Call slowCall = m_jit.nearCall();
+
         m_jit.exceptionCheck();
         m_jit.jump().linkTo(mainPath, &m_jit);
@@ -998 +1082 @@
         
         setResultAndResetStack();
-        
-        m_jit.addJSDirectCall(call, slowPath, callLinkInfo);
+
+        m_jit.addJSDirectCall(call, slowCall, slowPath, callLinkInfo);
         return;
     }
-    
+
+    if (isTail && calleeGPRTemporary != std::nullopt)
+        m_jit.move(tailCallee->gpr(), calleeGPRTemporary->gpr());
+
     m_jit.emitStoreCallSiteIndex(callSite);
     
@@ -1026 +1113 @@
     if (node->op() == TailCall) {
         CallFrameShuffler callFrameShuffler(m_jit, shuffleData);
-        callFrameShuffler.setCalleeJSValueRegs(JSValueRegs(GPRInfo::regT0));
+        if (argumentsLocation == StackArgs)
+            callFrameShuffler.setCalleeJSValueRegs(JSValueRegs(argumentRegisterForCallee()));
         callFrameShuffler.prepareForSlowPath();
-    } else {
-        m_jit.move(calleeGPR, GPRInfo::regT0); // Callee needs to be in regT0
-
-        if (isTail)
-            m_jit.emitRestoreCalleeSaves(); // This needs to happen after we moved calleeGPR to regT0
-    }
-
-    m_jit.move(MacroAssembler::TrustedImmPtr(callLinkInfo), GPRInfo::regT2); // Link info needs to be in regT2
+    } else if (isTail)
+        m_jit.emitRestoreCalleeSaves();
+
+    m_jit.move(MacroAssembler::TrustedImmPtr(callLinkInfo), GPRInfo::nonArgGPR0); // Link info needs to be in nonArgGPR0
     JITCompiler::Call slowCall = m_jit.nearCall();
 
     done.link(&m_jit);
 
-    if (isTail)
+    if (isTail) {
+        tailCallee = std::nullopt;
+        calleeGPRTemporary = std::nullopt;
         m_jit.abortWithReason(JITDidReturnFromTailCall);
-    else
+    } else
         setResultAndResetStack();
 
@@ -4167 +4253 @@
     }
 
+    case GetArgumentRegister:
+        break;
+            
     case GetRestLength: {
         compileGetRestLength(node);