Changeset 268783 in webkit

Timestamp:

Oct 20, 2020, 9:00:05 PM (6 years ago)

Author:

sbarati@apple.com

Message:

Don't OSR exit to bc#0 for FTL argument type checks during loop OSR entry
​https://bugs.webkit.org/show_bug.cgi?id=217925
<rdar://problem/70369407>

Reviewed by Michael Saboff and Tadeu Zagallo.

JSTests:

• stress/ftl-osr-entry-should-not-exit-to-bc-zero.js: Added.

Source/JavaScriptCore:

When the FTL was emitting type checks for the named arguments of a function,
it was always emitting these type checks with an exit origin of bc#0. It was
doing this even if we were an OSR entry compilation! This meant that type
checks for arguments that failed during loop OSR entry would incorrectly exit
back to bc#0.

This patch fixes this by having the OSR entry runtime code validate the
argument types before OSR entering. The current OSR entry compiled code in
the FTL is designed to only allow exiting after all ExtractOSREntryLocal and
MovHints have executed, so it is simpler to put the type checks in the runtime
instead of the compiled code.

This patch also makes it so we do exponential backoff when failing to OSR
enter. This is needed due to insufficient profiling where we never properly
profile the type of arguments. Before this, we'd OSR exit in the FTL code
itself, which does exponential backoff when recompiling. This patch builds
this same exponential backoff in for when we fail to OSR enter enough times
to give up on the OSR entry compilation.

• ftl/FTLForOSREntryJITCode.h:
• ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::lower):

• ftl/FTLOSREntry.cpp:

(JSC::FTL::prepareOSREntry):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/ftl-osr-entry-should-not-exit-to-bc-zero.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGOperations.cpp (modified) (4 diffs)
• Source/JavaScriptCore/ftl/FTLForOSREntryJITCode.h (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (1 diff)
• Source/JavaScriptCore/ftl/FTLOSREntry.cpp (modified) (1 diff)

trunk/JSTests/ChangeLog

@@ +1 @@
+2020-10-20  Saam Barati  <sbarati@apple.com>
+
+        Don't OSR exit to bc#0 for FTL argument type checks during loop OSR entry
+        https://bugs.webkit.org/show_bug.cgi?id=217925
+        <rdar://problem/70369407>
+
+        Reviewed by Michael Saboff and Tadeu Zagallo.
+
+        * stress/ftl-osr-entry-should-not-exit-to-bc-zero.js: Added.
+
 2020-10-20  Michael Saboff  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2020-10-20  Saam Barati  <sbarati@apple.com>
+
+        Don't OSR exit to bc#0 for FTL argument type checks during loop OSR entry
+        https://bugs.webkit.org/show_bug.cgi?id=217925
+        <rdar://problem/70369407>
+
+        Reviewed by Michael Saboff and Tadeu Zagallo.
+
+        When the FTL was emitting type checks for the named arguments of a function,
+        it was always emitting these type checks with an exit origin of bc#0. It was
+        doing this even if we were an OSR entry compilation! This meant that type
+        checks for arguments that failed during loop OSR entry would incorrectly exit
+        back to bc#0.
+        
+        This patch fixes this by having the OSR entry runtime code validate the
+        argument types before OSR entering. The current OSR entry compiled code in
+        the FTL is designed to only allow exiting after all ExtractOSREntryLocal and
+        MovHints have executed, so it is simpler to put the type checks in the runtime
+        instead of the compiled code.
+        
+        This patch also makes it so we do exponential backoff when failing to OSR
+        enter. This is needed due to insufficient profiling where we never properly
+        profile the type of arguments. Before this, we'd OSR exit in the FTL code
+        itself, which does exponential backoff when recompiling. This patch builds
+        this same exponential backoff in for when we fail to OSR enter enough times
+        to give up on the OSR entry compilation.
+
+        * ftl/FTLForOSREntryJITCode.h:
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::lower):
+        * ftl/FTLOSREntry.cpp:
+        (JSC::FTL::prepareOSREntry):
+
 2020-10-20  Michael Saboff  <msaboff@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGOperations.cpp

@@ -3769 +3769 @@
     }
 
+    auto failedOSREntry = [&] (CodeBlock* entryBlock) {
+        FTL::ForOSREntryJITCode* entryCode = entryBlock->jitCode()->ftlForOSREntry();
+        entryCode->countEntryFailure();
+        if (entryCode->entryFailureCount() <
+            Options::ftlOSREntryFailureCountForReoptimization()) {
+            CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry failed"));
+            jitCode->setOptimizationThresholdBasedOnCompilationResult(
+                codeBlock, CompilationDeferred);
+            return nullptr;
+        }
+
+        CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry failed too many times"));
+        codeBlock->baselineVersion()->countReoptimization();
+        jitCode->clearOSREntryBlockAndResetThresholds(codeBlock);
+        return nullptr;
+    };
+
     // If we can OSR Enter, do it right away.
     if (canOSREnterHere) {
@@ -3782 +3799 @@
                     return tagCodePtrWithStackPointerForJITCall(untagCodePtr<char*, JSEntryPtrTag>(address), callFrame);
                 }
+
+                return failedOSREntry(entryBlock);
             }
         }
@@ -3823 +3842 @@
         }
 
