- Revision
- 240024
- Author
- [email protected]
- Date
- 2019-01-15 18:17:31 -0800 (Tue, 15 Jan 2019)
Log 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/_javascript_Core:
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):
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (240023 => 240024)
--- trunk/JSTests/ChangeLog 2019-01-16 01:41:42 UTC (rev 240023)
+++ trunk/JSTests/ChangeLog 2019-01-16 02:17:31 UTC (rev 240024)
@@ -1,3 +1,21 @@
+2019-01-15 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
Unreviewed, skip JIT tests if it's not enabled
Added: trunk/JSTests/stress/string-get-by-val-lowering.js (0 => 240024)
--- trunk/JSTests/stress/string-get-by-val-lowering.js (rev 0)
+++ trunk/JSTests/stress/string-get-by-val-lowering.js 2019-01-16 02:17:31 UTC (rev 240024)
@@ -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');
+}
Added: trunk/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js (0 => 240024)
--- trunk/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js (rev 0)
+++ trunk/JSTests/stress/type-for-get-by-val-can-be-widen-after-ai.js 2019-01-16 02:17:31 UTC (rev 240024)
@@ -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();
+}
Modified: trunk/Source/_javascript_Core/ChangeLog (240023 => 240024)
--- trunk/Source/_javascript_Core/ChangeLog 2019-01-16 01:41:42 UTC (rev 240023)
+++ trunk/Source/_javascript_Core/ChangeLog 2019-01-16 02:17:31 UTC (rev 240024)
@@ -1,3 +1,25 @@
+2019-01-15 Yusuke Suzuki <[email protected]>
+
+ [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 <[email protected]>
Try ripping out inferred types because it might be a performance improvement
Modified: trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp (240023 => 240024)
--- trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2019-01-16 01:41:42 UTC (rev 240023)
+++ trunk/Source/_javascript_Core/dfg/DFGFixupPhase.cpp 2019-01-16 02:17:31 UTC (rev 240024)
@@ -781,7 +781,7 @@
// 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;
}
@@ -899,6 +899,10 @@
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));
Modified: trunk/Source/_javascript_Core/dfg/DFGSSALoweringPhase.cpp (240023 => 240024)
--- trunk/Source/_javascript_Core/dfg/DFGSSALoweringPhase.cpp 2019-01-16 01:41:42 UTC (rev 240023)
+++ trunk/Source/_javascript_Core/dfg/DFGSSALoweringPhase.cpp 2019-01-16 02:17:31 UTC (rev 240024)
@@ -124,6 +124,10 @@
case Array::SlowPutArrayStorage:
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;
}
@@ -130,7 +134,7 @@
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));
Modified: trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp (240023 => 240024)
--- trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-01-16 01:41:42 UTC (rev 240023)
+++ trunk/Source/_javascript_Core/ftl/FTLLowerDFGToB3.cpp 2019-01-16 02:17:31 UTC (rev 240024)
@@ -6590,7 +6590,7 @@
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 @@
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());
