Attachment 237337 Details for Bug 130115 – proposed patch

[patch] proposed patch

dce-bug-130115.diff (text/plain), 5.79 KB, created by Gergő Balogh on 2014-08-29 00:03:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Gergő Balogh

Created: 2014-08-29 00:03:29 PDT

Size: 5.79 KB

patch

obsolete

>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 0692853..07c1eb8 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,16 @@
>+2014-08-28  Gergo Balogh  <gbalogh.u-szeged@partner.samsung.com>
>+
>+        DCE claims to preserve CPS but does not preserve it in the strictest form, specifically variables-at-tail for captured variables.
>+        Nhttps://bugs.webkit.org/show_bug.cgi?id=130115
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * dfg/DFGDCEPhase.cpp:
>+        (JSC::DFG::DCEPhase::fixupBlock):
>+        (JSC::DFG::DCEPhase::cleanVariables):
>+        (JSC::DFG::DCEPhase::cleanVariablesAtTail):
>+        (JSC::DFG::performDCE): Deleted.
>+
> 2014-08-26  Brian J. Burg  <burg@cs.washington.edu>
> 
>         Web Inspector: put feature flags for Inspector domains in the protocol specification
>diff --git a/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp b/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
>index a69eb00..365b149 100644
>--- a/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGDCEPhase.cpp
>@@ -210,7 +210,7 @@ private:
>             }
>             
>             cleanVariables(block->variablesAtHead);
>-            cleanVariables(block->variablesAtTail);
>+            cleanVariablesAtTail(block, block->variablesAtTail);
>             break;
>         }
>             
>@@ -285,10 +285,27 @@ private:
>                 continue;
>             if (node->op() != Phantom && node->op() != Check && node->shouldGenerate())
>                 continue;
>+            RELEASE_ASSERT(node->op() != GetLocal);
>+            variables[i] = 0;
>+        }
>+    }
>+    
>+    template<typename VariablesVectorType>
>+    void cleanVariablesAtTail(BasicBlock* block, VariablesVectorType& variables)
>+    {
>+        bool recalcVars = false;
>+        BitVector varsToRecalc;
>+
>+        for (unsigned i = variables.size(); i--;) {
>+            Node* node = variables[i];
>+            if (!node)
>+                continue;
>+            if (node->op() != Phantom && node->shouldGenerate())
>+                continue;
>             if (node->op() == GetLocal) {
>                 node = node->child1().node();
>-                
>-                // FIXME: In the case that the variable is captured, we really want to be able
>+
>+                // In the case that the variable is captured, we really want to be able
>                 // to replace the variable-at-tail with the last use of the variable in the same
>                 // way that CPS rethreading would do. The child of the GetLocal isn't necessarily
>                 // the same as what CPS rethreading would do. For example, we may have:
>@@ -297,36 +314,51 @@ private:
>                 // b: GetLocal(@a) // live
>                 // c: GetLocal(@a) // dead
>                 //
>-                // When killing @c, the code below will set the variable-at-tail to @a, while CPS
>-                // rethreading would have set @b. This is a benign bug, since all clients of CPS
>-                // only use the variable-at-tail of captured variables to get the
>-                // VariableAccessData and observe that it is in fact captured. But, this feels
>-                // like it could cause bugs in the future.
>-                //
>-                // It's tempting to just dethread and then invoke CPS rethreading, but CPS
>-                // rethreading fails to preserve exact ref-counts. So we would need a fixpoint.
>-                // It's probably the case that this fixpoint will be guaranteed to converge after
>-                // the second iteration (i.e. the second run of DCE will not kill anything and so
>-                // will not need to dethread), but for now the safest approach is probably just to
>-                // allow for this tiny bit of sloppiness.
>-                //
>-                // Another possible solution would be to simply say that DCE dethreads but then
>-                // we never rethread before going to the backend. That feels intuitively right
>-                // because it's unlikely that any of the phases after DCE in the backend rely on
>-                // ThreadedCPS.
>-                //
>-                // https://bugs.webkit.org/show_bug.cgi?id=130115
>+                // When killing @c, variables[i] = node; in the code below will set the 
>+                // variable-at-tail to @a, while CPS rethreading would have set @b.
>+                // Thus, in such cases we iterate through all the nodes of the block and mimic
>+                // CPS rethreading more closely.
>                 ASSERT(
>                     node->op() == Phi || node->op() == SetArgument
>                     || node->variableAccessData()->isCaptured());
>                 
>                 if (node->shouldGenerate()) {
>+                    if (!recalcVars) {
>+                        recalcVars = true;
>+                        varsToRecalc.ensureSize(variables.size());
>+                    }
>+                    varsToRecalc.quickSet(i);
>+
>                     variables[i] = node;
>                     continue;
>                 }
>             }
>             variables[i] = 0;
>         }
>+
>+        if (!recalcVars)
>+            return;
>+
>+        for (unsigned nodeIndex = 0; nodeIndex < block->size(); ++nodeIndex) {
>+            Node *node = block->at(nodeIndex);
>+            if (!node->shouldGenerate())
>+                continue;
>+
>+            switch (node->op()) {
>+            case SetArgument:
>+            case SetLocal:
>+            case GetLocal: 
>+                {
>+                    unsigned varIndex = variables.indexForOperand(node->local());
>+                    if (varsToRecalc.quickGet(varIndex))
>+                        variables[varIndex] = node;
>+                    break;
>+                }
>+            default:
>+                break;
>+            }
>+
>+        }
>     }
>     
>     Vector<Node*, 128> m_worklist;

Actions: View | Formatted Diff | Diff

Attachments on bug 130115: 237337 | 237338