Attachment 227254 Details for Bug 130295 – the wrong approach

[patch] the wrong approach

blah.patch (text/plain), 17.95 KB, created by Filip Pizlo on 2014-03-19 21:56:53 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2014-03-19 21:56:53 PDT

Size: 17.95 KB

patch

obsolete

>Index: Source/JavaScriptCore/bytecode/CodeBlock.cpp
>===================================================================
>--- Source/JavaScriptCore/bytecode/CodeBlock.cpp	(revision 165941)
>+++ Source/JavaScriptCore/bytecode/CodeBlock.cpp	(working copy)
>@@ -2439,8 +2439,6 @@ bool CodeBlock::isCaptured(VirtualRegist
>     // Ditto for the arguments object.
>     if (usesArguments() && operand == argumentsRegister())
>         return true;
>-
>-    // Ditto for the arguments object.
>     if (usesArguments() && operand == unmodifiedArgumentsRegister(argumentsRegister()))
>         return true;
> 
>Index: Source/JavaScriptCore/dfg/DFGBasicBlock.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGBasicBlock.cpp	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGBasicBlock.cpp	(working copy)
>@@ -82,6 +82,15 @@ bool BasicBlock::isInBlock(Node* myNode)
>     return false;
> }
> 
>+bool BasicBlock::hasForwardSuccessor()
>+{
>+    for (unsigned i = numSuccessors(); i--;) {
>+        if (successor(i)->index > index)
>+            return true;
>+    }
>+    return false;
>+}
>+
> void BasicBlock::removePredecessor(BasicBlock* block)
> {
>     for (unsigned i = 0; i < predecessors.size(); ++i) {
>Index: Source/JavaScriptCore/dfg/DFGBasicBlock.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGBasicBlock.h	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGBasicBlock.h	(working copy)
>@@ -95,6 +95,8 @@ struct BasicBlock : RefCounted<BasicBloc
>         return last()->successorForCondition(condition);
>     }
>     
>+    bool hasForwardSuccessor();
>+    
>     void removePredecessor(BasicBlock* block);
>     void replacePredecessor(BasicBlock* from, BasicBlock* to);
> 
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -3052,6 +3052,7 @@ bool ByteCodeParser::parseBlock(unsigned
>         case op_ret:
>             flushArgumentsAndCapturedVariables();
>             if (inlineCallFrame()) {
>+                addToGraph(InlineEnd);
>                 ASSERT(m_inlineStackTop->m_returnValue.isValid());
>                 setDirect(m_inlineStackTop->m_returnValue, get(VirtualRegister(currentInstruction[1].u.operand)), ImmediateSet);
>                 m_inlineStackTop->m_didReturn = true;
>@@ -3596,8 +3597,12 @@ ByteCodeParser::InlineStackEntry::Inline
>             m_inlineCallFrame->capturedVars = m_inlineCallFrame->caller.inlineCallFrame->capturedVars;
>         else {
>             for (int i = byteCodeParser->m_codeBlock->m_numVars; i--;) {
>-                if (byteCodeParser->m_codeBlock->isCaptured(virtualRegisterForLocal(i)))
>-                    m_inlineCallFrame->capturedVars.set(i);
>+                VirtualRegister reg = virtualRegisterForLocal(i);
>+                if (byteCodeParser->m_codeBlock->isCaptured(reg)) {
>+                    VirtualRegister remapped = VirtualRegister(
>+                        reg.offset() + m_inlineCallFrame->stackOffset);
>+                    m_inlineCallFrame->capturedVars.set(remapped.toLocal());
>+                }
>             }
>         }
> 
>Index: Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGCPSRethreadingPhase.cpp	(working copy)
>@@ -30,6 +30,7 @@
> 
> #include "DFGBasicBlockInlines.h"
> #include "DFGGraph.h"
>+#include "DFGInsertionSet.h"
> #include "DFGPhase.h"
> #include "JSCInlines.h"
> 
>@@ -39,6 +40,7 @@ class CPSRethreadingPhase : public Phase
> public:
>     CPSRethreadingPhase(Graph& graph)
>         : Phase(graph, "CPS rethreading")
>+        , m_operands(OperandsLike, m_graph.block(0)->variablesAtHead);
>     {
>     }
>     
>@@ -51,6 +53,7 @@ public:
>         
>         clearIsLoadedFrom();
>         freeUnnecessaryNodes();
>+        insertFlushes();
>         m_graph.clearReplacements();
>         canonicalizeLocalsInBlocks();
>         propagatePhis<LocalOperand>();
>@@ -84,7 +87,6 @@ private:
>                 Node* node = block->at(fromIndex++);
>                 switch (node->op()) {
>                 case GetLocal:
>-                case Flush:
>                 case PhantomLocal:
>                     node->children.setChild1(Edge());
>                     break;
>@@ -102,6 +104,8 @@ private:
>                         break;
>                     }
>                     break;
>+                case Flush:
>+                    continue;
>                 default:
>                     break;
>                 }
>@@ -116,6 +120,109 @@ private:
>     }
>     
>     template<OperandKind operandKind>
>+    void initializeOperands()
>+    {
>+        for (size_t i = m_operands.sizeFor<operandKind>(); i--;) {
>+            Node* node = m_block->variablesAtHead.atFor<operandKind>(i);
>+            if (node)
>+                m_operands.atFor<operandFor>(i) = node->variableAccessData();
>+            else
>+                m_operands.atFor<operandFor>(i) = nullptr;
>+        }
>+    }
>+    
>+    void insertFlush(unsigned nodeIndex, int operand)
>+    {
>+        VariableAccessData* data = m_operands.operand(operand);
>+        if (!data) {
>+            
>+        }
>+        
>+    }
>+    
>+    void insertFlushesForInlineCallFrame(InlineCallFrame* inlineCallFrame, unsigned nodeIndex)
>+    {
>+        int numArguments;
>+        int stackOffset;
>+        
>+        if (inlineCallFrame) {
>+            numArguments = inlineCallFrame->arguments.size();
>+            stackOffset = inlineCallFrame->stackOffset;
>+            
>+            if (inlineCallFrame->isClosureCall) {
>+                insertFlush(nodeIndex, stackOffset + JSStack::Callee);
>+                insertFlush(nodeIndex, stackOffset + JSStack::ScopeChain);
>+            }
>+            
>+            for (int local = inlineCallFrame->capturedVars.size(); local--;) {
>+                if (inlineCallFrame->capturedVars.quickGet(i))
>+                    insertFlush(nodeIndex, stackOffset + virtualRegisterForLocal(local).offset());
>+            }
>+        } else {
>+            numArguments = codeBlock()->numParameters();
>+            stackOffset = 0;
>+            
>+            for (int local = codeBlock()->m_numVars; ++local) {
>+                if (codeBlock()->isCaptured(local))
>+                    insertFlush(nodeIndex, virtualRegisterForLocal(local).offset());
>+            }
>+        }
>+        
>+        for (int argument = numArguments; argument-- > 1;)
>+            insertFlush(nodeIndex, stackOffset + virtualRegisterForArgument(argument).offset());
>+    }
>+    
>+    void insertFlushes()
>+    {
>+        // This is the sole repository of knowledge about what needs to be flushed.
>+        
>+        for (BlockIndex blockIndex = m_graph.numBlocks(); blockIndex--;) {
>+            m_block = m_graph.block(blockIndex);
>+            if (!m_block)
>+                continue;
>+            ASSERT(m_block->isReachable);
>+            
>+            initializeOperands<ArgumentOperand>();
>+            initializeOperands<LocalOperand>();
>+            
>+            for (unsigned nodeIndex = 0; nodeIndex < m_block->size(); ++i) {
>+                Node* node = m_block->at(nodeIndex);
>+                switch (node->op()) {
>+                case SetLocal: {
>+                    // PROBLEM: Need to flush if it's a "normal set" but don't flush
>+                    // otherwise.
>+                    
>+                    VariableAccessData* data = node->variableAccessData();
>+                    m_operands.operand(data->local()) = data;
>+                    break;
>+                }
>+
>+                case GetLocal:
>+                case GetArgument:
>+                case SetArgument:
>+                case PhantomLocal: {
>+                    VariableAccessData* data = node->variableAccessData();
>+                    m_operands.operand(data->local()) = data;
>+                    break;
>+                }
>+                    
>+                case Flush:
>+                    RELEASE_ASSERT_NOT_REACHED();
>+                    break;
>+                    
>+                case InlineEnd: {
>+                    
>+                    break;
>+                }
>+                    
>+                default:
>+                    break;
>+                }
>+            }
>+        }
>+    }
>+    
>+    template<OperandKind operandKind>
>     void clearVariablesAtHeadAndTail()
>     {
>         ASSERT(
>@@ -483,8 +590,10 @@ private:
>     }
>     
>     BasicBlock* m_block;
>+    Operands<VariableAccessData*> m_operands;
>     Vector<PhiStackEntry, 128> m_argumentPhiStack;
>     Vector<PhiStackEntry, 128> m_localPhiStack;
>+    InsertionSet m_insertionSet;
> };
> 
> bool performCPSRethreading(Graph& graph)
>Index: Source/JavaScriptCore/dfg/DFGClobberize.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGClobberize.h	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGClobberize.h	(working copy)
>@@ -153,6 +153,7 @@ void clobberize(Graph& graph, Node* node
>     case CheckTierUpAndOSREnter:
>     case LoopHint:
>     case InvalidationPoint:
>+    case InlineEnd:
>         write(SideState);
>         return;
>         
>Index: Source/JavaScriptCore/dfg/DFGCommon.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGCommon.h	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGCommon.h	(working copy)
>@@ -103,7 +103,7 @@ enum OptimizationFixpointState { BeforeF
> // Describes the form you can expect the entire graph to be in.
> enum GraphForm {
>     // LoadStore form means that basic blocks may freely use GetLocal, SetLocal,
>-    // GetLocalUnlinked, and Flush for accessing local variables and indicating
>+    // and GetLocalUnlinked for accessing local variables and indicating
>     // where their live ranges ought to be. Data flow between local accesses is
>     // implicit. Liveness is only explicit at block heads (variablesAtHead).
>     // This is only used by the DFG simplifier and is only preserved by same.
>Index: Source/JavaScriptCore/dfg/DFGNodeType.h
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGNodeType.h	(revision 165941)
>+++ Source/JavaScriptCore/dfg/DFGNodeType.h	(working copy)
>@@ -73,6 +73,9 @@ namespace JSC { namespace DFG {
>     /* want all state. */\
>     macro(LoopHint, NodeMustGenerate) \
>     \
>+    /* Hint that we are at an inlined return site. */\
>+    macro(InlineEnd, NodeMustGenerate) \
>+    \
>     /* Special node for OSR entry into the FTL. Indicates that we're loading a local */\
>     /* variable from the scratch buffer. */\
>     macro(ExtractOSREntryLocal, NodeResultJS) \
>Index: Source/JavaScriptCore/ftl/FTLCapabilities.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLCapabilities.cpp	(revision 165941)
>+++ Source/JavaScriptCore/ftl/FTLCapabilities.cpp	(working copy)
>@@ -151,6 +151,7 @@ inline CapabilityLevel canCompile(Node* 
>     case IsFunction:
>     case CheckHasInstance:
>     case InstanceOf:
>+    case InlineEnd:
>         // These are OK.
>         break;
>     case PutByIdDirect:
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(revision 165941)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp	(working copy)
>@@ -615,6 +615,7 @@ private:
>         case FunctionReentryWatchpoint:
>         case TypedArrayWatchpoint:
>         case AllocationProfileWatchpoint:
>+        case InlineEnd:
>             break;
>         case Unreachable:
>             RELEASE_ASSERT_NOT_REACHED();
>Index: Source/JavaScriptCore/tests/stress/inferred-infinite-loop-that-uses-captured-variables.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/inferred-infinite-loop-that-uses-captured-variables.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/inferred-infinite-loop-that-uses-captured-variables.js	(working copy)
>@@ -0,0 +1,27 @@
>+function bar(f) {
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+var shouldContinue = true;
>+
>+function foo(a) {
>+    var x = a + 1;
>+    while (shouldContinue) {
>+        bar(function() { return x; });
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables-with-osr-entry.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables-with-osr-entry.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables-with-osr-entry.js	(working copy)
>@@ -0,0 +1,26 @@
>+var count = 0;
>+
>+function bar(f) {
>+    if (++count >= 10000)
>+        throw f;
>+}
>+
>+noInline(bar);
>+
>+function foo(a) {
>+    var x = a + 1;
>+    for (;;) {
>+        bar(function() { return x; });
>+    }
>+}
>+
>+noInline(foo);
>+
>+try {
>+    foo(42);
>+} catch (f) {
>+    var result = f();
>+    if (result != 43)
>+        throw "Error: bad result: " + result;
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/infinite-loop-that-uses-captured-variables.js	(working copy)
>@@ -0,0 +1,25 @@
>+function bar(f) {
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+function foo(a) {
>+    var x = a + 1;
>+    for (;;) {
>+        bar(function() { return x; });
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/tricky-indirectly-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(working copy)
>@@ -0,0 +1,36 @@
>+var count = 0;
>+
>+function bar(f) {
>+    if (++count < 100)
>+        return;
>+    count = 0;
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+function fuzz(a) {
>+    return a != true;
>+}
>+
>+function foo(a) {
>+    var x = a + 1;
>+    var y = a + 2;
>+    var f = (function() { return x; });
>+    while (fuzz(y)) {
>+        bar(f);
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/tricky-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/tricky-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/tricky-inferred-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(working copy)
>@@ -0,0 +1,33 @@
>+var count = 0;
>+
>+function bar(f) {
>+    if (++count < 100)
>+        return;
>+    count = 0;
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+var shouldContinue = true;
>+
>+function foo(a) {
>+    var x = a + 1;
>+    var f = (function() { return x; });
>+    while (shouldContinue) {
>+        bar(f);
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables-and-creates-the-activation-outside-the-loop.js	(working copy)
>@@ -0,0 +1,31 @@
>+var count = 0;
>+
>+function bar(f) {
>+    if (++count < 100)
>+        return;
>+    count = 0;
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+function foo(a) {
>+    var x = a + 1;
>+    var f = (function() { return x; });
>+    for (;;) {
>+        bar(f);
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+
>Index: Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables.js
>===================================================================
>--- Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables.js	(revision 0)
>+++ Source/JavaScriptCore/tests/stress/tricky-infinite-loop-that-uses-captured-variables.js	(working copy)
>@@ -0,0 +1,30 @@
>+var count = 0;
>+
>+function bar(f) {
>+    if (++count < 100)
>+        return;
>+    count = 0;
>+    throw f;
>+}
>+
>+noInline(bar);
>+
>+function foo(a) {
>+    var x = a + 1;
>+    for (;;) {
>+        bar(function() { return x; });
>+    }
>+}
>+
>+noInline(foo);
>+
>+for (var i = 0; i < 10000; ++i) {
>+    try {
>+        foo(i);
>+    } catch (f) {
>+        var result = f();
>+        if (result != i + 1)
>+            throw "Error: bad result for i = " + i + ": " + result;
>+    }
>+}
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 130295: 227248 | 227254 | 227305 | 227313