Changeset 208560 in webkit


Ignore:
Timestamp:
Nov 10, 2016, 1:19:52 PM (9 years ago)
Author:
mark.lam@apple.com
Message:

Graph::methodOfGettingAValueProfileFor() should be returning the profile for the operand node.
​https://bugs.webkit.org/show_bug.cgi?id=164600
<rdar://problem/28828676>

Reviewed by Filip Pizlo.

JSTests:

  • stress/osr-exit-on-op-negate-should-no-fail-assertions.js: Added.

Source/JavaScriptCore:

Currently, Graph::methodOfGettingAValueProfileFor() assumes that the operand DFG
node that it is provided with always has a different origin than the node that is
using that operand. For example, in a DFG graph that looks like this:

a: ...
b: ArithAdd(@a, ...)

... when emitting speculation checks on @a for the ArithAdd node at @b,
Graph::methodOfGettingAValueProfileFor() is passed @a, and expects @a's to
originate from a different bytecode than @b. The intent here is to get the
profile for @a so that the OSR exit ramp for @b can update @a's profile with the
observed result type from @a so that future type prediction on incoming args for
the ArithAdd node can take this into consideration.

However, op_negate can be compiled into the following series of nodes:

a: ...
b: BooleanToNumber(@a)
c: DoubleRep(@b)
d: ArithNegate(@c)

All 3 nodes @b, @c, and @d maps to the same op_negate bytecode i.e. they have the
same origin. When the speculativeJIT emits a speculationCheck for DoubleRep, it
calls Graph::methodOfGettingAValueProfileFor() to get the ArithProfile for the
BooleanToNumber node. But because all 3 nodes have the same origin,
Graph::methodOfGettingAValueProfileFor() erroneously returns the ArithProfile for
the op_negate. Subsequently, the OSR exit ramp will modify the ArithProfile of
the op_negate and corrupt its profile. Instead, what the OSR exit ramp should be
doing is update the ArithProfile of op_negate's operand i.e. BooleanToNumber's
operand @a in this case.

The fix is to always pass the current node we're generating code for (in addition
to the operand node) to Graph::methodOfGettingAValueProfileFor(). This way, we
know the profile is valid if and only if the current node and its operand node
does not have the same origin.

In this patch, we also fixed the following:

  1. Teach Graph::methodOfGettingAValueProfileFor() to get the profile for BooleanToNumber's operand if the operand node it is given is BooleanToNumber.
  2. Change JITCompiler::appendExceptionHandlingOSRExit() to explicitly pass an empty MethodOfGettingAValueProfile(). It was implicitly doing this before.
  3. Change SpeculativeJIT::emitInvalidationPoint() to pass an empty MethodOfGettingAValueProfile(). It has no child node. Hence, it doesn't make sense to call Graph::methodOfGettingAValueProfileFor() for a child node that does not exist.
  • dfg/DFGGraph.cpp:

(JSC::DFG::Graph::methodOfGettingAValueProfileFor):

  • dfg/DFGGraph.h:
  • dfg/DFGJITCompiler.cpp:

(JSC::DFG::JITCompiler::appendExceptionHandlingOSRExit):

  • dfg/DFGSpeculativeJIT.cpp:

(JSC::DFG::SpeculativeJIT::speculationCheck):
(JSC::DFG::SpeculativeJIT::emitInvalidationPoint):

  • ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::appendOSRExitDescriptor):

