Rationalize the makeSpaceForCCall stuff
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2016 18:38:15 +0000 (18:38 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 7 Apr 2016 18:38:15 +0000 (18:38 +0000)
https://bugs.webkit.org/show_bug.cgi?id=156352

Reviewed by Mark Lam.

I want to add more code to PolymorphicAccess that makes C calls, so that I can finally fix
https://bugs.webkit.org/show_bug.cgi?id=130914 (allow transition caches to handle indexing
headers).

When trying to understand what it takes to make a C call, I came across code that was making
room on the stack for spilled arguments. This logic was guarded with some complicated
condition. At first, I tried to just refactor the code so that the same ugly condition
wouldn't have to be copy-pasted everywhere that we made C calls. But then I started thinking
about the condition, and realized that it was probably wrong: if the outer PolymorphicAccess
harness decides to reuse a register for the scratchGPR then the top of the stack will store
the old value of scratchGPR, but the condition wouldn't necessarily trigger. So if the call
then overwrote something on the stack, we'd have a bad time.

Making room on the stack for a call is a cheap operation. It's orders of magnitude cheaper
than the rest of the call. Therefore, I think that it's best to just unconditionally make
room on the stack.

This patch makes us do just that. I also made the relevant helpers not inline, because I
think that we have too many inline methods in our assemblers. Now it's much easier to make
C calls from PolymorphicAccess because you just call the AssemblyHelper methods for making
space. There are no special conditions or anything like that.

