Get rid of InlineStart so that I don't have to implement it in FTL
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Oct 2013 18:03:38 +0000 (18:03 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 28 Oct 2013 18:03:38 +0000 (18:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123302

Reviewed by Geoffrey Garen.

InlineStart was a special instruction that we would insert at the top of inlined code,
so that the backend could capture the OSR state of arguments to an inlined call. It used
to be that only the backend had this information, so this instruction was sort of an ugly
callback from the backend for filling in some data structures.

But in the time since when that code was written (two years ago?), we rationalized how
variables work. It's now the case that variables that the runtime must know about are
treated specially in IR (they are "flushed") and we know how we will represent them even
before we get to the backend. The last place that makes changes to their representation
is the StackLayoutPhase.

So, this patch gets rid of InlineStart, but keeps around the special meta-data that the
instruction had. Instead of handling the bookkeeping in the backend, we handle it in
StackLayoutPhase. This means that the DFG and FTL can share code for handling this
bookkeeping. This also means that now the FTL can compile code blocks that had inlining.

Of course, giving the FTL the ability to handle code blocks that had inlining means that
we're going to have new bugs. Sure enough, the FTL's linker didn't handle inline call
frames. This patch also fixes that.

* dfg/DFGAbstractInterpreterInlines.h:
(JSC::DFG::::executeEffects):
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleInlining):
(JSC::DFG::ByteCodeParser::InlineStackEntry::InlineStackEntry):
* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGFixupPhase.cpp:
(JSC::DFG::FixupPhase::fixupNode):
* dfg/DFGGraph.h:
* dfg/DFGNode.h:
* dfg/DFGNodeType.h:
* dfg/DFGPredictionPropagationPhase.cpp:
(JSC::DFG::PredictionPropagationPhase::propagate):
* dfg/DFGSafeToExecute.h:
(JSC::DFG::safeToExecute):
* dfg/DFGSpeculativeJIT.cpp:
* dfg/DFGSpeculativeJIT.h:
* dfg/DFGSpeculativeJIT32_64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGSpeculativeJIT64.cpp:
(JSC::DFG::SpeculativeJIT::compile):
* dfg/DFGStackLayoutPhase.cpp:
(JSC::DFG::StackLayoutPhase::run):
* ftl/FTLLink.cpp:
(JSC::FTL::link):

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

16 files changed:
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
Source/JavaScriptCore/dfg/DFGGraph.h
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
Source/JavaScriptCore/dfg/DFGSafeToExecute.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp
Source/JavaScriptCore/ftl/FTLLink.cpp

index fdafd12d85fc1bed42428ac7d2d35ca0ca979b60..e046c45c583dc5b6c6c8b4a05a7d3304b70dcc14 100644 (file)
@@ -1,3 +1,57 @@
+2013-10-24  Filip Pizlo  <fpizlo@apple.com>
+
+        Get rid of InlineStart so that I don't have to implement it in FTL
+        https://bugs.webkit.org/show_bug.cgi?id=123302
+
+        Reviewed by Geoffrey Garen.
+        
+        InlineStart was a special instruction that we would insert at the top of inlined code,
+        so that the backend could capture the OSR state of arguments to an inlined call. It used
+        to be that only the backend had this information, so this instruction was sort of an ugly
+        callback from the backend for filling in some data structures.
+        
+        But in the time since when that code was written (two years ago?), we rationalized how
+        variables work. It's now the case that variables that the runtime must know about are
+        treated specially in IR (they are "flushed") and we know how we will represent them even
+        before we get to the backend. The last place that makes changes to their representation
+        is the StackLayoutPhase.
+        
+        So, this patch gets rid of InlineStart, but keeps around the special meta-data that the
+        instruction had. Instead of handling the bookkeeping in the backend, we handle it in
+        StackLayoutPhase. This means that the DFG and FTL can share code for handling this
+        bookkeeping. This also means that now the FTL can compile code blocks that had inlining.
+        
+        Of course, giving the FTL the ability to handle code blocks that had inlining means that
+        we're going to have new bugs. Sure enough, the FTL's linker didn't handle inline call
+        frames. This patch also fixes that.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::::executeEffects):
+        * dfg/DFGByteCodeParser.cpp:
+        (JSC::DFG::ByteCodeParser::handleInlining):
+        (JSC::DFG::ByteCodeParser::InlineStackEntry::InlineStackEntry):
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode):
+        * dfg/DFGGraph.h:
+        * dfg/DFGNode.h:
+        * dfg/DFGNodeType.h:
+        * dfg/DFGPredictionPropagationPhase.cpp:
+        (JSC::DFG::PredictionPropagationPhase::propagate):
+        * dfg/DFGSafeToExecute.h:
+        (JSC::DFG::safeToExecute):
+        * dfg/DFGSpeculativeJIT.cpp:
+        * dfg/DFGSpeculativeJIT.h:
+        * dfg/DFGSpeculativeJIT32_64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGSpeculativeJIT64.cpp:
+        (JSC::DFG::SpeculativeJIT::compile):
+        * dfg/DFGStackLayoutPhase.cpp:
+        (JSC::DFG::StackLayoutPhase::run):
+        * ftl/FTLLink.cpp:
+        (JSC::FTL::link):
+
 2013-10-24  Filip Pizlo  <fpizlo@apple.com>
 
         The GetById->GetByOffset AI-based optimization should actually do things
