Changeset 161072 in webkit


Ignore:
Timestamp:
Dec 25, 2013 3:44:57 PM (10 years ago)
Author:
fpizlo@apple.com
Message:

DFG PhantomArguments shouldn't rely on a dead Phi graph
​https://bugs.webkit.org/show_bug.cgi?id=126218

Source/JavaScriptCore:

Reviewed by Oliver Hunt.

This change dramatically rationalizes our handling of PhantomArguments (i.e.
speculative elision of arguments object allocation).

It's now the case that if we decide that we can elide arguments allocation, we just
turn the arguments-creating node into a PhantomArguments and mark all locals that
it's stored to as being arguments aliases. Being an arguments alias and being a
PhantomArguments means basically the same thing: in DFG execution you have the empty
value, on OSR exit an arguments object is allocated in your place, and all operations
that use the value now just refer directly to the actual arguments in the call frame
header (or the arguments we know that we passed to the call, in case of inlining).

This means that we no longer have arguments simplification creating a dead Phi graph
that then has to be interpreted by the OSR exit logic. That sort of never made any
sense.

This means that PhantomArguments now has a clear story in SSA: basically SSA just
gets rid of the "locals" but everything else is the same.

Finally, this means that we can more easily get rid of forward exiting. As I was
working on the code to get rid of forward exiting, I realized that I'd have to
carefully preserve the special meanings of MovHint and SetLocal in the case of
PhantomArguments. It was really bizarre: even the semantics of MovHint were tied to
our specific treatment of PhantomArguments. After this change this is no longer the
case.

One of the really cool things about this change is that arguments reification now
just becomes a special kind of FlushFormat. This further unifies things: it means
that a MovHint(PhantomArguments) and a SetLocal(PhantomArguments) both have the same
meaning, since both of them dictate that the way we recover the local on exit is by
reifying arguments. Previously, the SetLocal(PhantomArguments) case needed some
special handling to accomplish this.

A downside of this approach is that we will now emit code to store the empty value
into aliased arguments variables, and we will even emit code to load that empty value
as well. As far as I can tell this doesn't cost anything, since PhantomArguments are
most profitable in cases where it allows us to simplify control flow and kill the
arguments locals entirely. Of course, this isn't an issue in SSA form since SSA form
also eliminates the locals.

  • dfg/DFGArgumentsSimplificationPhase.cpp:

(JSC::DFG::ArgumentsSimplificationPhase::run):
(JSC::DFG::ArgumentsSimplificationPhase::detypeArgumentsReferencingPhantomChild):

  • dfg/DFGFlushFormat.cpp:

(WTF::printInternal):

  • dfg/DFGFlushFormat.h:

(JSC::DFG::resultFor):
(JSC::DFG::useKindFor):
(JSC::DFG::dataFormatFor):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::compileCurrentBlock):

  • dfg/DFGSpeculativeJIT32_64.cpp:

(JSC::DFG::SpeculativeJIT::compile):

  • dfg/DFGSpeculativeJIT64.cpp:

(JSC::DFG::SpeculativeJIT::compile):

  • dfg/DFGValueSource.h:

(JSC::DFG::ValueSource::ValueSource):
(JSC::DFG::ValueSource::forFlushFormat):

  • dfg/DFGVariableAccessData.h:

(JSC::DFG::VariableAccessData::flushFormat):

  • ftl/FTLLowerDFGToLLVM.cpp:

(JSC::FTL::LowerDFGToLLVM::buildExitArguments):

LayoutTests:

Reviewed by Oliver Hunt.

Added a test for an obvious case that I don't think we had coverage for in
microbenchmarks. Of course, this case was already covered by more complex tests.

  • js/regress/inline-arguments-aliased-access-expected.txt: Added.
  • js/regress/inline-arguments-aliased-access.html: Added.
  • js/regress/script-tests/inline-arguments-aliased-access.js: Added.

(foo):
(bar):