* bytecode/PolymorphicAccess.cpp:
(JSC::AccessCase::generate):
* jit/AssemblyHelpers.cpp:
(JSC::AssemblyHelpers::emitLoadStructure):
(JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
(JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
(JSC::emitRandomThunkImpl):
* jit/AssemblyHelpers.h:
(JSC::AssemblyHelpers::makeSpaceOnStackForCCall): Deleted.
(JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall): Deleted.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@199166 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/PolymorphicAccess.cpp
Source/JavaScriptCore/jit/AssemblyHelpers.cpp
Source/JavaScriptCore/jit/AssemblyHelpers.h

index 098e875..23b03bc 100644 (file)
@@ -1,3 +1,43 @@
+2016-04-07  Filip Pizlo  <fpizlo@apple.com>
+
+        Rationalize the makeSpaceForCCall stuff
+        https://bugs.webkit.org/show_bug.cgi?id=156352
+
+        Reviewed by Mark Lam.
+
+        I want to add more code to PolymorphicAccess that makes C calls, so that I can finally fix
+        https://bugs.webkit.org/show_bug.cgi?id=130914 (allow transition caches to handle indexing
+        headers).
+
+        When trying to understand what it takes to make a C call, I came across code that was making
+        room on the stack for spilled arguments. This logic was guarded with some complicated
+        condition. At first, I tried to just refactor the code so that the same ugly condition
+        wouldn't have to be copy-pasted everywhere that we made C calls. But then I started thinking
+        about the condition, and realized that it was probably wrong: if the outer PolymorphicAccess
+        harness decides to reuse a register for the scratchGPR then the top of the stack will store
+        the old value of scratchGPR, but the condition wouldn't necessarily trigger. So if the call
+        then overwrote something on the stack, we'd have a bad time.
+
+        Making room on the stack for a call is a cheap operation. It's orders of magnitude cheaper
+        than the rest of the call. Therefore, I think that it's best to just unconditionally make
+        room on the stack.
+
+        This patch makes us do just that. I also made the relevant helpers not inline, because I
+        think that we have too many inline methods in our assemblers. Now it's much easier to make
+        C calls from PolymorphicAccess because you just call the AssemblyHelper methods for making
+        space. There are no special conditions or anything like that.
+
+        * bytecode/PolymorphicAccess.cpp:
+        (JSC::AccessCase::generate):
+        * jit/AssemblyHelpers.cpp:
+        (JSC::AssemblyHelpers::emitLoadStructure):
+        (JSC::AssemblyHelpers::makeSpaceOnStackForCCall):
+        (JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall):
+        (JSC::emitRandomThunkImpl):
+        * jit/AssemblyHelpers.h:
+        (JSC::AssemblyHelpers::makeSpaceOnStackForCCall): Deleted.
+        (JSC::AssemblyHelpers::reclaimSpaceOnStackForCCall): Deleted.
+
 2016-04-07  Commit Queue  <commit-queue@webkit.org>
 
         Unreviewed, rolling out r199128 and r199141.
index c51af8b..6076baf 100644 (file)
@@ -892,7 +892,7 @@ void AccessCase::generate(AccessGenerationState& state)
 
             done.link(&jit);
 
-            jit.addPtr(CCallHelpers::TrustedImm32((jit.codeBlock()->stackPointerOffset() * sizeof(Register)) - state.preservedReusedRegisterState.numberOfBytesPreserved - state.numberOfStackBytesUsedForRegisterPreservation()),
+            jit.addPtr(CCallHelpers::TrustedImm32((codeBlock->stackPointerOffset() * sizeof(Register)) - state.preservedReusedRegisterState.numberOfBytesPreserved - state.numberOfStackBytesUsedForRegisterPreservation()),
                 GPRInfo::callFrameRegister, CCallHelpers::stackPointerRegister);
             state.restoreLiveRegistersFromStackForCall(isGetter());
 
@@ -908,12 +908,10 @@ void AccessCase::generate(AccessGenerationState& state)
                         CodeLocationLabel(vm.getCTIStub(linkCallThunkGenerator).code()));
                 });
         } else {
-            // Need to make room for the C call so any of our stack spillage isn't overwritten.
-            // We also need to make room because we may be an inline cache in the FTL and not
-            // have a JIT call frame.
-            bool needsToMakeRoomOnStackForCCall = state.numberOfStackBytesUsedForRegisterPreservation() || codeBlock->jitType() == JITCode::FTLJIT;
-            if (needsToMakeRoomOnStackForCCall)
-                jit.makeSpaceOnStackForCCall();
+            // Need to make room for the C call so any of our stack spillage isn't overwritten. It's
+            // hard to track if someone did spillage or not, so we just assume that we always need
+            // to make some space here.
+            jit.makeSpaceOnStackForCCall();
 
             // getter: EncodedJSValue (*GetValueFunc)(ExecState*, EncodedJSValue thisValue, PropertyName);
             // setter: void (*PutValueFunc)(ExecState*, EncodedJSValue thisObject, EncodedJSValue value);
@@ -944,8 +942,7 @@ void AccessCase::generate(AccessGenerationState& state)
             operationCall = jit.call();
             if (m_type == CustomValueGetter || m_type == CustomAccessorGetter)
                 jit.setupResults(valueRegs);
-            if (needsToMakeRoomOnStackForCCall)
-                jit.reclaimSpaceOnStackForCCall();
+            jit.reclaimSpaceOnStackForCCall();
 
             CCallHelpers::Jump noException =
                 jit.emitExceptionCheck(CCallHelpers::InvertedExceptionCheck);
index 3b7be1e..21b9e8e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, 2013-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2013-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -455,6 +455,20 @@ void AssemblyHelpers::emitLoadStructure(RegisterID source, RegisterID dest, Regi
 #endif
 }
 
+void AssemblyHelpers::makeSpaceOnStackForCCall()
+{
+    unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+    if (stackOffset)
+        subPtr(TrustedImm32(stackOffset), stackPointerRegister);
+}
+
+void AssemblyHelpers::reclaimSpaceOnStackForCCall()
+{
+    unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
+    if (stackOffset)
+        addPtr(TrustedImm32(stackOffset), stackPointerRegister);
+}
+
 #if USE(JSVALUE64)
 template<typename LoadFromHigh, typename StoreToHigh, typename LoadFromLow, typename StoreToLow>
 void emitRandomThunkImpl(AssemblyHelpers& jit, GPRReg scratch0, GPRReg scratch1, GPRReg scratch2, FPRReg result, const LoadFromHigh& loadFromHigh, const StoreToHigh& storeToHigh, const LoadFromLow& loadFromLow, const StoreToLow& storeToLow)
index 81a245c..7732756 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2011, 2013-2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2011, 2013-2016 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -1325,19 +1325,8 @@ public:
 
     Vector<BytecodeAndMachineOffset>& decodedCodeMapFor(CodeBlock*);
 
-    void makeSpaceOnStackForCCall()
-    {
-        unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
-        if (stackOffset)
-            subPtr(TrustedImm32(stackOffset), stackPointerRegister);
-    }
-
-    void reclaimSpaceOnStackForCCall()
-    {
-        unsigned stackOffset = WTF::roundUpToMultipleOf(stackAlignmentBytes(), maxFrameExtentForSlowPathCall);
-        if (stackOffset)
-            addPtr(TrustedImm32(stackOffset), stackPointerRegister);
-    }
+    void makeSpaceOnStackForCCall();
+    void reclaimSpaceOnStackForCCall();
 
 #if USE(JSVALUE64)
     void emitRandomThunk(JSGlobalObject*, GPRReg scratch0, GPRReg scratch1, GPRReg scratch2, FPRReg result);