Graph::methodOfGettingAValueProfileFor() should be returning the profile for the...
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 21:19:52 +0000 (21:19 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 10 Nov 2016 21:19:52 +0000 (21:19 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@208560 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/osr-exit-on-op-negate-should-no-fail-assertions.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGGraph.cpp
Source/JavaScriptCore/dfg/DFGGraph.h
Source/JavaScriptCore/dfg/DFGJITCompiler.cpp
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index b2a19b2..135a420 100644 (file)
@@ -1,3 +1,13 @@
+2016-11-10  Mark Lam  <mark.lam@apple.com>
+
+        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.
+
+        * stress/osr-exit-on-op-negate-should-no-fail-assertions.js: Added.
+
 2016-11-08  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Avoid cloned arguments allocation in ArrayPrototype methods
diff --git a/JSTests/stress/osr-exit-on-op-negate-should-no-fail-assertions.js b/JSTests/stress/osr-exit-on-op-negate-should-no-fail-assertions.js
new file mode 100644 (file)
index 0000000..708de63
--- /dev/null
@@ -0,0 +1,22 @@
+//@ runFTLNoCJIT
+// This test passes if it does not crash or fail any assertions.
+
+function inlineable(x) {
+    return -x;
+}
+
+function test(y) {
+    var results = [];
+    for (var j = 0; j < 300; j++) {
+        var k = j % y.length;
+        try {
+            results.push(inlineable(y[k]));
+        } catch (e) {
+        }
+    }
+}
+noInline(test);
+
+for (var i = 0; i < 1000; i++) {
+    test([false, -Infinity, Infinity, 0x50505050, undefined]);
+}
index afb07c6..85abc67 100644 (file)
@@ -1,3 +1,68 @@
+2016-11-10  Mark Lam  <mark.lam@apple.com>
+
+        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.
+
+        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):
+
 2016-11-10  Aaron Chu  <aaron_chu@apple.com>
 
         Web Inspector: AXI: clarify button roles (e.g. toggle or popup button)
index 3dd2358..a29b86d 100644 (file)
@@ -1613,45 +1613,47 @@ ControlEquivalenceAnalysis& Graph::ensureControlEquivalenceAnalysis()
     return *m_controlEquivalenceAnalysis;
 }
 
-MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* node)
-{
-    while (node) {
-        CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
-        
-        if (node->accessesStack(*this)) {
-            ValueProfile* result = [&] () -> ValueProfile* {
-                if (!node->local().isArgument())
-                    return nullptr;
-                int argument = node->local().toArgument();
-                Node* argumentNode = m_arguments[argument];
-                if (!argumentNode)
-                    return nullptr;
-                if (node->variableAccessData() != argumentNode->variableAccessData())
-                    return nullptr;
-                return profiledBlock->valueProfileForArgument(argument);
-            }();
-            if (result)
-                return result;
-            
-            if (node->op() == GetLocal) {
-                return MethodOfGettingAValueProfile::fromLazyOperand(
-                    profiledBlock,
-                    LazyOperandValueProfileKey(
-                        node->origin.semantic.bytecodeIndex, node->local()));
+MethodOfGettingAValueProfile Graph::methodOfGettingAValueProfileFor(Node* currentNode, Node* operandNode)
+{
+    for (Node* node = operandNode; node;) {
+        // currentNode is null when we're doing speculation checks for checkArgumentTypes().
+        if (!currentNode || node->origin != currentNode->origin) {
+            CodeBlock* profiledBlock = baselineCodeBlockFor(node->origin.semantic);
+
+            if (node->accessesStack(*this)) {
+                ValueProfile* result = [&] () -> ValueProfile* {
+                    if (!node->local().isArgument())
+                        return nullptr;
+                    int argument = node->local().toArgument();
+                    Node* argumentNode = m_arguments[argument];
+                    if (!argumentNode)
+                        return nullptr;
+                    if (node->variableAccessData() != argumentNode->variableAccessData())
+                        return nullptr;
+                    return profiledBlock->valueProfileForArgument(argument);
+                }();
+                if (result)
+                    return result;
+
+                if (node->op() == GetLocal) {
+                    return MethodOfGettingAValueProfile::fromLazyOperand(
+                        profiledBlock,
+                        LazyOperandValueProfileKey(
+                            node->origin.semantic.bytecodeIndex, node->local()));
+                }
             }
-        }
-        
-        if (node->hasHeapPrediction())
-            return profiledBlock->valueProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);
-        
-        {
+
+            if (node->hasHeapPrediction())
+                return profiledBlock->valueProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex);
+
             if (profiledBlock->hasBaselineJITProfiling()) {
                 if (ArithProfile* result = profiledBlock->arithProfileForBytecodeOffset(node->origin.semantic.bytecodeIndex))
                     return result;
             }
         }
-        
+
         switch (node->op()) {
+        case BooleanToNumber:
         case Identity:
         case ValueRep:
         case DoubleRep:
index a4c3f40..6417b85 100644 (file)
@@ -417,7 +417,7 @@ public:
         return hasExitSite(node->origin.semantic, exitKind);
     }
     
-    MethodOfGettingAValueProfile methodOfGettingAValueProfileFor(Node*);
+    MethodOfGettingAValueProfile methodOfGettingAValueProfileFor(Node* currentNode, Node* operandNode);
     
     BlockIndex numBlocks() const { return m_blocks.size(); }
     BasicBlock* block(BlockIndex blockIndex) const { return m_blocks[blockIndex].get(); }
index d35fdc5..0ac6aff 100644 (file)
@@ -588,7 +588,7 @@ void JITCompiler::noticeOSREntry(BasicBlock& basicBlock, JITCompiler::Label bloc
 
 void JITCompiler::appendExceptionHandlingOSRExit(ExitKind kind, unsigned eventStreamIndex, CodeOrigin opCatchOrigin, HandlerInfo* exceptionHandler, CallSiteIndex callSite, MacroAssembler::JumpList jumpsToFail)
 {
-    OSRExit exit(kind, JSValueRegs(), graph().methodOfGettingAValueProfileFor(nullptr), m_speculative.get(), eventStreamIndex);
+    OSRExit exit(kind, JSValueRegs(), MethodOfGettingAValueProfile(), m_speculative.get(), eventStreamIndex);
     exit.m_codeOrigin = opCatchOrigin;
     exit.m_exceptionHandlerCallSiteIndex = callSite;
     OSRExitCompilationInfo& exitInfo = appendExitInfo(jumpsToFail);
index 3eb730f..7e8f8cf 100644 (file)
@@ -251,7 +251,7 @@ void SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource
         m_jit.appendExitInfo(jumpsToFail);
     } else
         m_jit.appendExitInfo(jumpToFail);
-    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
+    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
 }
 
 void SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource, Node* node, const MacroAssembler::JumpList& jumpsToFail)
@@ -266,7 +266,7 @@ void SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource
         m_jit.appendExitInfo(myJumpsToFail);
     } else
         m_jit.appendExitInfo(jumpsToFail);
-    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
+    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
 }
 
 OSRExitJumpPlaceholder SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource, Node* node)
