Changeset 171190 in webkit


Ignore:
Timestamp:
Jul 17, 2014 11:17:43 AM (10 years ago)
Author:
fpizlo@apple.com
Message:

DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
https://bugs.webkit.org/show_bug.cgi?id=134988
<rdar://problem/17706349>

Reviewed by Oliver Hunt.

Luckily, we also don't need this optimization to be super powerful: the only place
where it really matters is for getting rid of the redundancy between op_enter and
op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
two things. This change updates the store eliminator to know about only that small,
obviously safe, set of nodes over which we can store-eliminate.

This shouldn't have any performance impact in the DFG because this optimization kicks
in relatively rarely already. And once we tier up into the FTL, we get a much better
store elimination over LLVM IR, so this really shouldn't matter at all.

The tricky part of this patch is that there is a close relative of this optimization,
for uncaptured variables that got flushed. This happens for arguments to inlined calls.
I make this work by splitting it into two different store eliminators.

Note that in the process of crafting the tests, I realized that we were incorrectly
DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
to a check node followed by the actual allocation.

  • dfg/DFGCSEPhase.cpp:

(JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
(JSC::DFG::CSEPhase::setLocalStoreElimination):
(JSC::DFG::CSEPhase::performNodeCSE):
(JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.

  • dfg/DFGNodeType.h:
  • tests/stress/capture-escape-and-throw.js: Added.

(foo.f):
(foo):

  • tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.

(foo):
(bar):

Location:
trunk/Source/JavaScriptCore
Files:
2 added
3 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/JavaScriptCore/ChangeLog

    r171123 r171190  
     12014-07-16  Filip Pizlo  <fpizlo@apple.com>
     2
     3        DFG Flush(SetLocal) store elimination is overzealous for captured variables in the presence of nodes that have no effects but may throw
     4        https://bugs.webkit.org/show_bug.cgi?id=134988
     5        <rdar://problem/17706349>
     6
     7        Reviewed by Oliver Hunt.
     8       
     9        Luckily, we also don't need this optimization to be super powerful: the only place
     10        where it really matters is for getting rid of the redundancy between op_enter and
     11        op_init_lazy_reg, and in that case, there is a small set of possible nodes between the
     12        two things. This change updates the store eliminator to know about only that small,
     13        obviously safe, set of nodes over which we can store-eliminate.
     14       
     15        This shouldn't have any performance impact in the DFG because this optimization kicks
     16        in relatively rarely already. And once we tier up into the FTL, we get a much better
     17        store elimination over LLVM IR, so this really shouldn't matter at all.
     18       
     19        The tricky part of this patch is that there is a close relative of this optimization,
     20        for uncaptured variables that got flushed. This happens for arguments to inlined calls.
     21        I make this work by splitting it into two different store eliminators.
     22       
     23        Note that in the process of crafting the tests, I realized that we were incorrectly
     24        DCEing NewArrayWithSize. That's not cool, since that can throw an exception for
     25        negative array sizes. If we ever did want to DCE this node, we'd need to lower the node
     26        to a check node followed by the actual allocation.
     27
     28        * dfg/DFGCSEPhase.cpp:
     29        (JSC::DFG::CSEPhase::uncapturedSetLocalStoreElimination):
     30        (JSC::DFG::CSEPhase::capturedSetLocalStoreElimination):
     31        (JSC::DFG::CSEPhase::setLocalStoreElimination):
     32        (JSC::DFG::CSEPhase::performNodeCSE):
     33        (JSC::DFG::CSEPhase::SetLocalStoreEliminationResult::SetLocalStoreEliminationResult): Deleted.
     34        * dfg/DFGNodeType.h:
     35        * tests/stress/capture-escape-and-throw.js: Added.
     36        (foo.f):
     37        (foo):
     38        * tests/stress/new-array-with-size-throw-exception-and-tear-off-arguments.js: Added.
     39        (foo):
     40        (bar):
     41
    1422014-07-15  Benjamin Poulain  <benjamin@webkit.org>
    243
  • trunk/Source/JavaScriptCore/dfg/DFGCSEPhase.cpp

    r170555 r171190  
    929929    }
    930930   
    931     struct SetLocalStoreEliminationResult {
    932         SetLocalStoreEliminationResult()
    933             : mayBeAccessed(false)
    934             , mayExit(false)
    935             , mayClobberWorld(false)
    936         {
    937         }
    938        
    939         bool mayBeAccessed;
    940         bool mayExit;
    941         bool mayClobberWorld;
    942     };
    943     SetLocalStoreEliminationResult setLocalStoreElimination(
    944         VirtualRegister local, Node* expectedNode)
    945     {
    946         SetLocalStoreEliminationResult result;
     931    bool uncapturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
     932    {
    947933        for (unsigned i = m_indexInBlock; i--;) {
    948934            Node* node = m_currentBlock->at(i);
     
    951937            case Flush:
    952938                if (node->local() == local)
    953                     result.mayBeAccessed = true;
     939                    return false;
    954940                break;
    955941               
    956942            case GetLocalUnlinked:
    957943                if (node->unlinkedLocal() == local)
    958                     result.mayBeAccessed = true;
     944                    return false;
    959945                break;
    960946               
     
    963949                    break;
    964950                if (node != expectedNode)
    965                     result.mayBeAccessed = true;
    966                 return result;
     951                    return false;
     952                return true;
    967953            }
    968954               
     
    970956            case PutClosureVar:
    971957                if (static_cast<VirtualRegister>(node->varNumber()) == local)
    972                     result.mayBeAccessed = true;
     958                    return false;
    973959                break;
    974960               
     
    978964                    break;
    979965                if (m_graph.uncheckedActivationRegister() == local)
    980                     result.mayBeAccessed = true;
     966                    return false;
    981967                break;
    982968               
     
    985971            case GetMyArgumentsLengthSafe:
    986972                if (m_graph.uncheckedArgumentsRegisterFor(node->origin.semantic) == local)
    987                     result.mayBeAccessed = true;
     973                    return false;
    988974                break;
    989975               
    990976            case GetMyArgumentByVal:
    991977            case GetMyArgumentByValSafe:
    992                 result.mayBeAccessed = true;
    993                 break;
     978                return false;
    994979               
    995980            case GetByVal:
    996981                // If this is accessing arguments then it's potentially accessing locals.
    997982                if (node->arrayMode().type() == Array::Arguments)
    998                     result.mayBeAccessed = true;
     983                    return false;
    999984                break;
    1000985               
     
    1006991                // argument register. But that seems like it would buy us very little since
    1007992                // any kind of tear offs are rare to begin with.
    1008                 result.mayBeAccessed = true;
    1009                 break;
    1010                
    1011             default:
    1012                 break;
    1013             }
    1014             result.mayExit |= node->canExit();
    1015             result.mayClobberWorld |= m_graph.clobbersWorld(node);
     993                return false;
     994               
     995            default:
     996                break;
     997            }
     998            if (m_graph.clobbersWorld(node))
     999                return false;
    10161000        }
    10171001        RELEASE_ASSERT_NOT_REACHED();
    1018         // Be safe in release mode.
    1019         result.mayBeAccessed = true;
    1020         return result;
     1002        return false;
     1003    }
     1004
     1005    bool capturedSetLocalStoreElimination(VirtualRegister local, Node* expectedNode)
     1006    {
     1007        for (unsigned i = m_indexInBlock; i--;) {
     1008            Node* node = m_currentBlock->at(i);
     1009            switch (node->op()) {
     1010            case GetLocal:
     1011            case Flush:
     1012                if (node->local() == local)
     1013                    return false;
     1014                break;
     1015               
     1016            case GetLocalUnlinked:
     1017                if (node->unlinkedLocal() == local)
     1018                    return false;
     1019                break;
     1020               
     1021            case SetLocal: {
     1022                if (node->local() != local)
     1023                    break;
     1024                if (node != expectedNode)
     1025                    return false;
     1026                return true;
     1027            }
     1028               
     1029            case Phantom:
     1030            case Check:
     1031            case HardPhantom:
     1032            case MovHint:
     1033            case JSConstant:
     1034            case DoubleConstant:
     1035            case Int52Constant:
     1036                break;
     1037               
     1038            default:
     1039                return false;
     1040            }
     1041        }
     1042        RELEASE_ASSERT_NOT_REACHED();
     1043        return false;
     1044    }
     1045   
     1046    bool setLocalStoreElimination(VariableAccessData* variableAccessData, Node* expectedNode)
     1047    {
     1048        if (variableAccessData->isCaptured())
     1049            return capturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
     1050        return uncapturedSetLocalStoreElimination(variableAccessData->local(), expectedNode);
    10211051    }
    10221052   
     
    12161246            ASSERT(m_graph.m_form != SSA);
    12171247            VariableAccessData* variableAccessData = node->variableAccessData();
    1218             VirtualRegister local = variableAccessData->local();
    12191248            if (!node->child1()) {
    12201249                // FIXME: It's silly that we punt on flush-eliminating here. We don't really
     
    12401269                    break;
    12411270            }
    1242             SetLocalStoreEliminationResult result =
    1243                 setLocalStoreElimination(local, replacement);
    1244             if (result.mayBeAccessed || result.mayClobberWorld)
     1271            if (!setLocalStoreElimination(variableAccessData, replacement))
    12451272                break;
    12461273            ASSERT(replacement->op() == SetLocal);
    1247             // FIXME: Investigate using mayExit as a further optimization.
    12481274            node->convertToPhantom();
    12491275            Node* dataNode = replacement->child1().node();
  • trunk/Source/JavaScriptCore/dfg/DFGNodeType.h

    r171096 r171190  
    234234    macro(NewObject, NodeResultJS) \
    235235    macro(NewArray, NodeResultJS | NodeHasVarArgs) \
    236     macro(NewArrayWithSize, NodeResultJS) \
     236    macro(NewArrayWithSize, NodeResultJS | NodeMustGenerate) \
    237237    macro(NewArrayBuffer, NodeResultJS) \
    238238    macro(NewTypedArray, NodeResultJS | NodeClobbersWorld | NodeMustGenerate) \
Note: See TracChangeset for help on using the changeset viewer.