Location:
trunk
Files:
3 added
11 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r161067 r161072  
     12013-12-25  Filip Pizlo  <fpizlo@apple.com>
     2
     3        DFG PhantomArguments shouldn't rely on a dead Phi graph
     4        https://bugs.webkit.org/show_bug.cgi?id=126218
     5
     6        Reviewed by Oliver Hunt.
     7       
     8        Added a test for an obvious case that I don't think we had coverage for in
     9        microbenchmarks. Of course, this case was already covered by more complex tests.
     10
     11        * js/regress/inline-arguments-aliased-access-expected.txt: Added.
     12        * js/regress/inline-arguments-aliased-access.html: Added.
     13        * js/regress/script-tests/inline-arguments-aliased-access.js: Added.
     14        (foo):
     15        (bar):
     16
    1172013-12-25  Dirk Schulze  <krit@webkit.org>
    218

  • trunk/Source/JavaScriptCore/ChangeLog

    r161033 r161072  
     12013-12-25  Filip Pizlo  <fpizlo@apple.com>
     2
     3        DFG PhantomArguments shouldn't rely on a dead Phi graph
     4        https://bugs.webkit.org/show_bug.cgi?id=126218
     5
     6        Reviewed by Oliver Hunt.
     7       
     8        This change dramatically rationalizes our handling of PhantomArguments (i.e.
     9        speculative elision of arguments object allocation).
     10       
     11        It's now the case that if we decide that we can elide arguments allocation, we just
     12        turn the arguments-creating node into a PhantomArguments and mark all locals that
     13        it's stored to as being arguments aliases. Being an arguments alias and being a
     14        PhantomArguments means basically the same thing: in DFG execution you have the empty
     15        value, on OSR exit an arguments object is allocated in your place, and all operations
     16        that use the value now just refer directly to the actual arguments in the call frame
     17        header (or the arguments we know that we passed to the call, in case of inlining).
     18       
     19        This means that we no longer have arguments simplification creating a dead Phi graph
     20        that then has to be interpreted by the OSR exit logic. That sort of never made any
     21        sense.
     22       
     23        This means that PhantomArguments now has a clear story in SSA: basically SSA just
     24        gets rid of the "locals" but everything else is the same.
     25       
     26        Finally, this means that we can more easily get rid of forward exiting. As I was
     27        working on the code to get rid of forward exiting, I realized that I'd have to
     28        carefully preserve the special meanings of MovHint and SetLocal in the case of
     29        PhantomArguments. It was really bizarre: even the semantics of MovHint were tied to
     30        our specific treatment of PhantomArguments. After this change this is no longer the
     31        case.
     32       
     33        One of the really cool things about this change is that arguments reification now
     34        just becomes a special kind of FlushFormat. This further unifies things: it means
     35        that a MovHint(PhantomArguments) and a SetLocal(PhantomArguments) both have the same
     36        meaning, since both of them dictate that the way we recover the local on exit is by
     37        reifying arguments. Previously, the SetLocal(PhantomArguments) case needed some
     38        special handling to accomplish this.
     39       
     40        A downside of this approach is that we will now emit code to store the empty value
     41        into aliased arguments variables, and we will even emit code to load that empty value
     42        as well. As far as I can tell this doesn't cost anything, since PhantomArguments are
     43        most profitable in cases where it allows us to simplify control flow and kill the
     44        arguments locals entirely. Of course, this isn't an issue in SSA form since SSA form
     45        also eliminates the locals.
     46
     47        * dfg/DFGArgumentsSimplificationPhase.cpp:
     48        (JSC::DFG::ArgumentsSimplificationPhase::run):
     49        (JSC::DFG::ArgumentsSimplificationPhase::detypeArgumentsReferencingPhantomChild):
     50        * dfg/DFGFlushFormat.cpp:
     51        (WTF::printInternal):
     52        * dfg/DFGFlushFormat.h:
     53        (JSC::DFG::resultFor):
     54        (JSC::DFG::useKindFor):
     55        (JSC::DFG::dataFormatFor):
     56        * dfg/DFGSpeculativeJIT.cpp:
     57        (JSC::DFG::SpeculativeJIT::compileCurrentBlock):
     58        * dfg/DFGSpeculativeJIT32_64.cpp:
     59        (JSC::DFG::SpeculativeJIT::compile):
     60        * dfg/DFGSpeculativeJIT64.cpp:
     61        (JSC::DFG::SpeculativeJIT::compile):
     62        * dfg/DFGValueSource.h:
     63        (JSC::DFG::ValueSource::ValueSource):
     64        (JSC::DFG::ValueSource::forFlushFormat):
     65        * dfg/DFGVariableAccessData.h:
     66        (JSC::DFG::VariableAccessData::flushFormat):
     67        * ftl/FTLLowerDFGToLLVM.cpp:
     68        (JSC::FTL::LowerDFGToLLVM::buildExitArguments):
     69
    1702013-12-23  Oliver Hunt  <oliver@apple.com>
    271

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

    r159886 r161072  
    396396                        break;
    397397
    398                     ASSERT(!variableAccessData->isCaptured());
    399                    
    400                     // If this is a store into a VariableAccessData* that is marked as
    401                     // arguments aliasing for an InlineCallFrame* that does not create
    402                     // arguments, then flag the VariableAccessData as being an
    403                     // arguments-aliased. This'll let the OSR exit machinery do the right
    404                     // things. Note also that the SetLocal should become dead as soon as
    405                     // we replace all uses of this variable with GetMyArgumentsLength and
    406                     // GetMyArgumentByVal.
    407                     ASSERT(m_argumentsAliasing.find(variableAccessData)->value.isValid());
    408398                    if (variableAccessData->mergeIsArgumentsAlias(true)) {
    409399                        changed = true;
    … …  
    421411                }
    422412                   
    423                 case PhantomLocal: {
    424                     VariableAccessData* variableAccessData = node->variableAccessData();
    425                    
    426                     if (variableAccessData->isCaptured()
    427                         || !m_argumentsAliasing.find(variableAccessData)->value.isValid()
    428                         || m_createsArguments.contains(node->codeOrigin.inlineCallFrame))
    429                         break;
    430                    
    431                     // Turn PhantomLocals into just GetLocals. This will preserve the threading
    432                     // of the local through to this point, but will allow it to die, causing
    433                     // only OSR to know about it.
    434 
    435                     node->setOpAndDefaultFlags(GetLocal);
    436                     break;
    437                 }
    438 
    439413                case Flush: {
    440414                    VariableAccessData* variableAccessData = node->variableAccessData();
    … …  
    460434                    //    precisely what we don't want.
    461435                    for (unsigned i = 0; i < AdjacencyList::Size; ++i)
    462                         removeArgumentsReferencingPhantomChild(node, i);
     436                        detypeArgumentsReferencingPhantomChild(node, i);
    463437                    break;
    464438                }
    … …  
    471445                        break;
    472446                    node->convertToPhantom();
    473                     node->children.setChild1(Edge());
    474447                    break;
    475448                }
    … …  
    489462                        break;
    490463                   
    491                     node->children.child1() = node->children.child2();
    492                     node->children.child2() = Edge();
     464                    insertionSet.insertNode(
     465                        indexInBlock, SpecNone, Phantom, node->codeOrigin, node->child1());
     466                   
     467                    node->child1() = node->child2();
     468                    node->child2() = Edge();
    493469                    node->setOpAndDefaultFlags(GetMyArgumentByVal);
    494470                    changed = true;
    … …  
    504480                        break;
    505481                   
    506                     node->children.child1() = Edge();
     482                    insertionSet.insertNode(
     483                        indexInBlock, SpecNone, Phantom, node->codeOrigin, node->child1());
     484                   
     485                    node->child1() = Edge();
    507486                    node->setOpAndDefaultFlags(GetMyArgumentsLength);
    508487                    changed = true;
    … …  
    581560                        codeOrigin);
    582561                    insertionSet.insertNode(
    583                         indexInBlock, SpecNone, Phantom, codeOrigin,
    584                         children);
     562                        indexInBlock, SpecNone, Phantom, codeOrigin, children);
    585563                   
    586564                    changed = true;
    … …  
    592570                        continue;
    593571                   
    594                     node->setOpAndDefaultFlags(Phantom);
    595                     node->children.reset();
     572                    node->convertToPhantom();
    596573                    break;
    597574                }
    … …  
    628605        }
    629606       
     607        for (BlockIndex blockIndex = 0; blockIndex < m_graph.numBlocks(); ++blockIndex) {
     608            BasicBlock* block = m_graph.block(blockIndex);
     609            if (!block)
     610                continue;
     611            for (unsigned indexInBlock = 0; indexInBlock < block->size(); ++indexInBlock) {
     612                Node* node = block->at(indexInBlock);
     613                if (node->op() != Phantom)
     614                    continue;
     615                for (unsigned i = 0; i < AdjacencyList::Size; ++i)
     616                    detypeArgumentsReferencingPhantomChild(node, i);
     617            }
     618        }
     619       
    630620        if (changed) {
    631621            m_graph.dethread();
    … …  
    765755    }
    766756   
    767     void removeArgumentsReferencingPhantomChild(Node* node, unsigned edgeIndex)
     757    void detypeArgumentsReferencingPhantomChild(Node* node, unsigned edgeIndex)
    768758    {
    769759        Edge edge = node->children.child(edgeIndex);
    … …  
    772762       
    773763        switch (edge->op()) {
    774         case Phi: // Arises if we had CSE on a GetLocal of the arguments register.
    775         case GetLocal: // Arises if we had CSE on an arguments access to a variable aliased to the arguments.
    776         case SetLocal: { // Arises if we had CSE on a GetLocal of the arguments register.
     764        case GetLocal: {
    777765            VariableAccessData* variableAccessData = edge->variableAccessData();
    778             bool isDeadArgumentsRegister =
    779                 variableAccessData->local() ==
    780                     m_graph.uncheckedArgumentsRegisterFor(edge->codeOrigin)
    781                 && !m_createsArguments.contains(edge->codeOrigin.inlineCallFrame);
    782             bool isAliasedArgumentsRegister =
    783                 !variableAccessData->isCaptured()
    784                 && m_argumentsAliasing.find(variableAccessData)->value.isValid()
    785                 && !m_createsArguments.contains(edge->codeOrigin.inlineCallFrame);
    786             if (!isDeadArgumentsRegister && !isAliasedArgumentsRegister)
     766            if (!variableAccessData->isArgumentsAlias())
    787767                break;
    788             node->children.removeEdge(edgeIndex);
     768            node->children.child(edgeIndex).setUseKind(UntypedUse);
    789769            break;
    790770        }
    791771           
    792         case CreateArguments: { // Arises if we CSE two GetLocals to the arguments register and then CSE the second use of the GetLocal to the first.
    793             if (m_createsArguments.contains(edge->codeOrigin.inlineCallFrame))
    794                 break;
    795             node->children.removeEdge(edgeIndex);
     772        case PhantomArguments: {
     773            node->children.child(edgeIndex).setUseKind(UntypedUse);
    796774            break;
    797775        }

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

    r159394 r161072  
    5757        out.print("FlushedJSValue");
    5858        return;
     59    case FlushedArguments:
     60        out.print("FlushedArguments");
     61        return;
    5962    case ConflictingFlush:
    6063        out.print("ConflictingFlush");

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

    r159394 r161072  
    4747    FlushedBoolean,
    4848    FlushedJSValue,
     49    FlushedArguments,
    4950    ConflictingFlush
    5051};
    … …  
    5758    case FlushedCell:
    5859    case ConflictingFlush:
     60    case FlushedArguments:
    5961        return NodeResultJS;
    6062    case FlushedInt32:
    … …  
    7779    case FlushedJSValue:
    7880    case ConflictingFlush:
     81    case FlushedArguments:
    7982        return UntypedUse;
    8083    case FlushedCell:
    … …  
    111114    case FlushedBoolean:
    112115        return DataFormatBoolean;
     116    case FlushedArguments:
     117        return DataFormatArguments;
    113118    }
    114119    RELEASE_ASSERT_NOT_REACHED();

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

    r160919 r161072  
    14371437        VariableAccessData* variable = node->variableAccessData();
    14381438        DataFormat format;
    1439         if (variable->isArgumentsAlias())
    1440             format = DataFormatArguments;
    1441         else if (!node->refCount())
     1439        if (!node->refCount())
    14421440            continue; // No need to record dead SetLocal's.
    14431441        else

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

    r160796 r161072  
    18901890        }
    18911891           
    1892         case FlushedJSValue: {
     1892        case FlushedJSValue:
     1893        case FlushedArguments: {
    18931894            GPRTemporary result(this);
    18941895            GPRTemporary tag(this);
    … …  
    19761977        }
    19771978           
    1978         case FlushedJSValue: {
     1979        case FlushedJSValue:
     1980        case FlushedArguments: {
    19791981            if (generationInfoFromVirtualRegister(node->child1()->virtualRegister()).registerFormat() == DataFormatDouble) {
    19801982                SpeculateDoubleOperand value(this, node->child1(), ManualOperandSpeculation);
    … …  
    19901992            noResult(node);
    19911993            recordSetLocal(DataFormatJS);
    1992            
    1993             // If we're storing an arguments object that has been optimized away,
    1994             // our variable event stream for OSR exit now reflects the optimized
    1995             // value (JSValue()). On the slow path, we want an arguments object
    1996             // instead. We add an additional move hint to show OSR exit that it
    1997             // needs to reconstruct the arguments object.
    1998             if (node->child1()->op() == PhantomArguments)
    1999                 compileMovHint(node);
    20001994            break;
    20011995        }

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

    r160796 r161072  
    23002300        }
    23012301           
    2302         case FlushedJSValue: {
     2302        case FlushedJSValue:
     2303        case FlushedArguments: {
    23032304            JSValueOperand value(this, node->child1());
    23042305            m_jit.store64(value.gpr(), JITCompiler::addressFor(node->machineLocal()));
    … …  
    23062307           
    23072308            recordSetLocal(DataFormatJS);
    2308            
    2309             // If we're storing an arguments object that has been optimized away,
    2310             // our variable event stream for OSR exit now reflects the optimized
    2311             // value (JSValue()). On the slow path, we want an arguments object
    2312             // instead. We add an additional move hint to show OSR exit that it
    2313             // needs to reconstruct the arguments object.
    2314             if (node->child1()->op() == PhantomArguments)
    2315                 compileMovHint(node);
    23162309            break;
    23172310        }

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

    r159394 r161072  
    123123        : m_kind(valueSourceKind)
    124124    {
    125         ASSERT(kind() == ArgumentsSource || kind() == SourceIsDead);
     125        ASSERT(kind() == ArgumentsSource || kind() == SourceIsDead || kind() == ArgumentsSource);
    126126    }
    127127   
    … …  
    160160        case FlushedBoolean:
    161161            return ValueSource(BooleanInJSStack, where);
     162        case FlushedArguments:
     163            return ValueSource(ArgumentsSource);
    162164        }
    163165        RELEASE_ASSERT_NOT_REACHED();

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

    r160796 r161072  
    329329        ASSERT(find() == this);
    330330       
     331        if (isArgumentsAlias())
     332            return FlushedArguments;
     333       
    331334        if (!shouldUnboxIfPossible())
    332335            return FlushedJSValue;

  • trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp

    r160587 r161072  
    42104210                exit.m_values[i] = ExitValue::inJSStackAsDouble(flush.virtualRegister());
    42114211                break;
     4212               
     4213            case FlushedArguments:
     4214                // FIXME: implement PhantomArguments.
     4215                // https://bugs.webkit.org/show_bug.cgi?id=113986
     4216                RELEASE_ASSERT_NOT_REACHED();
     4217                break;
    42124218            }
    42134219        }
Note: See TracChangeset for help on using the changeset viewer.