Changeset 230964 in webkit

Timestamp:

Apr 24, 2018 11:54:47 AM (6 years ago)

Author:

fpizlo@apple.com

Message:

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:

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• dfg/DFGClobberize.h (modified) (1 diff)
• dfg/DFGNode.cpp (modified) (2 diffs)
• dfg/DFGNode.h (modified) (3 diffs)
• dfg/DFGNodeType.h (modified) (1 diff)
• dfg/DFGObjectAllocationSinkingPhase.cpp (modified) (2 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/JavaScriptCore/dfg/DFGClobberize.h

@@ -1189 +1189 @@
         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;
     }

trunk/Source/JavaScriptCore/dfg/DFGNode.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -84 +84 @@
 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);
 }
 

trunk/Source/JavaScriptCore/dfg/DFGNode.h

@@ -432 +432 @@
 
     void remove(Graph&);
+    void removeWithoutChecks();
 
     void convertToCheckStructure(RegisteredStructureSet* set)
@@ -452 +453 @@
     }
     
-    void replaceWith(Graph& graph, Node* other)
-    {
-        remove(graph);
-        setReplacement(other);
-    }
+    void replaceWith(Graph&, Node* other);
+    void replaceWithWithoutChecks(Node* other);
 
     void convertToIdentity();
@@ -563 +561 @@
     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);
     }
     

trunk/Source/JavaScriptCore/dfg/DFGNodeType.h

@@ -1 +1 @@
 /*
- * 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

trunk/Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp

@@ -1 +1 @@
 /*
- * 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
@@ -961 +961 @@
                 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));