Changeset 240024 in webkit

Timestamp:

Jan 15, 2019, 6:17:31 PM (8 years ago)

Author:

yusukesuzuki@slowstart.org

Message:

[JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
​https://bugs.webkit.org/show_bug.cgi?id=193438
<rdar://problem/45581249>

Reviewed by Saam Barati and Keith Miller.

JSTests:

Under the heavy load (like, compiling WebKit), AI in this code can broaden type information after the 1st run.
Then, GetByVal(String) crashed.

• stress/string-get-by-val-lowering.js: Added.

(shouldBe):
(test):

• stress/type-for-get-by-val-can-be-widen-after-ai.js: Added.

(Hello):
(foo):

Source/JavaScriptCore:

GetByVal(Array::String) emits Check(String) before that. But AI can broaden type constraint in the second run.
After the first run removes Check(String), it would happen that AI starts saying the type of 1st child is not String.
To claim that it *is* a String type, we should use KnownStringUse here.

• dfg/DFGFixupPhase.cpp:

(JSC::DFG::FixupPhase::fixupNode): StringCharAt and GetByVal(Array::String) share the underlying compiler code. We should
change StringUse => KnownStringUse for StringCharAt too. And StringCharAt and StringCharCodeAt potentially have the same
problem. This patch fixes it too.

• dfg/DFGSSALoweringPhase.cpp:

(JSC::DFG::SSALoweringPhase::lowerBoundsCheck):

• ftl/FTLLowerDFGToB3.cpp:

(JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
(JSC::FTL::DFG::LowerDFGToB3::compileStringCharCodeAt):

Location:

trunk

Files:

• JSTests/ChangeLog (modified) (1 diff)
• JSTests/stress/string-get-by-val-lowering.js (added)
• JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js (added)
• Source/JavaScriptCore/ChangeLog (modified) (1 diff)
• Source/JavaScriptCore/dfg/DFGFixupPhase.cpp (modified) (2 diffs)
• Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp (modified) (2 diffs)
• Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp (modified) (2 diffs)

trunk/JSTests/ChangeLog

@@ +1 @@
+2019-01-15  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
+        https://bugs.webkit.org/show_bug.cgi?id=193438
+        <rdar://problem/45581249>
+
+        Reviewed by Saam Barati and Keith Miller.
+
+        Under the heavy load (like, compiling WebKit), AI in this code can broaden type information after the 1st run.
+        Then, GetByVal(String) crashed.
+
+        * stress/string-get-by-val-lowering.js: Added.
+        (shouldBe):
+        (test):
+        * stress/type-for-get-by-val-can-be-widen-after-ai.js: Added.
+        (Hello):
+        (foo):
+
 2019-01-15  Tomas Popela  <tpopela@redhat.com>
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2019-01-15  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
+
+        [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
+        https://bugs.webkit.org/show_bug.cgi?id=193438
+        <rdar://problem/45581249>
+
+        Reviewed by Saam Barati and Keith Miller.
+
+        GetByVal(Array::String) emits Check(String) before that. But AI can broaden type constraint in the second run.
+        After the first run removes Check(String), it would happen that AI starts saying the type of 1st child is not String.
+        To claim that it *is* a String type, we should use KnownStringUse here.
+
+        * dfg/DFGFixupPhase.cpp:
+        (JSC::DFG::FixupPhase::fixupNode): StringCharAt and GetByVal(Array::String) share the underlying compiler code. We should
+        change StringUse => KnownStringUse for StringCharAt too. And StringCharAt and StringCharCodeAt potentially have the same
+        problem. This patch fixes it too.
+        * dfg/DFGSSALoweringPhase.cpp:
+        (JSC::DFG::SSALoweringPhase::lowerBoundsCheck):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
+        (JSC::FTL::DFG::LowerDFGToB3::compileStringCharCodeAt):
+
 2019-01-15  Saam Barati  <sbarati@apple.com>
 

trunk/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp

@@ -782 +782 @@
             ASSERT(node->arrayMode() == ArrayMode(Array::String, Array::Read));
             blessArrayOperation(node->child1(), node->child2(), node->child3());
-            fixEdge<KnownCellUse>(node->child1());
+            fixEdge<KnownStringUse>(node->child1());
             fixEdge<Int32Use>(node->child2());
             break;
@@ -899 +899 @@
                 break;
             case Array::ForceExit:
+                break;
+            case Array::String:
+                fixEdge<KnownStringUse>(m_graph.varArgChild(node, 0));
+                fixEdge<Int32Use>(m_graph.varArgChild(node, 1));
                 break;
             default:

trunk/Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp

@@ -125 +125 @@
             op = GetVectorLength;
             break;
+        case Array::String:
+            // When we need to support this, it will require additional code since base's useKind is KnownStringUse.
+            DFG_CRASH(m_graph, m_node, "Array::String's base.useKind() is KnownStringUse");
+            break;
         default:
             break;
@@ -131 +135 @@
         Node* length = m_insertionSet.insertNode(
             m_nodeIndex, SpecInt32Only, op, m_node->origin,
-            OpInfo(m_node->arrayMode().asWord()), base, storage);
+            OpInfo(m_node->arrayMode().asWord()), Edge(base.node(), KnownCellUse), storage);
         m_insertionSet.insertNode(
             m_nodeIndex, SpecInt32Only, CheckInBounds, m_node->origin,

trunk/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -6591 +6591 @@
     void compileStringCharAt()
     {
-        LValue base = lowCell(m_graph.child(m_node, 0));
+        LValue base = lowString(m_graph.child(m_node, 0));
         LValue index = lowInt32(m_graph.child(m_node, 1));
         LValue storage = lowStorage(m_graph.child(m_node, 2));
@@ -6705 +6705 @@
         LBasicBlock continuation = m_out.newBlock();
 
-        LValue base = lowCell(m_node->child1());
+        LValue base = lowString(m_node->child1());
         LValue index = lowInt32(m_node->child2());
         LValue storage = lowStorage(m_node->child3());