Changeset 192190 in webkit


Ignore:
Timestamp:
Nov 9, 2015 4:39:13 PM (8 years ago)
Author:
sbarati@apple.com
Message:

DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
​https://bugs.webkit.org/show_bug.cgi?id=145295

Reviewed by Filip Pizlo.

This patch fixes PutStackSinkingPhase to no longer escape the stack
locations that LoadVarargs and ForwardVarargs write to. We used
to consider sinking PutStacks right before a LoadVarargs/ForwardVarargs
because we considered them uses of such stack locations. They aren't
uses of those stack locations, they unconditionally write to those
stack locations. Sinking PutStacks to these nodes was not needed before,
but seemed mostly innocent. But I ran into a problem with this while implementing
FTL try/catch where we would end up having to generate a value for a sunken PutStack
right before a LoadVarargs. This would cause us to issue a GetStack that loaded garbage that
was then forwarded into a Phi that was used as the source as the PutStack. This caused the
abstract interpreter to confuse itself on type information for the garbage GetStack
that was fed into the Phi, which would cause the abstract interpreter to then claim
that the basic block with the PutStack in it would never be reached. This isn't true, the
block would indeed be reached. The solution here is to be more precise about the
liveness of locals w.r.t LoadVarargs and ForwardVarargs.

  • dfg/DFGPreciseLocalClobberize.h:

(JSC::DFG::PreciseLocalClobberizeAdaptor::PreciseLocalClobberizeAdaptor):
(JSC::DFG::PreciseLocalClobberizeAdaptor::write):

  • dfg/DFGPutStackSinkingPhase.cpp:
  • dfg/DFGSSACalculator.h:
Location:
trunk/Source/JavaScriptCore
Files:
4 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r192187 r192190  
     12015-11-09  Saam barati  <sbarati@apple.com>
     2
     3        DFG::PutStackSinkingPhase should not treat the stack variables written by LoadVarargs/ForwardVarargs as being live
     4        https://bugs.webkit.org/show_bug.cgi?id=145295
     5
     6        Reviewed by Filip Pizlo.
     7
     8        This patch fixes PutStackSinkingPhase to no longer escape the stack
     9        locations that LoadVarargs and ForwardVarargs write to. We used
     10        to consider sinking PutStacks right before a LoadVarargs/ForwardVarargs
     11        because we considered them uses of such stack locations. They aren't
     12        uses of those stack locations, they unconditionally write to those
     13        stack locations. Sinking PutStacks to these nodes was not needed before,
     14        but seemed mostly innocent. But I ran into a problem with this while implementing
     15        FTL try/catch where we would end up having to generate a value for a sunken PutStack
     16        right before a LoadVarargs. This would cause us to issue a GetStack that loaded garbage that
     17        was then forwarded into a Phi that was used as the source as the PutStack. This caused the
     18        abstract interpreter to confuse itself on type information for the garbage GetStack
     19        that was fed into the Phi, which would cause the abstract interpreter to then claim
     20        that the basic block with the PutStack in it would never be reached. This isn't true, the
     21        block would indeed be reached. The solution here is to be more precise about the
     22        liveness of locals w.r.t LoadVarargs and ForwardVarargs.
     23
     24        * dfg/DFGPreciseLocalClobberize.h:
     25        (JSC::DFG::PreciseLocalClobberizeAdaptor::PreciseLocalClobberizeAdaptor):
     26        (JSC::DFG::PreciseLocalClobberizeAdaptor::write):
     27        * dfg/DFGPutStackSinkingPhase.cpp:
     28        * dfg/DFGSSACalculator.h:
     29
    1302015-11-09  Filip Pizlo  <fpizlo@apple.com>
    231

  • trunk/Source/JavaScriptCore/dfg/DFGPreciseLocalClobberize.h

    r191965 r192190  
    4343        , m_node(node)
    4444        , m_read(read)
    45         , m_write(write)
     45        , m_unconditionalWrite(write)
    4646        , m_def(def)
    4747    {
    … …  
    7171        if (heap.kind() == Stack) {
    7272            RELEASE_ASSERT(!heap.payload().isTop());
    73             callIfAppropriate(m_write, VirtualRegister(heap.payload().value32()));
     73            callIfAppropriate(m_unconditionalWrite, VirtualRegister(heap.payload().value32()));
    7474            return;
    7575        }
    … …  
    156156    Node* m_node;
    157157    const ReadFunctor& m_read;
    158     const WriteFunctor& m_write;
     158    const WriteFunctor& m_unconditionalWrite;
    159159    const DefFunctor& m_def;
    160160};

  • trunk/Source/JavaScriptCore/dfg/DFGPutStackSinkingPhase.cpp

    r191870 r192190  
    109109                        dataLog("Live at ", node, ": ", live, "\n");
    110110                   
     111                    Vector<VirtualRegister, 4> reads;
     112                    Vector<VirtualRegister, 4> writes;
    111113                    auto escapeHandler = [&] (VirtualRegister operand) {
    112114                        if (operand.isHeader())
    … …  
    114116                        if (verbose)
    115117                            dataLog("    ", operand, " is live at ", node, "\n");
     118                        reads.append(operand);
     119                    };
     120
     121                    auto writeHandler = [&] (VirtualRegister operand) {
     122                        RELEASE_ASSERT(node->op() == PutStack || node->op() == LoadVarargs || node->op() == ForwardVarargs);
     123                        writes.append(operand);
     124                    };
     125
     126                    preciseLocalClobberize(
     127                        m_graph, node, escapeHandler, writeHandler,
     128                        [&] (VirtualRegister, LazyNode) { });
     129
     130                    for (VirtualRegister operand : writes)
     131                        live.operand(operand) = false;
     132                    for (VirtualRegister operand : reads)
    116133                        live.operand(operand) = true;
    117                     };
    118                    
    119                     // FIXME: This might mishandle LoadVarargs and ForwardVarargs. It might make us
    120                     // think that the locals being written are stack-live here. They aren't. This
    121                     // should be harmless since we overwrite them anyway, but still, it's sloppy.
    122                     // https://bugs.webkit.org/show_bug.cgi?id=145295
    123                     preciseLocalClobberize(
    124                         m_graph, node, escapeHandler, escapeHandler,
    125                         [&] (VirtualRegister operand, LazyNode source) {
    126                             RELEASE_ASSERT(source.isNode());
    127 
    128                             if (source.asNode() == node) {
    129                                 // This is a load. Ignore it.
    130                                 return;
    131                             }
    132                            
    133                             RELEASE_ASSERT(node->op() == PutStack);
    134                             live.operand(operand) = false;
    135                         });
    136134                }
    137135               
    … …  
    206204        //     identified an operation that would have observed that PutStack.
    207205        //
    208         // This code has some interesting quirks because of the fact that neither liveness nor
    209         // deferrals are very precise. They are only precise enough to be able to correctly tell us
    210         // when we may [sic] need to execute PutStacks. This means that they may report the need to
    211         // execute a PutStack in cases where we actually don't really need it, and that's totally OK.
     206        // We need to be precise about liveness in this phase because not doing so
     207        // could cause us to insert a PutStack before a node we thought may escape a
     208        // value that it doesn't really escape. Sinking this PutStack above such a node may
     209        // cause us to insert a GetStack that we forward to the Phi we're feeding into the
     210        // sunken PutStack. Inserting such a GetStack could cause us to load garbage and
     211        // can confuse the AI to claim untrue things (like that the program will exit when
     212        // it really won't).
    212213        BlockMap<Operands<FlushFormat>> deferredAtHead(m_graph);
    213214        BlockMap<Operands<FlushFormat>> deferredAtTail(m_graph);
    … …  
    270271                        // from.
    271272                        continue;
     273                    } else if (node->op() == PutStack) {
     274                        VirtualRegister operand = node->stackAccessData()->local;
     275                        deferred.operand(operand) = node->stackAccessData()->format;
     276                        continue;
    272277                    }
    273278                   
    … …  
    280285                        deferred.operand(operand) = DeadFlush;
    281286                    };
     287
     288                    auto writeHandler = [&] (VirtualRegister operand) {
     289                        RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
     290                        deferred.operand(operand) = DeadFlush;
     291                    };
    282292                   
    283293                    preciseLocalClobberize(
    284                         m_graph, node, escapeHandler, escapeHandler,
    285                         [&] (VirtualRegister operand, LazyNode source) {
    286                             RELEASE_ASSERT(source.isNode());
    287 
    288                             if (source.asNode() == node) {
    289                                 // This is a load. Ignore it.
    290                                 return;
    291                             }
    292                            
    293                             deferred.operand(operand) = node->stackAccessData()->format;
    294                         });
     294                        m_graph, node, escapeHandler, writeHandler,
     295                        [&] (VirtualRegister, LazyNode) { });
    295296                }
    296297               
    … …  
    352353        }
    353354       
    354         HashSet<Node*> putLocalsToSink;
     355        HashSet<Node*> putStacksToSink;
    355356       
    356357        for (BasicBlock* block : m_graph.blocksInNaturalOrder()) {
    … …  
    358359                switch (node->op()) {
    359360                case PutStack:
    360                     putLocalsToSink.add(node);
     361                    putStacksToSink.add(node);
    361362                    ssaCalculator.newDef(
    362363                        operandToVariable.operand(node->stackAccessData()->local),
    … …  
    497498                        deferred.operand(operand) = DeadFlush;
    498499                    };
    499                
     500
     501                    auto writeHandler = [&] (VirtualRegister operand) {
     502                        // LoadVarargs and ForwardVarargs are unconditional writes to the stack
     503                        // locations they claim to write to. They do not read from the stack
     504                        // locations they write to. This makes those stack locations dead right
     505                        // before a LoadVarargs/ForwardVarargs. This means we should never sink
     506                        // PutStacks right to this point.
     507                        RELEASE_ASSERT(node->op() == LoadVarargs || node->op() == ForwardVarargs);
     508                        deferred.operand(operand) = DeadFlush;
     509                    };
     510
    500511                    preciseLocalClobberize(
    501                         m_graph, node, escapeHandler, escapeHandler,
     512                        m_graph, node, escapeHandler, writeHandler,
    502513                        [&] (VirtualRegister, LazyNode) { });
    503514                    break;
    … …  
    553564                Node* node = block->at(nodeIndex);
    554565               
    555                 if (!putLocalsToSink.contains(node))
     566                if (!putStacksToSink.contains(node))
    556567                    continue;
    557568               
    558569                node->remove();
    559570            }
    560            
    561             insertionSet.execute(block);
    562571        }
    563572       

  • trunk/Source/JavaScriptCore/dfg/DFGSSACalculator.h

    r191870 r192190  
    9292//         https://bugs.webkit.org/show_bug.cgi?id=136639
    9393//
    94 //    4.3) Insert Upsilons for each Phi in each successor block. Use the available values table to
    95 //         decide the source value for each Phi's variable. Note that you could also use
    96 //         SSACalculator::reachingDefAtTail() instead of the available values table, though your
    97 //         local available values table is likely to be more efficient.
     94//    4.3) Insert Upsilons at the end of the current block for the corresponding Phis in each successor block.
     95//         Use the available values table to decide the source value for each Phi's variable. Note that
     96//         you could also use SSACalculator::reachingDefAtTail() instead of the available values table,
     97//         though your local available values table is likely to be more efficient.
    9898//
    9999// The most obvious use of SSACalculator is for the CPS->SSA conversion itself, but it's meant to
Note: See TracChangeset for help on using the changeset viewer.