DFG CSE should know how to decay a MultiGetByOffset
authorfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2018 18:54:47 +0000 (18:54 +0000)
committerfpizlo@apple.com <fpizlo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 24 Apr 2018 18:54:47 +0000 (18:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=159859

Reviewed by Keith Miller.

This teaches Node::remove() how to decay a MultiGetByOffset to a CheckStructure, so that
clobberize() can report a def() for MultiGetByOffset.

This is a slight improvement to codegen in splay because splay is a heavy user of
MultiGetByOffset. It uses it redundantly in one of its hot functions (the function called
"splay_"). I don't see a net speed-up in the benchmark. However, this is just a first step to
removing MultiXByOffset-related redundancies, which by my estimates account for 16% of
splay's time.

* dfg/DFGClobberize.h:
(JSC::DFG::clobberize):
* dfg/DFGNode.cpp:
(JSC::DFG::Node::remove):
(JSC::DFG::Node::removeWithoutChecks):
(JSC::DFG::Node::replaceWith):
(JSC::DFG::Node::replaceWithWithoutChecks):
* dfg/DFGNode.h:
(JSC::DFG::Node::convertToMultiGetByOffset):
(JSC::DFG::Node::replaceWith): Deleted.
* dfg/DFGNodeType.h:
* dfg/DFGObjectAllocationSinkingPhase.cpp:

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGClobberize.h
Source/JavaScriptCore/dfg/DFGNode.cpp
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGNodeType.h
Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

index 3ef24ed..2ff7eae 100644 (file)
@@ -1,3 +1,32 @@
+2018-04-24  Filip Pizlo  <fpizlo@apple.com>
+
+        DFG CSE should know how to decay a MultiGetByOffset
+        https://bugs.webkit.org/show_bug.cgi?id=159859
+
+        Reviewed by Keith Miller.
+        
+        This teaches Node::remove() how to decay a MultiGetByOffset to a CheckStructure, so that
+        clobberize() can report a def() for MultiGetByOffset.
+        
+        This is a slight improvement to codegen in splay because splay is a heavy user of
+        MultiGetByOffset. It uses it redundantly in one of its hot functions (the function called
+        "splay_"). I don't see a net speed-up in the benchmark. However, this is just a first step to
+        removing MultiXByOffset-related redundancies, which by my estimates account for 16% of
+        splay's time.
+
+        * dfg/DFGClobberize.h:
+        (JSC::DFG::clobberize):
+        * dfg/DFGNode.cpp:
+        (JSC::DFG::Node::remove):
+        (JSC::DFG::Node::removeWithoutChecks):
+        (JSC::DFG::Node::replaceWith):
+        (JSC::DFG::Node::replaceWithWithoutChecks):
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::convertToMultiGetByOffset):
+        (JSC::DFG::Node::replaceWith): Deleted.
+        * dfg/DFGNodeType.h:
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+
 2018-04-24  Keith Miller  <keith_miller@apple.com>
 
         Update API docs with information on which run loop the VM will use
index 27e1435..24fd941 100644 (file)
@@ -1188,9 +1188,7 @@ void clobberize(Graph& graph, Node* node, const ReadFunctor& read, const WriteFu
         read(JSObject_butterflyMask);
         AbstractHeap heap(NamedProperties, node->multiGetByOffsetData().identifierNumber);
         read(heap);
-        // FIXME: We cannot def() for MultiGetByOffset because CSE is not smart enough to decay it
-        // to a CheckStructure.
-        // https://bugs.webkit.org/show_bug.cgi?id=159859
+        def(HeapLocation(NamedPropertyLoc, heap, node->child1()), LazyNode(node));
         return;
     }
         
