Attachment 359227 Details for Bug 193438 – Patch

[patch] Patch

bug-193438-20190115165415.patch (text/plain), 7.64 KB, created by Yusuke Suzuki on 2019-01-15 16:54:17 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-01-15 16:54:17 PST

Size: 7.64 KB

patch

obsolete

>Subversion Revision: 240000
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 3e57f55dc915ebb4fa52941378ea919206d816de..05d8c7588ad6a4449072424b93f2d5edc52e71ee 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,25 @@
>+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 NOBODY (OOPS!).
>+
>+        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 tyep 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  Tomas Popela  <tpopela@redhat.com>
> 
>         Unreviewed: Fix the -Wformat compiler warnings
>diff --git a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>index 05112da4d6f5161403173eccd23dfc2f7091079b..e779deb4d48c11e1947e5c40dc69e1d0eec1c584 100644
>--- a/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGFixupPhase.cpp
>@@ -782,7 +782,7 @@ class FixupPhase : public Phase {
>             // Currently we have no good way of refining these.
>             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;
>         }
>@@ -900,6 +900,10 @@ class FixupPhase : public Phase {
>                 break;
>             case Array::ForceExit:
>                 break;
>+            case Array::String:
>+                fixEdge<KnownStringUse>(m_graph.varArgChild(node, 0));
>+                fixEdge<Int32Use>(m_graph.varArgChild(node, 1));
>+                break;
>             default:
>                 fixEdge<KnownCellUse>(m_graph.varArgChild(node, 0));
>                 fixEdge<Int32Use>(m_graph.varArgChild(node, 1));
>diff --git a/Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp b/Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp
>index e7261845f2c2d38ec11af0e2a7bee3a6f23a97fc..ad38994086223e5911a3f50ce47c609129ed6ceb 100644
>--- a/Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGSSALoweringPhase.cpp
>@@ -124,13 +124,17 @@ class SSALoweringPhase : public Phase {
>         case Array::SlowPutArrayStorage:
>             op = GetVectorLength;
>             break;
>+        case Array::String:
>+            // When we need supporting this, it requires an additional code since base's useKind is KnownStringUse.
>+            RELEASE_ASSERT_NOT_REACHED();
>+            break;
>         default:
>             break;
>         }
> 
>         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,
>             index, Edge(length, KnownInt32Use));
>diff --git a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>index 73836a8aad901e4324a7eea8e40e1ad9368d5137..c6348ba0479b32c76163b6818aed8f2b4d64157b 100644
>--- a/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>+++ b/Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>@@ -6590,7 +6590,7 @@ class LowerDFGToB3 {
>     
>     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));
>             
>@@ -6704,7 +6704,7 @@ class LowerDFGToB3 {
>         LBasicBlock is16Bit = m_out.newBlock();
>         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());
>         
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 54c47fac29ba7b7ae6aee5dd2bee0be449b2e143..cec69157185c72248ecb37c9d0385a94caff3e78 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,21 @@
>+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 NOBODY (OOPS!).
>+
>+        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>
> 
>         Unreviewed, skip JIT tests if it's not enabled
>diff --git a/JSTests/stress/string-get-by-val-lowering.js b/JSTests/stress/string-get-by-val-lowering.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..86933eb741a0932a7d3d1c4d8453ecd8ed21b727
>--- /dev/null
>+++ b/JSTests/stress/string-get-by-val-lowering.js
>@@ -0,0 +1,17 @@
>+function shouldBe(actual, expected) {
>+    if (actual !== expected)
>+        throw new Error('bad value: ' + actual);
>+}
>+noInline(shouldBe);
>+
>+function test(value)
>+{
>+    return value[2];
>+}
>+noInline(test);
>+
>+for (var i = 0; i < 1e4; ++i) {
>+    shouldBe(test("Hello"), 'l');
>+    shouldBe(test("World"), 'r');
>+    shouldBe(test("Nice"), 'c');
>+}
>diff --git a/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js b/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..94818b7f9d7b8c560226e82c0e82bcfac7f53b37
>--- /dev/null
>+++ b/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js
>@@ -0,0 +1,25 @@
>+//@ runDefault("--jitPolicyScale=0")
>+// Run with for i in {1..1000}; do echo $i && VM=/path/to/WebKit/WebKitBuild/Debug/ && DYLD_FRAMEWORK_PATH=$VM $VM/jsc --useDollarVM=1 --jitPolicyScale=0 type-for-get-by-val-can-be-widen-after-ai.js ; done
>+
>+function Hello(y) {
>+  this.y = y;
>+  this.x = foo(this.y);
>+}
>+function foo(z) {
>+  try {
>+    for (var i = 0; i < 1; i++) {
>+      z[i];
>+    }
>+  } catch {
>+  }
>+}
>+new Hello('a');
>+new Hello('a');
>+for (let i = 0; i < 100; ++i) {
>+  new Hello();
>+}
>+
>+// Busy loop to let the crash reporter have a chance to capture the crash log for the Compiler thread.
>+for (let i = 0; i < 1000000; ++i) {
>+    $vm.ftlTrue();
>+}

Flags:

keith_miller: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193438: 359220 | 359226 | 359227