Attachment 349950 Details for Bug 189682 – patch for landing

[patch] patch for landing

a-backup.diff (text/plain), 8.34 KB, created by Saam Barati on 2018-09-17 15:16:35 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-09-17 15:16:35 PDT

Size: 8.34 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 236086)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,14 @@
>+2018-09-17  Saam barati  <sbarati@apple.com>
>+
>+        DFGValidate should only validate that edges have a result in SSA if we haven't proven that we have OSR exited
>+        https://bugs.webkit.org/show_bug.cgi?id=189682
>+        <rdar://problem/43557315>
>+
>+        Reviewed by Mark Lam.
>+
>+        * stress/arguments-elimination-will-generate-edge-without-result.js: Added.
>+        (foo):
>+
> 2018-09-14  Saam barati  <sbarati@apple.com>
> 
>         Don't dump OSRAvailabilityData in Graph::dump because a stale Availability may point to a Node that is already freed
>Index: JSTests/stress/arguments-elimination-will-generate-edge-without-result.js
>===================================================================
>--- JSTests/stress/arguments-elimination-will-generate-edge-without-result.js	(nonexistent)
>+++ JSTests/stress/arguments-elimination-will-generate-edge-without-result.js	(working copy)
>@@ -0,0 +1,9 @@
>+//@ runDefault("--validateGraphAtEachPhase=true", "--jitPolicyScale=0", "--useConcurrentJIT=0")
>+
>+'use strict';
>+function foo() {
>+  return arguments[1][0] === arguments[0];
>+}
>+for (let i = 0; i < 100000; ++i) {
>+  foo(0, 0);
>+}
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 236078)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,21 @@
>+2018-09-17  Saam barati  <sbarati@apple.com>
>+
>+        DFGValidate should only validate that edges have a result in SSA if we haven't proven that we have OSR exited
>+        https://bugs.webkit.org/show_bug.cgi?id=189682
>+        <rdar://problem/43557315>
>+
>+        Reviewed by Mark Lam.
>+
>+        The arguments elimination phase may generate code where a node after an
>+        OSR exit will have a child edge to a node that does not have a result.
>+        This is because the arguments elimination phase stops doing transformations
>+        once it encounters a node inside of a block that is guaranteed OSR exit.
>+        
>+        This patch makes it so that validation does not fail on code like this
>+        since it is valid.
>+
>+        * dfg/DFGValidate.cpp:
>+
> 2018-09-17  Darin Adler  <darin@apple.com>
> 
>         Use OpaqueJSString rather than JSRetainPtr inside WebKit
>Index: Source/JavaScriptCore/dfg/DFGValidate.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGValidate.cpp	(revision 236078)
>+++ Source/JavaScriptCore/dfg/DFGValidate.cpp	(working copy)
>@@ -108,59 +108,64 @@ public:
>             BasicBlock* block = m_graph.block(blockIndex);
>             if (!block)
>                 continue;
>+            bool isOSRExited = false;
>             for (size_t i = 0; i < block->numNodes(); ++i) {
>                 Node* node = block->node(i);
>                 m_acceptableNodes.add(node);
>-                if (!node->shouldGenerate())
>-                    continue;
>-                if (node->op() == Upsilon) {
>-                    VALIDATE((node), m_graph.m_form == SSA);
>-                    if (node->phi()->shouldGenerate())
>-                        m_myRefCounts.find(node)->value++;
>-                }
>-                for (unsigned j = 0; j < m_graph.numChildren(node); ++j) {
>-                    // Phi children in LoadStore form are invalid.
>-                    if (m_graph.m_form == LoadStore && block->isPhiIndex(i))
>-                        continue;
>-                    
>-                    Edge edge = m_graph.child(node, j);
>-                    if (!edge)
>-                        continue;
>-                    
>-                    m_myRefCounts.find(edge.node())->value++;
>-
>-                    validateEdgeWithDoubleResultIfNecessary(node, edge);
>-                    VALIDATE((node, edge), edge->hasInt52Result() == (edge.useKind() == Int52RepUse));
>-                    
>-                    if (m_graph.m_form == SSA) {
>-                        // In SSA, all edges must hasResult().
>-                        VALIDATE((node, edge), edge->hasResult());
>-                        continue;
>+                if (node->shouldGenerate()) {
>+                    if (node->op() == Upsilon) {
>+                        VALIDATE((node), m_graph.m_form == SSA);
>+                        if (node->phi()->shouldGenerate())
>+                            m_myRefCounts.find(node)->value++;
>                     }
>-                    
>-                    // Unless I'm a Flush, Phantom, GetLocal, or Phi, my children should hasResult().
>-                    switch (node->op()) {
>-                    case Flush:
>-                    case GetLocal:
>-                        VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>-                        VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>-                        break;
>-                    case PhantomLocal:
>-                        VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>-                        VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>-                        VALIDATE((node, edge), edge->op() != SetLocal);
>-                        break;
>-                    case Phi:
>-                        VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>-                        if (m_graph.m_unificationState == LocallyUnified)
>+
>+                    for (unsigned j = 0; j < m_graph.numChildren(node); ++j) {
>+                        // Phi children in LoadStore form are invalid.
>+                        if (m_graph.m_form == LoadStore && block->isPhiIndex(i))
>+                            continue;
>+                        
>+                        Edge edge = m_graph.child(node, j);
>+                        if (!edge)
>+                            continue;
>+                        
>+                        m_myRefCounts.find(edge.node())->value++;
>+
>+                        validateEdgeWithDoubleResultIfNecessary(node, edge);
>+                        VALIDATE((node, edge), edge->hasInt52Result() == (edge.useKind() == Int52RepUse));
>+                        
>+                        if (m_graph.m_form == SSA) {
>+                            // In SSA, all edges must hasResult(). We also have phases that will break
>+                            // this if they guarantee an exit will happen before such a node executes.
>+                            VALIDATE((node, edge), edge->hasResult() || isOSRExited);
>+                            continue;
>+                        }
>+                        
>+                        // Unless I'm a Flush, Phantom, GetLocal, or Phi, my children should hasResult().
>+                        switch (node->op()) {
>+                        case Flush:
>+                        case GetLocal:
>+                            VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>+                            VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>                             break;
>-                        VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>-                        break;
>-                    default:
>-                        VALIDATE((node, edge), edge->hasResult());
>-                        break;
>+                        case PhantomLocal:
>+                            VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>+                            VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>+                            VALIDATE((node, edge), edge->op() != SetLocal);
>+                            break;
>+                        case Phi:
>+                            VALIDATE((node, edge), edge->hasVariableAccessData(m_graph));
>+                            if (m_graph.m_unificationState == LocallyUnified)
>+                                break;
>+                            VALIDATE((node, edge), edge->variableAccessData() == node->variableAccessData());
>+                            break;
>+                        default:
>+                            VALIDATE((node, edge), edge->hasResult());
>+                            break;
>+                        }
>                     }
>                 }
>+
>+                isOSRExited |= node->isPseudoTerminal();
>             }
>         }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 189682: 349947 | 349950 | 349979 | 350298