Attachment 343886 Details for Bug 186814 – WIP

[patch] WIP

b-backup.diff (text/plain), 6.06 KB, created by Saam Barati on 2018-06-28 19:15:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2018-06-28 19:15:07 PDT

Size: 6.06 KB

patch

obsolete

>Index: JSTests/stress/multi-get-by-offset-cse-must-consider-resulting-type.js
>===================================================================
>--- JSTests/stress/multi-get-by-offset-cse-must-consider-resulting-type.js	(nonexistent)
>+++ JSTests/stress/multi-get-by-offset-cse-must-consider-resulting-type.js	(working copy)
>@@ -0,0 +1,29 @@
>+function bar(testArgs) {
>+  (() => {
>+    try {
>+      testArgs.func.call(1);
>+    } catch (e) {
>+      if (!testArgs.qux) {
>+        return e == testArgs.qux;
>+      }
>+    }
>+  })();
>+}
>+for (var i = 0; i < 100000; i++) {
>+  [
>+    {
>+      func: ()=>{},
>+    },
>+    {
>+      func: Int8Array.prototype.values,
>+      foo: 0
>+    },
>+    {
>+      func: Int8Array.prototype.values,
>+      qux: 2
>+    },
>+    {
>+      func: Int8Array.prototype.values,
>+    },
>+  ].forEach(bar);
>+}
>Index: Source/JavaScriptCore/dfg/DFGCSEPhase.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGCSEPhase.cpp	(revision 233343)
>+++ Source/JavaScriptCore/dfg/DFGCSEPhase.cpp	(working copy)
>@@ -279,6 +279,99 @@ private:
> #endif
> };
> 
>+static inline bool shouldBlockReplacement(Graph& graph, const LazyNode& lazyNode, const LazyNode& lazyReplacement)
>+{
>+    if (!lazyNode.isNode() || !lazyReplacement.isNode())
>+        return false;
>+
>+    // OOPS: GetByOffset is probs broken!
>+
>+    Node* node = lazyNode.asNode();
>+    Node* replacement = lazyReplacement.asNode();
>+    if (node->op() != MultiGetByOffset && node->op() != GetByOffset && replacement->op() != GetByOffset && replacement->op() != MultiGetByOffset)
>+        return false;
>+    if (node->op() != replacement->op()) // Can't CSE between GetByOffset and MultiGetByOffset.
>+        return true;
>+    if (node->op() != MultiGetByOffset) {
>+        // OOPS: figure out GetByOffset.
>+        return false;
>+    }
>+
>+    dataLogLn("-----------------------------");
>+    dataLogLn("Node: ", node->index(), " replacement: ", replacement->index());
>+
>+    RELEASE_ASSERT(node->op() == GetByOffset || node->op() == MultiGetByOffset);
>+    RELEASE_ASSERT(replacement->op() == GetByOffset || replacement->op() == MultiGetByOffset);
>+
>+    UniquedStringImpl* uid = graph.identifiers()[node->multiGetByOffsetData().identifierNumber];
>+    RELEASE_ASSERT(uid == graph.identifiers()[replacement->multiGetByOffsetData().identifierNumber]);
>+
>+    auto valueFor = [&] (Node* node) -> std::pair<RegisteredStructureSet, AbstractValue> {
>+        AbstractValue resultValue;
>+        RegisteredStructureSet baseSet; // The final result here is the structures the incoming base can be.
>+
>+        // OOPS: we need to track of the structure set that GetByOffset uses to check the incoming base value.
>+        //if (node->op() == GetByOffset) {
>+
>+        //}
>+
>+        for (const MultiGetByOffsetCase& getCase : node->multiGetByOffsetData().cases) {
>+            RegisteredStructureSet set = getCase.set();
>+            baseSet.merge(set);
>+
>+            switch (getCase.method().kind()) {
>+            case GetByOffsetMethod::Constant: {
>+                AbstractValue thisResult;
>+                thisResult.set(
>+                    graph,
>+                    *getCase.method().constant(),
>+                    StructuresAreWatched);
>+                resultValue.merge(thisResult);
>+                break;
>+            }
>+
>+
>+            case GetByOffsetMethod::Load: {
>+                resultValue.merge(
>+                    graph.inferredValueForProperty(set, uid, StructuresAreWatched));
>+                break;
>+            }
>+
>+            default: {
>+                resultValue.makeHeapTop();
>+                break;
>+            } }
>+        }
>+
>+        return { baseSet, resultValue };
>+    };
>+
>+    auto forNode = valueFor(node);
>+    auto forReplacement = valueFor(replacement);
>+
>+    // Replacement must be a subtype of node. We may have performed optimizations based on the type of
>+    // node already, so we can't replace it with something broader.
>+    if (!forReplacement.first.isSubsetOf(forNode.first)) {
>+        dataLogLn("Blocked replacement b/c base set of replacement is not subset of node");
>+        return true;
>+    }
>+    if (forReplacement.second.value() != forNode.second.value()) {
>+        dataLogLn("Blocked replacement b/c values are not equal");
>+        return true;
>+    }
>+    if (!forReplacement.second.isType(forNode.second.m_type)) {
>+        dataLogLn("Blocked replacement b/c type for replacement is not subtype of node");
>+        return true;
>+    }
>+    if (forNode.second.m_structure.isFinite()) {
>+        if (!forReplacement.second.m_structure.isSubsetOf(forNode.second.m_structure)) {
>+            dataLogLn("Blocked replacement b/c replacement result is not structure subset of node");
>+            return true;
>+        }
>+    }
>+    return false;
>+}
>+
> class LocalCSEPhase : public Phase {
> public:
>     LocalCSEPhase(Graph& graph)
>@@ -540,6 +633,9 @@ private:
>             LazyNode match = m_maps.addImpure(location, value);
>             if (!match)
>                 return;
>+
>+            if (shouldBlockReplacement(m_graph, value, match))
>+                return;
>         
>             if (m_node->op() == GetLocal) {
>                 // Usually the CPS rethreading phase does this. But it's OK for us to mess with
>@@ -835,7 +931,7 @@ public:
>             dataLog("    Got heap location def: ", location, " -> ", value, "\n");
>         
>         LazyNode match = findReplacement(location);
>-        
>+
>         if (DFGCSEPhaseInternal::verbose)
>             dataLog("      Got match: ", match, "\n");
>         
>@@ -847,6 +943,9 @@ public:
>             return;
>         }
> 
>+        if (shouldBlockReplacement(m_graph, value, match))
>+            return;
>+        
>         if (value.isNode() && value.asNode() == m_node) {
>             if (!match.isNode()) {
>                 // We need to properly record the constant in order to use an existing one if applicable.
>@@ -905,11 +1004,13 @@ public:
> 
> bool performLocalCSE(Graph& graph)
> {
>+    graph.dump();
>     return runPhase<LocalCSEPhase>(graph);
> }
> 
> bool performGlobalCSE(Graph& graph)
> {
>+    graph.dump();
>     return runPhase<GlobalCSEPhase>(graph);
> }
>

Actions: View | Formatted Diff | Diff

Attachments on bug 186814: 343886 | 344388 | 344744