Changeset 184288 in webkit


Ignore:
Timestamp:
May 13, 2015, 10:39:02 AM (10 years ago)
Author:
fpizlo@apple.com
Message:

REGRESSION(r184260): arguments elimination has stopped working because of Check(UntypedUse:) from SSAConversionPhase
​https://bugs.webkit.org/show_bug.cgi?id=144951

Reviewed by Michael Saboff.

There were two issues here:

  • In r184260 we expected a small number of possible use kinds in Check nodes, and UntypedUse was not one of them. That seemed like a sensible assumption because we don't create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to follow the same idiom as everyone else and not create tautological checks.


  • It's clearly not very robust to assume that Checks will not be used tautologically. So, this changes how we validate Checks in the escape analyses. We now use willHaveCheck, which catches cases that AI would have already marked as unnecessary. It then also uses a new helper called alreadyChecked(), which allows us to just ask if the check is unnecessary for objects. That's a good fall-back in case AI hadn't run yet.
  • dfg/DFGArgumentsEliminationPhase.cpp:
  • dfg/DFGMayExit.cpp:
  • dfg/DFGObjectAllocationSinkingPhase.cpp:

(JSC::DFG::ObjectAllocationSinkingPhase::handleNode):

  • dfg/DFGSSAConversionPhase.cpp:

(JSC::DFG::SSAConversionPhase::run):

  • dfg/DFGUseKind.h:

(JSC::DFG::alreadyChecked):

  • dfg/DFGVarargsForwardingPhase.cpp:
