lowXYZ in FTLLower should always filter the type of the incoming edge
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 03:29:57 +0000 (03:29 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 03:29:57 +0000 (03:29 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189939
<rdar://problem/44407030>

Reviewed by Michael Saboff.

JSTests:

* stress/ftl-should-always-filter-for-low-type-check-functions.js: Added.
(foo):
(test):

Source/JavaScriptCore:

For example, the FTL may know more about data flow than AI in certain programs,
and it needs to inform AI of these data flow properties to appease the assertion
we have in AI that a node must perform type checks on its child nodes.

For example, consider this program:

```
bb#1
a: Phi // Let's say it has an Int32 result, so it goes into the int32 hash table in FTLLower
Branch(...,  #2, #3)

bb#2
ArrayifyToStructure(Cell:@a) // This modifies @a to have the its previous type union the type of some structure set.
Jump(#3)

bb#3
c: Add(Int32:@something, Int32:@a)
```

When the Add node does lowInt32() for @a, FTL lower used to just grab it
from the int32 hash table without filtering the AbstractValue. However,
the parent node is asking for a type check to happen, so we must inform
AI of this "type check" if we want to appease the assertion that all nodes
perform type checks for their edges that semantically perform type checks.
This patch makes it so we filter the AbstractValue in the lowXYZ even
if FTLLower proved the value must be XYZ.

* ftl/FTLLowerDFGToB3.cpp:
(JSC::FTL::DFG::LowerDFGToB3::compilePhi):
(JSC::FTL::DFG::LowerDFGToB3::simulatedTypeCheck):
(JSC::FTL::DFG::LowerDFGToB3::lowInt32):
(JSC::FTL::DFG::LowerDFGToB3::lowCell):
(JSC::FTL::DFG::LowerDFGToB3::lowBoolean):

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

JSTests/ChangeLog
JSTests/stress/ftl-should-always-filter-for-low-type-check-functions.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

index 08edee9..ec3e15f 100644 (file)
@@ -1,3 +1,15 @@
+2018-10-03  Saam barati  <sbarati@apple.com>
+
+        lowXYZ in FTLLower should always filter the type of the incoming edge
+        https://bugs.webkit.org/show_bug.cgi?id=189939
+        <rdar://problem/44407030>
+
+        Reviewed by Michael Saboff.
+
+        * stress/ftl-should-always-filter-for-low-type-check-functions.js: Added.
+        (foo):
+        (test):
+
 2018-10-03  Mark Lam  <mark.lam@apple.com>
 
         Make string MaxLength for all WTF and JS strings consistently equal to INT_MAX.
diff --git a/JSTests/stress/ftl-should-always-filter-for-low-type-check-functions.js b/JSTests/stress/ftl-should-always-filter-for-low-type-check-functions.js
new file mode 100644 (file)
index 0000000..0a1f4f3
--- /dev/null
@@ -0,0 +1,31 @@
+//@ runDefault("--useConcurrentJIT=0", "--jitPolicyScale=0", "--maximumInliningDepth=2")
+
+function foo(x, y) {
+    var w = 0;
+    for (var i = 0; i < x.length; ++i) {
+        for (var j = 0; j < x.length; ++j)
+            w += foo(j, i);
+        y[i] = w;
+    }
+}
+
+function test(x, a3) {
+      a1 = [];
+      a2 = [];
+
+    for (i = 0; i < x; ++i)
+        a1[i] = 0;
+
+    for (i = 0; i < 10; ++i) {
+        foo(a3, a2);
+        foo(a3, a1);
+    }
+}
+noDFG(test);
+
+a3 = [];
+for (var i = 0; i < 3; ++i)
+    a3[i] = 0;
+
+for (var i = 3; i <= 12; i *= 2)
+    test(i, a3);
index fe39196..08cb7df 100644 (file)
@@ -1,3 +1,45 @@
+2018-10-03  Saam barati  <sbarati@apple.com>
+
+        lowXYZ in FTLLower should always filter the type of the incoming edge
+        https://bugs.webkit.org/show_bug.cgi?id=189939
+        <rdar://problem/44407030>
+
+        Reviewed by Michael Saboff.
+
+        For example, the FTL may know more about data flow than AI in certain programs,
+        and it needs to inform AI of these data flow properties to appease the assertion
+        we have in AI that a node must perform type checks on its child nodes.
+        
+        For example, consider this program:
+        
+        ```
+        bb#1
+        a: Phi // Let's say it has an Int32 result, so it goes into the int32 hash table in FTLLower
+        Branch(...,  #2, #3)
+        
+        bb#2
+        ArrayifyToStructure(Cell:@a) // This modifies @a to have the its previous type union the type of some structure set.
+        Jump(#3)
+        
+        bb#3
+        c: Add(Int32:@something, Int32:@a)
+        ```
+        
+        When the Add node does lowInt32() for @a, FTL lower used to just grab it
+        from the int32 hash table without filtering the AbstractValue. However,
+        the parent node is asking for a type check to happen, so we must inform
+        AI of this "type check" if we want to appease the assertion that all nodes
+        perform type checks for their edges that semantically perform type checks.
+        This patch makes it so we filter the AbstractValue in the lowXYZ even
+        if FTLLower proved the value must be XYZ.
+
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compilePhi):
+        (JSC::FTL::DFG::LowerDFGToB3::simulatedTypeCheck):
+        (JSC::FTL::DFG::LowerDFGToB3::lowInt32):
+        (JSC::FTL::DFG::LowerDFGToB3::lowCell):
+        (JSC::FTL::DFG::LowerDFGToB3::lowBoolean):
+
 2018-10-03  Michael Saboff  <msaboff@apple.com>
 
         Command line jsc should report memory footprint in bytes
index f849ffa..7ffa052 100644 (file)
@@ -1416,7 +1416,7 @@ private:
             setJSValue(phi);
             break;
         default:
-            DFG_CRASH(m_graph, m_node, "Bad use kind");
+            DFG_CRASH(m_graph, m_node, "Bad result type");
             break;
         }
     }