index 3a9180d..c1db0cb 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013, 2014, 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -83,27 +83,62 @@ bool Node::hasVariableAccessData(Graph& graph)
 
 void Node::remove(Graph& graph)
 {
-    if (flags() & NodeHasVarArgs) {
-        unsigned targetIndex = 0;
-        for (unsigned i = 0; i < numChildren(); ++i) {
-            Edge& edge = graph.varArgChild(this, i);
-            if (!edge)
-                continue;
-            if (edge.willHaveCheck()) {
-                Edge& dst = graph.varArgChild(this, targetIndex++);
-                std::swap(dst, edge);
-                continue;
+    switch (op()) {
+    case MultiGetByOffset: {
+        MultiGetByOffsetData& data = multiGetByOffsetData();
+        StructureSet set;
+        for (MultiGetByOffsetCase& getCase : data.cases) {
+            getCase.set().forEach(
+                [&] (RegisteredStructure structure) {
+                    set.add(structure.get());
+                });
+        }
+        convertToCheckStructure(graph.addStructureSet(set));
+        return;
+    }
+        
+    default:
+        if (flags() & NodeHasVarArgs) {
+            unsigned targetIndex = 0;
+            for (unsigned i = 0; i < numChildren(); ++i) {
+                Edge& edge = graph.varArgChild(this, i);
+                if (!edge)
+                    continue;
+                if (edge.willHaveCheck()) {
+                    Edge& dst = graph.varArgChild(this, targetIndex++);
+                    std::swap(dst, edge);
+                    continue;
+                }
+                edge = Edge();
             }
-            edge = Edge();
+            setOpAndDefaultFlags(CheckVarargs);
+            children.setNumChildren(targetIndex);
+        } else {
+            children = children.justChecks();
+            setOpAndDefaultFlags(Check);
         }
-        setOpAndDefaultFlags(CheckVarargs);
-        children.setNumChildren(targetIndex);
-    } else {
-        children = children.justChecks();
-        setOpAndDefaultFlags(Check);
+        return;
     }
 }
 
+void Node::removeWithoutChecks()
+{
+    children = AdjacencyList();
+    setOpAndDefaultFlags(Check);
+}
+
+void Node::replaceWith(Graph& graph, Node* other)
+{
+    remove(graph);
+    setReplacement(other);
+}
+
+void Node::replaceWithWithoutChecks(Node* other)
+{
+    removeWithoutChecks();
+    setReplacement(other);
+}
+
 void Node::convertToIdentity()
 {
     RELEASE_ASSERT(child1());
index d201315..4d97cd3 100644 (file)
@@ -431,6 +431,7 @@ public:
     }
 
     void remove(Graph&);
+    void removeWithoutChecks();
 
     void convertToCheckStructure(RegisteredStructureSet* set)
     {
@@ -451,11 +452,8 @@ public:
         children.setChild1(Edge(structure, CellUse));
     }
     
-    void replaceWith(Graph& graph, Node* other)
-    {
-        remove(graph);
-        setReplacement(other);
-    }
+    void replaceWith(Graph&, Node* other);
+    void replaceWithWithoutChecks(Node* other);
 
     void convertToIdentity();
     void convertToIdentityOn(Node*);
@@ -562,11 +560,11 @@ public:
     
     void convertToMultiGetByOffset(MultiGetByOffsetData* data)
     {
-        ASSERT(m_op == GetById || m_op == GetByIdFlush || m_op == GetByIdDirect || m_op == GetByIdDirectFlush);
+        RELEASE_ASSERT(m_op == GetById || m_op == GetByIdFlush || m_op == GetByIdDirect || m_op == GetByIdDirectFlush);
         m_opInfo = data;
         child1().setUseKind(CellUse);
         m_op = MultiGetByOffset;
-        ASSERT(m_flags & NodeMustGenerate);
+        RELEASE_ASSERT(m_flags & NodeMustGenerate);
     }
     
     void convertToPutByOffset(StorageAccessData& data, Edge storage, Edge base)
index 84ed382..d630b1d 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012-2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2012-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
index 8e291f2..1ceb201 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2018 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -960,7 +960,7 @@ private:
                 PromotedHeapLocation location(NamedPropertyPLoc, allocation->identifier(), identifierNumber);
                 if (Node* value = heapResolve(location)) {
                     if (allocation->structures().isSubsetOf(validStructures))
-                        node->replaceWith(m_graph, value);
+                        node->replaceWithWithoutChecks(value);
                     else {
                         Node* structure = heapResolve(PromotedHeapLocation(allocation->identifier(), StructurePLoc));
                         ASSERT(structure);