Object allocation not sinking properly through CheckStructure
authorbasile_clement@apple.com <basile_clement@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 18:37:58 +0000 (18:37 +0000)
committerbasile_clement@apple.com <basile_clement@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 4 May 2015 18:37:58 +0000 (18:37 +0000)
https://bugs.webkit.org/show_bug.cgi?id=144465

Reviewed by Filip Pizlo.

Currently, sinking an allocation through a CheckStructure will
completely ignore all structure checking, which is obviously wrong.

A CheckStructureImmediate node type was present for that purpose, but
the CheckStructures were not properly replaced.  This ensures that
CheckStructure nodes are replaced by CheckStructureImmediate nodes when
sunk through, and that structure checking happens correctly.

* dfg/DFGNode.h:
(JSC::DFG::Node::convertToCheckStructureImmediate): Added.
(JSC::DFG::Node::hasStructureSet):
* dfg/DFGObjectAllocationSinkingPhase.cpp:
(JSC::DFG::ObjectAllocationSinkingPhase::promoteSunkenFields):
* ftl/FTLLowerDFGToLLVM.cpp:
(JSC::FTL::LowerDFGToLLVM::compileCheckStructure):
(JSC::FTL::LowerDFGToLLVM::compileCheckStructureImmediate):
(JSC::FTL::LowerDFGToLLVM::checkStructure):
* tests/stress/sink_checkstructure.js: Added.
(foo):

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGNode.h
Source/JavaScriptCore/dfg/DFGObjectAllocationSinkingPhase.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToLLVM.cpp
Source/JavaScriptCore/tests/stress/sink_checkstructure.js [new file with mode: 0644]

index 1e2c88e..debb3f1 100644 (file)
@@ -1,3 +1,30 @@
+2015-05-04  Basile Clement  <basile_clement@apple.com>
+
+        Object allocation not sinking properly through CheckStructure
+        https://bugs.webkit.org/show_bug.cgi?id=144465
+
+        Reviewed by Filip Pizlo.
+
+        Currently, sinking an allocation through a CheckStructure will
+        completely ignore all structure checking, which is obviously wrong.
+
+        A CheckStructureImmediate node type was present for that purpose, but
+        the CheckStructures were not properly replaced.  This ensures that
+        CheckStructure nodes are replaced by CheckStructureImmediate nodes when
+        sunk through, and that structure checking happens correctly.
+
+        * dfg/DFGNode.h:
+        (JSC::DFG::Node::convertToCheckStructureImmediate): Added.
+        (JSC::DFG::Node::hasStructureSet):
+        * dfg/DFGObjectAllocationSinkingPhase.cpp:
+        (JSC::DFG::ObjectAllocationSinkingPhase::promoteSunkenFields):
+        * ftl/FTLLowerDFGToLLVM.cpp:
+        (JSC::FTL::LowerDFGToLLVM::compileCheckStructure):
+        (JSC::FTL::LowerDFGToLLVM::compileCheckStructureImmediate):
+        (JSC::FTL::LowerDFGToLLVM::checkStructure):
+        * tests/stress/sink_checkstructure.js: Added.
+        (foo):
+
 2015-05-01  Geoffrey Garen  <ggaren@apple.com>
 
         REGRESSION(r183570): jslib-traverse-jquery is 22% slower
index fb445b7..8e67df2 100644 (file)
@@ -414,6 +414,13 @@ struct Node {
         setOpAndDefaultFlags(CheckStructure);
         m_opInfo = bitwise_cast<uintptr_t>(set); 
     }