@@ -14627,6 +14627,11 @@ private:
     {
         m_state.setIsValid(false);
     }
+
+    void simulatedTypeCheck(Edge highValue, SpeculatedType typesPassedThrough)
+    {
+        m_interpreter.filter(highValue, typesPassedThrough);
+    }
     
     void typeCheck(
         FormattedValue lowValue, Edge highValue, SpeculatedType typesPassedThrough,
@@ -14652,6 +14657,7 @@ private:
         
         if (edge->hasConstant()) {
             JSValue value = edge->asJSValue();
+            simulatedTypeCheck(edge, SpecInt32Only);
             if (!value.isInt32()) {
                 if (mayHaveTypeCheck(edge.useKind()))
                     terminate(Uncountable);
@@ -14663,8 +14669,10 @@ private:
         }
         
         LoweredNodeValue value = m_int32Values.get(edge.node());
-        if (isValid(value))
+        if (isValid(value)) {
+            simulatedTypeCheck(edge, SpecInt32Only);
             return value.value();
+        }
         
         value = m_strictInt52Values.get(edge.node());
         if (isValid(value))
@@ -14772,6 +14780,7 @@ private:
         
         if (edge->op() == JSConstant) {
             FrozenValue* value = edge->constant();
+            simulatedTypeCheck(edge, SpecCellCheck);
             if (!value->value().isCell()) {
                 if (mayHaveTypeCheck(edge.useKind()))
                     terminate(Uncountable);
@@ -14899,6 +14908,7 @@ private:
         
         if (edge->hasConstant()) {
             JSValue value = edge->asJSValue();
+            simulatedTypeCheck(edge, SpecBoolean);
             if (!value.isBoolean()) {
                 if (mayHaveTypeCheck(edge.useKind()))
                     terminate(Uncountable);
@@ -14910,8 +14920,10 @@ private:
         }
         
         LoweredNodeValue value = m_booleanValues.get(edge.node());
-        if (isValid(value))
+        if (isValid(value)) {
+            simulatedTypeCheck(edge, SpecBoolean);
             return value.value();
+        }
         
         value = m_jsValueValues.get(edge.node());
         if (isValid(value)) {