@@ -275,7 +275,7 @@ OSRExitJumpPlaceholder SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSo
         return OSRExitJumpPlaceholder();
     unsigned index = m_jit.jitCode()->osrExit.size();
     m_jit.appendExitInfo();
-    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size()));
+    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size()));
     return OSRExitJumpPlaceholder(index);
 }
 
@@ -300,7 +300,7 @@ void SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource
         return;
     unsigned recoveryIndex = m_jit.jitCode()->appendSpeculationRecovery(recovery);
     m_jit.appendExitInfo(jumpToFail);
-    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(node), this, m_stream->size(), recoveryIndex));
+    m_jit.jitCode()->appendOSRExit(OSRExit(kind, jsValueSource, m_jit.graph().methodOfGettingAValueProfileFor(m_currentNode, node), this, m_stream->size(), recoveryIndex));
 }
 
 void SpeculativeJIT::speculationCheck(ExitKind kind, JSValueSource jsValueSource, Edge nodeUse, MacroAssembler::Jump jumpToFail, const SpeculationRecovery& recovery)
@@ -314,8 +314,7 @@ void SpeculativeJIT::emitInvalidationPoint(Node* node)
         return;
     OSRExitCompilationInfo& info = m_jit.appendExitInfo(JITCompiler::JumpList());
     m_jit.jitCode()->appendOSRExit(OSRExit(
-        UncountableInvalidation, JSValueSource(),
-        m_jit.graph().methodOfGettingAValueProfileFor(node),
+        UncountableInvalidation, JSValueSource(), MethodOfGettingAValueProfile(),
         this, m_stream->size()));
     info.m_replacementSource = m_jit.watchpointLabel();
     ASSERT(info.m_replacementSource.isSet());
index e0197a1..66c2d8f 100644 (file)
@@ -12412,7 +12412,7 @@ private:
     OSRExitDescriptor* appendOSRExitDescriptor(FormattedValue lowValue, Node* highValue)
     {
         return &m_ftlState.jitCode->osrExitDescriptors.alloc(
-            lowValue.format(), m_graph.methodOfGettingAValueProfileFor(highValue),
+            lowValue.format(), m_graph.methodOfGettingAValueProfileFor(m_node, highValue),
             availabilityMap().m_locals.numberOfArguments(),
             availabilityMap().m_locals.numberOfLocals());
     }