-        FTL::ForOSREntryJITCode* entryCode = entryBlock->jitCode()->ftlForOSREntry();
-        entryCode->countEntryFailure();
-        if (entryCode->entryFailureCount() <
-            Options::ftlOSREntryFailureCountForReoptimization()) {
-            CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry failed"));
-            jitCode->setOptimizationThresholdBasedOnCompilationResult(
-                codeBlock, CompilationDeferred);
-            return nullptr;
-        }
-
-        // OSR entry failed. Oh no! This implies that we need to retry. We retry
-        // without exponential backoff and we only do this for the entry code block.
-        CODEBLOCK_LOG_EVENT(codeBlock, "delayFTLCompile", ("OSR entry failed too many times"));
-        jitCode->clearOSREntryBlockAndResetThresholds(codeBlock);
-        return nullptr;
+        return failedOSREntry(entryBlock);
     }
 
@@ -3934 +3939 @@
     void* address = FTL::prepareOSREntry(vm, callFrame, codeBlock, jitCode->osrEntryBlock(), originBytecodeIndex, streamIndex);
     if (!address)
-        return nullptr;
+        return failedOSREntry(jitCode->osrEntryBlock());
     return tagCodePtrWithStackPointerForJITCall(untagCodePtr<char*, JSEntryPtrTag>(address), callFrame);
 }

trunk/Source/JavaScriptCore/ftl/FTLForOSREntryJITCode.h

@@ -56 +56 @@
     
     ForOSREntryJITCode* ftlForOSREntry() final;
-    
+    Vector<DFG::FlushFormat>& argumentFlushFormats() { return m_argumentFlushFormats; }
+
 private:
+    Vector<DFG::FlushFormat> m_argumentFlushFormats;
     ScratchBuffer* m_entryBuffer; // Only for OSR entry code blocks.
     BytecodeIndex m_bytecodeIndex;

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -359 +359 @@
             }
 
-            for (unsigned i = codeBlock()->numParameters(); i--;) {
-                MethodOfGettingAValueProfile profile(&m_graph.m_profiledBlock->valueProfileForArgument(i));
-                VirtualRegister operand = virtualRegisterForArgumentIncludingThis(i);
-                LValue jsValue = m_out.load64(addressFor(operand));
-                
-                switch (m_graph.m_argumentFormats[0][i]) {
-                case FlushedInt32:
-                    speculate(BadType, jsValueValue(jsValue), profile, isNotInt32(jsValue));
-                    break;
-                case FlushedBoolean:
-                    speculate(BadType, jsValueValue(jsValue), profile, isNotBoolean(jsValue));
-                    break;
-                case FlushedCell:
-                    speculate(BadType, jsValueValue(jsValue), profile, isNotCell(jsValue));
-                    break;
-                case FlushedJSValue:
-                    break;
-                default:
-                    DFG_CRASH(m_graph, nullptr, "Bad flush format for argument");
-                    break;
+            if (m_graph.m_plan.mode() == FTLForOSREntryMode) {
+                auto* jitCode = m_ftlState.jitCode->ftlForOSREntry();
+                jitCode->argumentFlushFormats().reserveInitialCapacity(codeBlock()->numParameters());
+                for (unsigned i = codeBlock()->numParameters(); i--;)
+                    jitCode->argumentFlushFormats().append(m_graph.m_argumentFormats[0][i]);
+            } else {
+                for (unsigned i = codeBlock()->numParameters(); i--;) {
+                    MethodOfGettingAValueProfile profile(&m_graph.m_profiledBlock->valueProfileForArgument(i));
+                    VirtualRegister operand = virtualRegisterForArgumentIncludingThis(i);
+                    LValue jsValue = m_out.load64(addressFor(operand));
+                    
+                    switch (m_graph.m_argumentFormats[0][i]) {
+                    case FlushedInt32:
+                        speculate(BadType, jsValueValue(jsValue), profile, isNotInt32(jsValue));
+                        break;
+                    case FlushedBoolean:
+                        speculate(BadType, jsValueValue(jsValue), profile, isNotBoolean(jsValue));
+                        break;
+                    case FlushedCell:
+                        speculate(BadType, jsValueValue(jsValue), profile, isNotCell(jsValue));
+                        break;
+                    case FlushedJSValue:
+                        break;
+                    default:
+                        DFG_CRASH(m_graph, nullptr, "Bad flush format for argument");
+                        break;
+                    }
                 }
             }
+
             m_out.jump(firstDFGBasicBlock);
         }

trunk/Source/JavaScriptCore/ftl/FTLOSREntry.cpp

@@ -75 +75 @@
         JSValue valueOnStack = callFrame->r(virtualRegisterForArgumentIncludingThis(argument)).asanUnsafeJSValue();
         Optional<JSValue> reconstructedValue = values.argument(argument);
+        {
+            JSValue valueToValidate = reconstructedValue ? *reconstructedValue : valueOnStack;
+            auto flushFormat = entryCode->argumentFlushFormats()[argument];
+            switch (flushFormat) {
+            case DFG::FlushedInt32:
+                if (!valueToValidate.isInt32())
+                    return nullptr;
+                break;
+            case DFG::FlushedBoolean:
+                if (!valueToValidate.isBoolean())
+                    return nullptr;
+                break;
+            case DFG::FlushedCell:
+                if (!valueToValidate.isCell())
+                    return nullptr;
+                break;
+            case DFG::FlushedJSValue:
+                break;
+            default:
+                dataLogLn("Unknown flush format for argument during FTL osr entry: ", flushFormat);
+                RELEASE_ASSERT_NOT_REACHED();
+                break;
+            }
+        }
+
         if (!argument) {
             // |this| argument can be unboxed. We should store boxed value instead for loop OSR entry since FTL assumes that all arguments are flushed JSValue.