Location:
trunk/Source/JavaScriptCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/JavaScriptCore/ChangeLog

    r184287 r184288  
     12015-05-13  Filip Pizlo  <fpizlo@apple.com>
     2
     3        REGRESSION(r184260): arguments elimination has stopped working because of Check(UntypedUse:) from SSAConversionPhase
     4        https://bugs.webkit.org/show_bug.cgi?id=144951
     5
     6        Reviewed by Michael Saboff.
     7       
     8        There were two issues here:
     9       
     10        - In r184260 we expected a small number of possible use kinds in Check nodes, and
     11          UntypedUse was not one of them. That seemed like a sensible assumption because we don't
     12          create Check nodes unless it's to have a check. But, SSAConversionPhase was creating a
     13          Check that could have UntypedUse. I fixed this. It's cleaner for SSAConversionPhase to
     14          follow the same idiom as everyone else and not create tautological checks.
     15       
     16        - It's clearly not very robust to assume that Checks will not be used tautologically. So,
     17          this changes how we validate Checks in the escape analyses. We now use willHaveCheck,
     18          which catches cases that AI would have already marked as unnecessary. It then also uses
     19          a new helper called alreadyChecked(), which allows us to just ask if the check is
     20          unnecessary for objects. That's a good fall-back in case AI hadn't run yet.
     21
     22        * dfg/DFGArgumentsEliminationPhase.cpp:
     23        * dfg/DFGMayExit.cpp:
     24        * dfg/DFGObjectAllocationSinkingPhase.cpp:
     25        (JSC::DFG::ObjectAllocationSinkingPhase::handleNode):
     26        * dfg/DFGSSAConversionPhase.cpp:
     27        (JSC::DFG::SSAConversionPhase::run):
     28        * dfg/DFGUseKind.h:
     29        (JSC::DFG::alreadyChecked):
     30        * dfg/DFGVarargsForwardingPhase.cpp:
     31
     32k
    1332015-05-13  Yusuke Suzuki  <utatane.tea@gmail.com>
    234

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

    r184260 r184288  
    6464        DFG_ASSERT(m_graph, nullptr, m_graph.m_form == SSA);
    6565       
     66        if (verbose) {
     67            dataLog("Graph before arguments elimination:\n");
     68            m_graph.dump();
     69        }
     70       
    6671        identifyCandidates();
    6772        if (m_candidates.isEmpty())
    … …  
    170175                        node,
    171176                        [&] (Edge edge) {
    172                             switch (edge.useKind()) {
    173                             case CellUse:
    174                             case ObjectUse:
    175                                 break;
    176                                
    177                             default:
    178                                 escape(edge);
    179                                 break;
    180                             }
     177                            if (edge.willNotHaveCheck())
     178                                return;
     179                           
     180                            if (alreadyChecked(edge.useKind(), SpecObject))
     181                                return;
     182                           
     183                            escape(edge);
    181184                        });
    182185                    break;

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

    r183401 r184288  
    5252
    5353        switch (edge.useKind()) {
     54        // These are shady because nodes that have these use kinds will typically exit for
     55        // unrelated reasons. For example CompareEq doesn't usually exit, but if it uses ObjectUse
     56        // then it will.
    5457        case ObjectUse:
    5558        case ObjectOrOtherUse:
     59            m_result = true;
     60            break;
     61           
     62        // These are shady because they check the structure even if the type of the child node
     63        // passes the StringObject type filter.
    5664        case StringObjectUse:
    5765        case StringOrStringObjectUse:
    5866            m_result = true;
    5967            break;
     68           
    6069        default:
    6170            break;

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

    r184260 r184288  
    842842                node,
    843843                [&] (Edge edge) {
    844                     bool ok = true;
    845 
    846                     switch (edge.useKind()) {
    847                     case KnownCellUse:
    848                     case CellUse:
    849                     case ObjectUse:
    850                         // All of our allocations will pass this.
    851                         break;
    852                        
    853                     case FunctionUse:
    854                         // Function allocations will pass this.
    855                         if (edge->op() != NewFunction)
    856                             ok = false;
    857                         break;
    858                        
    859                     default:
    860                         ok = false;
    861                         break;
    862                     }
    863                    
    864                     if (!ok)
    865                         escape(edge.node());
     844                    if (edge.willNotHaveCheck())
     845                        return;
     846                   
     847                    if (alreadyChecked(edge.useKind(), SpecObject))
     848                        return;
     849                   
     850                    escape(edge.node());
    866851                });
    867852            break;

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

    r183497 r184288  
    277277                case SetLocal: {
    278278                    VariableAccessData* variable = node->variableAccessData();
     279                    Node* child = node->child1().node();
    279280                   
    280281                    if (!!(node->flags() & NodeIsFlushed)) {
    … …  
    283284                                variable->local(), variable->flushFormat()));
    284285                    } else
    285                         node->setOpAndDefaultFlags(Check);
     286                        node->remove();
    286287                   
    287288                    if (verbose)
    288                         dataLog("Mapping: ", variable->local(), " -> ", node->child1().node(), "\n");
    289                     valueForOperand.operand(variable->local()) = node->child1().node();
     289                        dataLog("Mapping: ", variable->local(), " -> ", child, "\n");
     290                    valueForOperand.operand(variable->local()) = child;
    290291                    break;
    291292                }

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

    r176425 r184288  
    214214}
    215215
     216// Returns true if we've already guaranteed the type
     217inline bool alreadyChecked(UseKind kind, SpeculatedType type)
     218{
     219    // If the check involves the structure then we need to know more than just the type to be sure
     220    // that the check is done.
     221    if (usesStructure(kind))
     222        return false;
     223   
     224    return !(type & ~typeFilterFor(kind));
     225}
     226
    216227inline UseKind useKindForResult(NodeFlags result)
    217228{

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

    r184260 r184288  
    110110                    node,
    111111                    [&] (Edge edge) {
    112                         switch (edge.useKind()) {
    113                         case CellUse:
    114                         case ObjectUse:
    115                             if (edge == candidate)
    116                                 lastUserIndex = nodeIndex;
    117                             break;
    118                         default:
    119                             sawEscape = true;
    120                             break;
    121                         } 
     112                        if (edge == candidate)
     113                            lastUserIndex = nodeIndex;
     114                       
     115                        if (edge.willNotHaveCheck())
     116                            return;
     117                       
     118                        if (alreadyChecked(edge.useKind(), SpecObject))
     119                            return;
     120                       
     121                        sawEscape = true;
    122122                    });
    123123                if (sawEscape) {
Note: See TracChangeset for help on using the changeset viewer.