Attachment 215127 Details for Bug 123302 – the patch

[patch] the patch

blah.patch (text/plain), 18.69 KB, created by Filip Pizlo on 2013-10-24 18:08:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2013-10-24 18:08:29 PDT

Size: 18.69 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 157969)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -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 NOBODY (OOPS!).
>+        
>+        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 handing 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  Mark Lam  <mark.lam@apple.com>
> 
>         Better way to fix part of broken C Loop LLINT build.
>Index: Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h	(working copy)
>@@ -1541,7 +1541,6 @@ bool AbstractInterpreter<AbstractStateTy
>         break;
>             
>     case Phantom:
>-    case InlineStart:
>     case CountExecution:
>     case CheckTierUpInLoop:
>     case CheckTierUpAtReturn:
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -1304,11 +1304,11 @@ bool ByteCodeParser::handleInlining(Node
>     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
>         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::Inline
>             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: Source/JavaScriptCore/dfg/DFGClobberize.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGClobberize.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGClobberize.h	(working copy)
>@@ -124,7 +124,6 @@ void clobberize(Graph& graph, Node* node
>     case Flush:
>     case PhantomLocal:
>     case SetArgument:
>-    case InlineStart:
>     case Breakpoint:
>     case PhantomArguments:
>     case Jump:
>Index: Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGFixupPhase.cpp	(working copy)
>@@ -907,7 +907,6 @@ private:
>         case Flush:
>         case PhantomLocal:
>         case GetLocalUnlinked:
>-        case InlineStart:
>         case GetMyScope:
>         case GetClosureVar:
>         case GetGlobalVar:
>Index: Source/JavaScriptCore/dfg/DFGGraph.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGGraph.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGGraph.h	(working copy)
>@@ -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: Source/JavaScriptCore/dfg/DFGNode.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGNode.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGNode.h	(working copy)
>@@ -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: Source/JavaScriptCore/dfg/DFGNodeType.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGNodeType.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGNodeType.h	(working copy)
>@@ -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: Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp	(working copy)
>@@ -586,7 +586,6 @@ private:
>             break;
>             
>         // These gets ignored because it doesn't do anything.
>-        case InlineStart:
>         case CountExecution:
>         case PhantomLocal:
>         case Flush:
>Index: Source/JavaScriptCore/dfg/DFGSafeToExecute.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSafeToExecute.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGSafeToExecute.h	(working copy)
>@@ -129,7 +129,6 @@ bool safeToExecute(AbstractStateType& st
>     case PhantomLocal:
>     case GetLocalUnlinked:
>     case SetArgument:
>-    case InlineStart:
>     case BitAnd:
>     case BitOr:
>     case BitXor:
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp	(working copy)
>@@ -1958,11 +1958,6 @@ void SpeculativeJIT::compile(Node* node)
>         break;
>     }
>         
>-    case InlineStart: {
>-        compileInlineStart(node);
>-        break;
>-    }
>-
>     case MovHint:
>     case ZombieHint: {
>         RELEASE_ASSERT_NOT_REACHED();
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -2269,11 +2269,6 @@ void SpeculativeJIT::compile(Node* node)
>         break;
>     }
>         
>-    case InlineStart: {
>-        compileInlineStart(node);
>-        break;
>-    }
>-
>     case MovHint:
>     case ZombieHint: {
>         RELEASE_ASSERT_NOT_REACHED();
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(working copy)
>@@ -1557,37 +1557,6 @@ void SpeculativeJIT::compileMovHintAndCh
>     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: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h	(working copy)
>@@ -707,7 +707,6 @@ public:
>     
>     void compileMovHint(Node*);
>     void compileMovHintAndCheck(Node*);
>-    void compileInlineStart(Node*);
> 
>     void nonSpeculativeUInt32ToNumber(Node*);
> 
>Index: Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp	(revision 157904)
>+++ Source/JavaScriptCore/dfg/DFGStackLayoutPhase.cpp	(working copy)
>@@ -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: Source/JavaScriptCore/ftl/FTLLink.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLink.cpp	(revision 157904)
>+++ Source/JavaScriptCore/ftl/FTLLink.cpp	(working copy)
>@@ -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 = state.graph.m_inlineCallFrames.release();
>+    
>     // 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

Flags:

ggaren: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 123302: 215123 | 215126 | 215127