Location:
trunk
Files:
1 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/JSTests/ChangeLog

    r208524 r208560  
     12016-11-10  Mark Lam  <mark.lam@apple.com>
     2
     3        Graph::methodOfGettingAValueProfileFor() should be returning the profile for the operand node.
     4        https://bugs.webkit.org/show_bug.cgi?id=164600
     5        <rdar://problem/28828676>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        * stress/osr-exit-on-op-negate-should-no-fail-assertions.js: Added.
     10
    1112016-11-08  Yusuke Suzuki  <utatane.tea@gmail.com>
    212

  • trunk/Source/JavaScriptCore/ChangeLog

    r208540 r208560  
     12016-11-10  Mark Lam  <mark.lam@apple.com>
     2
     3        Graph::methodOfGettingAValueProfileFor() should be returning the profile for the operand node.
     4        https://bugs.webkit.org/show_bug.cgi?id=164600
     5        <rdar://problem/28828676>
     6
     7        Reviewed by Filip Pizlo.
     8
     9        Currently, Graph::methodOfGettingAValueProfileFor() assumes that the operand DFG
     10        node that it is provided with always has a different origin than the node that is
     11        using that operand.  For example, in a DFG graph that looks like this:
     12
     13            a: ...
     14            b: ArithAdd(@a, ...)
     15
     16        ... when emitting speculation checks on @a for the ArithAdd node at @b,
     17        Graph::methodOfGettingAValueProfileFor() is passed @a, and expects @a's to
     18        originate from a different bytecode than @b.  The intent here is to get the
     19        profile for @a so that the OSR exit ramp for @b can update @a's profile with the
     20        observed result type from @a so that future type prediction on incoming args for
     21        the ArithAdd node can take this into consideration.
     22
     23        However, op_negate can be compiled into the following series of nodes:
     24
     25            a: ...
     26            b: BooleanToNumber(@a)
     27            c: DoubleRep(@b)
     28            d: ArithNegate(@c)
     29
     30        All 3 nodes @b, @c, and @d maps to the same op_negate bytecode i.e. they have the
     31        same origin.  When the speculativeJIT emits a speculationCheck for DoubleRep, it
     32        calls Graph::methodOfGettingAValueProfileFor() to get the ArithProfile for the
     33        BooleanToNumber node.  But because all 3 nodes have the same origin,
     34        Graph::methodOfGettingAValueProfileFor() erroneously returns the ArithProfile for
     35        the op_negate.  Subsequently, the OSR exit ramp will modify the ArithProfile of
     36        the op_negate and corrupt its profile.  Instead, what the OSR exit ramp should be
     37        doing is update the ArithProfile of op_negate's operand i.e. BooleanToNumber's
     38        operand @a in this case.
     39
     40        The fix is to always pass the current node we're generating code for (in addition
     41        to the operand node) to Graph::methodOfGettingAValueProfileFor().  This way, we
     42        know the profile is valid if and only if the current node and its operand node
     43        does not have the same origin.
     44
     45        In this patch, we also fixed the following:
     46        1. Teach Graph::methodOfGettingAValueProfileFor() to get the profile for
     47           BooleanToNumber's operand if the operand node it is given is BooleanToNumber.
     48        2. Change JITCompiler::appendExceptionHandlingOSRExit() to explicitly pass an
     49           empty MethodOfGettingAValueProfile().  It was implicitly doing this before.
     50        3. Change SpeculativeJIT::emitInvalidationPoint() to pass an empty
     51           MethodOfGettingAValueProfile().  It has no child node.  Hence, it doesn't
     52           make sense to call Graph::methodOfGettingAValueProfileFor() for a child node
     53           that does not exist.
     54
     55        * dfg/DFGGraph.cpp:
     56        (JSC::DFG::Graph::methodOfGettingAValueProfileFor):
     57        * dfg/DFGGraph.h:
     58        * dfg/DFGJITCompiler.cpp:
     59        (JSC::DFG::JITCompiler::appendExceptionHandlingOSRExit):
     60        * dfg/DFGSpeculativeJIT.cpp:
     61        (JSC::DFG::SpeculativeJIT::speculationCheck):
     62        (JSC::DFG::SpeculativeJIT::emitInvalidationPoint):
     63        * ftl/FTLLowerDFGToB3.cpp:
     64        (JSC::FTL::DFG::LowerDFGToB3::appendOSRExitDescriptor):
     65
    1662016-11-10  Aaron Chu  <aaron_chu@apple.com>
    267

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

    r208524 r208560  
    16141614}
    16151615
    1616 MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* node)
    1617 {
    1618     while (node) {
    1619         CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
    1620        
    1621         if (node->accessesStack(*this)) {
    1622             ValueProfile* result = [&] () -> ValueProfile* {
    1623                 if (!node->local().isArgument())
    1624                     return nullptr;
    1625                 int argument = node->local().toArgument();
    1626                 Node* argumentNode = m_arguments[argument];
    1627                 if (!argumentNode)
    1628                     return nullptr;
    1629                 if (node->variableAccessData() != argumentNode->variableAccessData())
    1630                     return nullptr;
    1631                 return profiledBlock->valueProfileForArgument(argument);
    1632             }();
    1633             if (result)
    1634                 return result;
    1635            
    1636             if (node->op() == GetLocal) {
    1637                 return MethodOfGettingAValueProfile::fromLazyOperand(
    1638                     profiledBlock,
    1639                     LazyOperandValueProfileKey(
    1640                         node->origin.semantic.bytecodeIndex, node->local()));
     1616MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* currentNode, Node* operandNode)
     1617{
     1618    for (Node* node = operandNode; node;) {
     1619        // currentNode is null when we're doing speculation checks for checkArgumentTypes().
     1620        if (!currentNode || node->origin != currentNode->origin) {
     1621            CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
     1622
     1623            if (node->accessesStack(*this)) {
     1624                ValueProfile* result = [&] () -> ValueProfile* {
     1625                    if (!node->local().isArgument())
     1626                        return nullptr;
     1627                    int argument = node->local().toArgument();
     1628                    Node* argumentNode = m_arguments[argument];
     1629                    if (!argumentNode)
     1630                        return nullptr;
     1631                    if (node->variableAccessData() != argumentNode->variableAccessData())
     1632                        return nullptr;
     1633                    return profiledBlock->valueProfileForArgument(argument);
     1634                }();
     1635                if (result)
     1636                    return result;
     1637
     1638                if (node->op() == GetLocal) {
     1639                    return MethodOfGettingAValueProfile::fromLazyOperand(
     1640                        profiledBlock,
     1641                        LazyOperandValueProfileKey(
     1642                            node->origin.semantic.bytecodeIndex, node->local()));
     1643                }
    16411644            }
    1642         }
    1643        
    1644         if (node->hasHeapPrediction())
    1645             return profiledBlock->valueProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);
    1646        
    1647         {
     1645
     1646            if (node->hasHeapPrediction())
     1647                return profiledBlock->valueProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);
     1648
    16481649            if (profiledBlock->hasBaselineJITProfiling()) {
    16491650                if (ArithProfile* result = profiledBlock->arithProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex))
    … …  
    16511652            }
    16521653        }
    1653        
     1654
    16541655        switch (node->op()) {
     1656        case BooleanToNumber:
    16551657        case Identity:
    16561658        case ValueRep:

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

    r208373 r208560  
    418418    }
    419419   
    420     MethodOfGettingAValueProfile methodOfGettingAValueProfileFor(Node*);
     420    MethodOfGettingAValueProfile methodOfGettingAValueProfileFor(Node* currentNode, Node* operandNode);
    421421   
    422422    BlockIndex numBlocks() const { return m_blocks.size(); }

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

    r207475 r208560  
    589589void JITCompiler::appendExceptionHandlingOSRExit(ExitKind kind, unsigned eventStreamIndex, CodeOrigin opCatchOrigin, HandlerInfo* exceptionHandler, CallSiteIndex callSite, MacroAssembler::JumpList jumpsToFail)
    590590{
    591     OSRExit exit(kind, JSValueRegs(), graph().methodOfGettingAValueProfileFor(nullptr), m_speculative.get(), eventStreamIndex);
     591    OSRExit exit(kind, JSValueRegs(), MethodOfGettingAValueProfile(), m_speculative.get(), eventStreamIndex);
    592592    exit.m_codeOrigin = opCatchOrigin;
    593593    exit.m_exceptionHandlerCallSiteIndex = callSite;

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

    r208524 r208560  
    252252    } else
    253253        m_jit.appendExitInfo(jumpToFail);
    254     m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
     254    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
    255255}
    256256
    … …  
    267267    } else
    268268        m_jit.appendExitInfo(jumpsToFail);
    269     m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
     269    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
    270270}
    271271
    … …  
    276276    unsigned index = m_jit.jitCode()->osrExit.size();
    277277    m_jit.appendExitInfo();
    278     m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
     278    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
    279279    return OSRExitJumpPlaceholder(index);
    280280}
    … …  
    301301    unsigned recoveryIndex = m_jit.jitCode()->appendSpeculationRecovery(recovery);
    302302    m_jit.appendExitInfo(jumpToFail);
    303     m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size(), recoveryIndex));
     303    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size(), recoveryIndex));
    304304}
    305305
    … …  
    315315    OSRExitCompilationInfo& info = m_jit.appendExitInfo(JITCompiler::JumpList());
    316316    m_jit.jitCode()->appendOSRExit(OSRExit(
    317         UncountableInvalidation, JSValueSource(),
    318         m_jit.graph().methodOfGettingAValueProfileFor(node),
     317        UncountableInvalidation, JSValueSource(), MethodOfGettingAValueProfile(),
    319318        this, m_stream->size()));
    320319    info.m_replacementSource = m_jit.watchpointLabel();

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

    r208524 r208560  
    1241312413    {
    1241412414        return &m_ftlState.jitCode->osrExitDescriptors.alloc(
    12415             lowValue.format(), m_graph.methodOfGettingAValueProfileFor(highValue),
     12415            lowValue.format(), m_graph.methodOfGettingAValueProfileFor(m_node, highValue),
    1241612416            availabilityMap().m_locals.numberOfArguments(),
    1241712417            availabilityMap().m_locals.numberOfLocals());
Note: See TracChangeset for help on using the changeset viewer.