+
+    void convertToCheckStructureImmediate(Node* structure)
+    {
+        ASSERT(op() == CheckStructure);
+        m_op = CheckStructureImmediate;
+        children.setChild1(Edge(structure, CellUse));
+    }
     
     void replaceWith(Node* other)
     {
@@ -1334,6 +1341,7 @@ struct Node {
     {
         switch (op()) {
         case CheckStructure:
+        case CheckStructureImmediate:
             return true;
         default:
             return false;
index 8f87342..6611236 100644 (file)
@@ -728,8 +728,17 @@ private:
                             m_localMapping.set(location, value.node());
                     },
                     [&] (PromotedHeapLocation location) {
-                        if (m_sinkCandidates.contains(location.base()))
-                            node->replaceWith(resolve(block, location));
+                        if (m_sinkCandidates.contains(location.base())) {
+                            switch (node->op()) {
+                            case CheckStructure:
+                                node->convertToCheckStructureImmediate(resolve(block, location));
+                                break;
+
+                            default:
+                                node->replaceWith(resolve(block, location));
+                                break;
+                            }
+                        }
                     });
             }
             
index 32f3ee0..e3e0e30 100644 (file)
@@ -1882,7 +1882,11 @@ private:
         
         LValue structureID = m_out.load32(cell, m_heaps.JSCell_structureID);
         
-        checkStructure(structureID, jsValueValue(cell), exitKind, m_node->structureSet());
+        checkStructure(
+            structureID, jsValueValue(cell), exitKind, m_node->structureSet(),
+            [this] (Structure* structure) {
+                return weakStructureID(structure);
+            });
     }
     
     void compileCheckCell()
@@ -5089,8 +5093,12 @@ private:
     
     void compileCheckStructureImmediate()
     {
-        LValue structureID = lowCell(m_node->child1());
-        checkStructure(structureID, noValue(), BadCache, m_node->structureSet());
+        LValue structure = lowCell(m_node->child1());
+        checkStructure(
+            structure, noValue(), BadCache, m_node->structureSet(),
+            [this] (Structure* structure) {
+                return weakStructure(structure);
+            });
     }
     
     void compileMaterializeNewObject()
@@ -5430,14 +5438,15 @@ private:
         return getArgumentsStart(m_node->origin.semantic.inlineCallFrame);
     }
     
+    template<typename Functor>
     void checkStructure(
-        LValue structureID, const FormattedValue& formattedValue, ExitKind exitKind,
-        const StructureSet& set)
+        LValue structureDiscriminant, const FormattedValue& formattedValue, ExitKind exitKind,
+        const StructureSet& set, const Functor& weakStructureDiscriminant)
     {
         if (set.size() == 1) {
             speculate(
                 exitKind, formattedValue, 0,
-                m_out.notEqual(structureID, weakStructureID(set[0])));
+                m_out.notEqual(structureDiscriminant, weakStructureDiscriminant(set[0])));
             return;
         }
         
@@ -5447,14 +5456,14 @@ private:
         for (unsigned i = 0; i < set.size() - 1; ++i) {
             LBasicBlock nextStructure = FTL_NEW_BLOCK(m_out, ("checkStructure nextStructure"));
             m_out.branch(
-                m_out.equal(structureID, weakStructureID(set[i])),
+                m_out.equal(structureDiscriminant, weakStructureDiscriminant(set[i])),
                 unsure(continuation), unsure(nextStructure));
             m_out.appendTo(nextStructure);
         }
         
         speculate(
             exitKind, formattedValue, 0,
-            m_out.notEqual(structureID, weakStructureID(set.last())));
+            m_out.notEqual(structureDiscriminant, weakStructureDiscriminant(set.last())));
         
         m_out.jump(continuation);
         m_out.appendTo(continuation, lastNext);
diff --git a/Source/JavaScriptCore/tests/stress/sink_checkstructure.js b/Source/JavaScriptCore/tests/stress/sink_checkstructure.js
new file mode 100644 (file)
index 0000000..2427a63
--- /dev/null
@@ -0,0 +1,17 @@
+function foo(p, q) {
+    var o = {};
+    if (p) o.f = 42;
+    if (q) { o.f++; return o; }
+}
+noInline(foo);
+
+var expected = foo(false, true).f;
+
+for (var i = 0; i < 1000000; i++) {
+    foo(true, true);
+}
+
+var result = foo(false, true).f;
+
+if (!Object.is(result, expected))
+    throw "Error: expected " + expected + "; FTL produced " + result;