index 7bc3b8c4e6be0c5f0ad07a8e0fd6b867b31af2d1..d008a92760c7bb6f7d77083920433d0ff62a46f5 100644 (file)
@@ -1541,7 +1541,6 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         break;
             
     case Phantom:
-    case InlineStart:
     case CountExecution:
     case CheckTierUpInLoop:
     case CheckTierUpAtReturn:
index 934d0a7cdc1b367031b1cd5cadf45151f4684b75..2e82417471844806326da71deae6d5a0ce8e4831 100644 (file)
@@ -1304,11 +1304,11 @@ bool ByteCodeParser::handleInlining(Node* callTargetNode, int resultOperand, con
     unsigned oldIndex = m_currentIndex;
     m_currentIndex = 0;
 
-    InlineStartData* inlineStartData = &m_graph.m_inlineStartData.alloc();
-    inlineStartData->argumentPositionStart = argumentPositionStart;
-    inlineStartData->calleeVariable = 0;
+    InlineVariableData inlineVariableData;
+    inlineVariableData.inlineCallFrame = m_inlineStackTop->m_inlineCallFrame;
+    inlineVariableData.argumentPositionStart = argumentPositionStart;
+    inlineVariableData.calleeVariable = 0;
     
-    addToGraph(InlineStart, OpInfo(inlineStartData));
     RELEASE_ASSERT(
         m_inlineStackTop->m_inlineCallFrame->isClosureCall
         == callLinkStatus.isClosureCall());
@@ -1321,9 +1321,11 @@ bool ByteCodeParser::handleInlining(Node* callTargetNode, int resultOperand, con
         calleeVariable->mergeShouldNeverUnbox(true);
         scopeVariable->mergeShouldNeverUnbox(true);
         
-        inlineStartData->calleeVariable = calleeVariable;
+        inlineVariableData.calleeVariable = calleeVariable;
     }
     
+    m_graph.m_inlineVariableData.append(inlineVariableData);
+    
     parseCodeBlock();
     
     m_currentIndex = oldIndex;
@@ -3370,7 +3372,7 @@ ByteCodeParser::InlineStackEntry::InlineStackEntry(
             m_inlineCallFrame->executable,
             byteCodeParser->m_codeBlock,
             m_inlineCallFrame,
-            byteCodeParser->m_codeBlock->ownerExecutable(), 
+            byteCodeParser->m_codeBlock->ownerExecutable(),
             codeBlock->ownerExecutable());
         m_inlineCallFrame->stackOffset = inlineCallFrameStart.offset() - JSStack::CallFrameHeaderSize;
         if (callee) {
index d968fe6876fe9b8f270e4db7fff134e2294c497c..463c3ffc27b4d2c900982d947006d4647f57eb93 100644 (file)
@@ -124,7 +124,6 @@ void clobberize(Graph& graph, Node* node, ReadFunctor& read, WriteFunctor& write
     case Flush:
     case PhantomLocal:
     case SetArgument:
-    case InlineStart:
     case Breakpoint:
     case PhantomArguments:
     case Jump:
index f77fd5933246d9d6cd48a91acd52f4214350137f..a94c0fa078526e9ea93dd80036eff5b39b08e688 100644 (file)
@@ -907,7 +907,6 @@ private:
         case Flush:
         case PhantomLocal:
         case GetLocalUnlinked:
-        case InlineStart:
         case GetMyScope:
         case GetClosureVar:
         case GetGlobalVar:
index 2f1a0241c17d6c999ee3cf038486d2252e4134f8..6a73377cf00e82ffbb5597902a745c942c29a702 100644 (file)
@@ -61,6 +61,12 @@ struct StorageAccessData {
     unsigned identifierNumber;
 };
 
+struct InlineVariableData {
+    InlineCallFrame* inlineCallFrame;
+    unsigned argumentPositionStart;
+    VariableAccessData* calleeVariable;
+};
+
 enum AddSpeculationMode {
     DontSpeculateInt32,
     SpeculateInt32AndTruncateConstants,
@@ -789,7 +795,7 @@ public:
     SegmentedVector<StructureTransitionData, 8> m_structureTransitionData;
     SegmentedVector<NewArrayBufferData, 4> m_newArrayBufferData;
     SegmentedVector<SwitchData, 4> m_switchData;
-    SegmentedVector<InlineStartData, 4> m_inlineStartData;
+    Vector<InlineVariableData, 4> m_inlineVariableData;
     OwnPtr<InlineCallFrameSet> m_inlineCallFrames;
     bool m_hasArguments;
     HashSet<ExecutableBase*> m_executablesWhoseArgumentsEscaped;
index 8cecf8ee2bd335c7ad467280615254952bec3d84..980452a165201e7bea78847c9313238ee270fccc 100644 (file)
@@ -139,11 +139,6 @@ struct SwitchData {
     bool didUseJumpTable;
 };
 
-struct InlineStartData {
-    unsigned argumentPositionStart;
-    VariableAccessData* calleeVariable;
-};
-
 // This type used in passing an immediate argument to Node constructor;
 // distinguishes an immediate value (typically an index into a CodeBlock data structure - 
 // a constant index, argument, or identifier) from a Node*.
@@ -1124,17 +1119,6 @@ struct Node {
         m_virtualRegister = virtualRegister;
     }
     
-    bool hasInlineStartData()
-    {
-        return op() == InlineStart;
-    }
-    
-    InlineStartData* inlineStartData()
-    {
-        ASSERT(hasInlineStartData());
-        return bitwise_cast<InlineStartData*>(m_opInfo);
-    }
-    
     bool hasExecutionCounter()
     {
         return op() == CountExecution;
index e0399b2a808df49502a4af7133643e84724b223a..4048d5d75fc1bece3354dea7aee1a90118d3bab7 100644 (file)
@@ -91,11 +91,6 @@ namespace JSC { namespace DFG {
     /* Marker for an argument being set at the prologue of a function. */\
     macro(SetArgument, 0 | NodeDoesNotExit) \
     \
-    /* Hint that inlining begins here. No code is generated for this node. It's only */\
-    /* used for copying OSR data into inline frame data, to support reification of */\
-    /* call frames of inlined functions. */\
-    macro(InlineStart, NodeMustGenerate | NodeDoesNotExit) \
-    \
     /* Nodes for bitwise operations. */\
     macro(BitAnd, NodeResultInt32 | NodeMustGenerate) \
     macro(BitOr, NodeResultInt32 | NodeMustGenerate) \
index 395578fe824b537885435abd9542270ddfff3a96..fc0954f3adeed4e2a6555764e4f7bae36b58ba9e 100644 (file)
@@ -586,7 +586,6 @@ private:
             break;
             
         // These gets ignored because it doesn't do anything.
-        case InlineStart:
         case CountExecution:
         case PhantomLocal:
         case Flush:
index 34684368bde049d2cc8aeb780c6b8040f05261c4..77cf7406f711bf4a8cf3464ed55cdb415fc14998 100644 (file)
@@ -129,7 +129,6 @@ bool safeToExecute(AbstractStateType& state, Graph& graph, Node* node)
     case PhantomLocal:
     case GetLocalUnlinked:
     case SetArgument:
-    case InlineStart:
     case BitAnd:
     case BitOr:
     case BitXor:
index 373dc44ad470685e1136b008845ae0a60907dbc0..06107d5e9df7a5c46eab1ba36450983cd0cbdff8 100644 (file)
@@ -1557,37 +1557,6 @@ void SpeculativeJIT::compileMovHintAndCheck(Node* node)
     noResult(node);
 }
 
-void SpeculativeJIT::compileInlineStart(Node* node)
-{
-    InlineCallFrame* inlineCallFrame = node->codeOrigin.inlineCallFrame;
-    InlineStartData* data = node->inlineStartData();
-    int argumentCountIncludingThis = inlineCallFrame->arguments.size();
-    for (int i = 0; i < argumentCountIncludingThis; ++i) {
-        ArgumentPosition& position = m_jit.graph().m_argumentPositions[
-            data->argumentPositionStart + i];
-        VariableAccessData* variable = position.someVariable();
-        ValueSource source;
-        if (!variable)
-            source = ValueSource(SourceIsDead);
-        else {
-            source = ValueSource::forFlushFormat(
-                variable->machineLocal(),
-                m_jit.graph().m_argumentPositions[data->argumentPositionStart + i].flushFormat());
-        }
-        inlineCallFrame->arguments[i] = source.valueRecovery();
-    }
-    
-    RELEASE_ASSERT(inlineCallFrame->isClosureCall == !!data->calleeVariable);
-    
-    if (inlineCallFrame->isClosureCall) {
-        ValueSource source = ValueSource::forFlushFormat(
-            data->calleeVariable->machineLocal(),
-            data->calleeVariable->flushFormat());
-        inlineCallFrame->calleeRecovery = source.valueRecovery();
-    } else
-        RELEASE_ASSERT(inlineCallFrame->calleeRecovery.isConstant());
-}
-
 void SpeculativeJIT::bail()
 {
     m_compileOkay = true;
index b730034a0375d31d5c6a48e4cda8f6a53340ceb6..dd2a8ba784878a170b82c56750a36e7eb8f8cc98 100644 (file)
@@ -707,7 +707,6 @@ public:
     
     void compileMovHint(Node*);
     void compileMovHintAndCheck(Node*);
-    void compileInlineStart(Node*);
 
     void nonSpeculativeUInt32ToNumber(Node*);
 
index d29f0e2fea1a2bcddc0b12cf4f1168b0d6d70995..dc4ef5e707c420a1b2dd6e8af8b26c8e8012a214 100644 (file)
@@ -1958,11 +1958,6 @@ void SpeculativeJIT::compile(Node* node)
         break;
     }
         
-    case InlineStart: {
-        compileInlineStart(node);
-        break;
-    }
-
     case MovHint:
     case ZombieHint: {
         RELEASE_ASSERT_NOT_REACHED();
index bcab6b4d6e1baabdc04d71c090e2d4b2ba8e14c4..faf8c778e663280d2606920c371cdc6ec279f18c 100644 (file)
@@ -2269,11 +2269,6 @@ void SpeculativeJIT::compile(Node* node)
         break;
     }
         
-    case InlineStart: {
-        compileInlineStart(node);
-        break;
-    }
-
     case MovHint:
     case ZombieHint: {
         RELEASE_ASSERT_NOT_REACHED();
index 1cdd217c6a29b6148001f3617d23e3fcd3c90089..b228183ecde47d2772cbdefd60d7907b7ef455f3 100644 (file)
@@ -30,6 +30,7 @@
 
 #include "DFGGraph.h"
 #include "DFGPhase.h"
+#include "DFGValueSource.h"
 #include "Operations.h"
 
 namespace JSC { namespace DFG {
@@ -166,35 +167,42 @@ public:
                 virtualRegisterForLocal(allocation[codeBlock()->activationRegister().toLocal()]));
         }
         
-        for (InlineCallFrameSet::iterator iter = m_graph.m_inlineCallFrames->begin(); !!iter; ++iter) {
-            InlineCallFrame* inlineCallFrame = *iter;
-            if (!inlineCallFrame->executable->usesArguments())
-                continue;
-            inlineCallFrame->argumentsRegister = virtualRegisterForLocal(
-                allocation[m_graph.argumentsRegisterFor(inlineCallFrame).toLocal()]);
+        for (unsigned i = m_graph.m_inlineVariableData.size(); i--;) {
+            InlineVariableData data = m_graph.m_inlineVariableData[i];
+            InlineCallFrame* inlineCallFrame = data.inlineCallFrame;
+            
+            if (inlineCallFrame->executable->usesArguments()) {
+                inlineCallFrame->argumentsRegister = virtualRegisterForLocal(
+                    allocation[m_graph.argumentsRegisterFor(inlineCallFrame).toLocal()]);
+
+                RELEASE_ASSERT(
+                    virtualRegisterForLocal(allocation[unmodifiedArgumentsRegister(
+                        m_graph.argumentsRegisterFor(inlineCallFrame)).toLocal()])
+                    == unmodifiedArgumentsRegister(inlineCallFrame->argumentsRegister));
+            }
             
-            // SpeculativeJIT::compileInlineStart() will do the same thing that this does,
-            // for cases where usesArguments() is true. That's OK. compileInlineStart() is
-            // designed for cases where the arguments end up having interesting data
-            // representations, but that only happens if they're not captured. And if they
-            // are captured, then we want to make sure everyone agrees on where they landed
-            // in the stack before we start generating any code - since SpeculativeJIT does
-            // not have to generate code in any particular order.
             for (unsigned argument = inlineCallFrame->arguments.size(); argument-- > 1;) {
-                VirtualRegister originalRegister = VirtualRegister(
-                    virtualRegisterForArgument(argument).offset() +
-                    inlineCallFrame->stackOffset);
-                VirtualRegister newRegister =
-                    virtualRegisterForLocal(allocation[originalRegister.toLocal()]);
-                inlineCallFrame->arguments[argument] =
-                    ValueRecovery::displacedInJSStack(newRegister, DataFormatJS);
+                ArgumentPosition& position = m_graph.m_argumentPositions[
+                    data.argumentPositionStart + argument];
+                VariableAccessData* variable = position.someVariable();
+                ValueSource source;
+                if (!variable)
+                    source = ValueSource(SourceIsDead);
+                else {
+                    source = ValueSource::forFlushFormat(
+                        variable->machineLocal(), variable->flushFormat());
+                }
+                inlineCallFrame->arguments[argument] = source.valueRecovery();
             }
             
-            RELEASE_ASSERT(
-                virtualRegisterForLocal(allocation[
-                    unmodifiedArgumentsRegister(
-                        m_graph.argumentsRegisterFor(inlineCallFrame)).toLocal()])
-                == unmodifiedArgumentsRegister(inlineCallFrame->argumentsRegister));
+            RELEASE_ASSERT(inlineCallFrame->isClosureCall == !!data.calleeVariable);
+            if (inlineCallFrame->isClosureCall) {
+                ValueSource source = ValueSource::forFlushFormat(
+                    data.calleeVariable->machineLocal(),
+                    data.calleeVariable->flushFormat());
+                inlineCallFrame->calleeRecovery = source.valueRecovery();
+            } else
+                RELEASE_ASSERT(inlineCallFrame->calleeRecovery.isConstant());
         }
         
         if (symbolTable) {
@@ -223,7 +231,7 @@ public:
             }
         }
         
-        // Finally, fix GetLocalUnlinked's.
+        // Fix GetLocalUnlinked's variable references.
         if (hasGetLocalUnlinked) {
             for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
                 BasicBlock* block = m_graph.block(blockIndex);
index 6af6f0697588ef2d205a1265a1019033dd0dc1b7..3a569486ad03494f7537fbc311231506684980e9 100644 (file)
@@ -57,6 +57,9 @@ void link(State& state)
     // LLVM will create its own jump tables as needed.
     codeBlock->clearSwitchJumpTables();
     
+    if (!state.graph.m_inlineCallFrames->isEmpty())
+        state.jitCode->common.inlineCallFrames = std::move(state.graph.m_inlineCallFrames);
+    
     // Create the entrypoint. Note that we use this entrypoint totally differently
     // depending on whether we're doing OSR entry or not.
     // FIXME: Except for OSR entry, this is a total kludge - LLVM should just use our