Bound liveness of SetArgumentMaybe nodes when maximal flush insertion phase is enabled
https://bugs.webkit.org/show_bug.cgi?id=197855
<rdar://problem/50236506>
Reviewed by Michael Saboff.
JSTests:
* stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js: Added.
(f0):
(bar):
(foo):
* stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js: Added.
(f1):
(f2):
(foo):
Source/JavaScriptCore:
Maximal flush insertion phase assumes it can extend the live range of
variables. However, this is not true with SetArgumentMaybe nodes, because
they are not guaranteed to demarcate the birth of a variable in the way
that SetArgumentDefinitely does. This caused things to break in SSA conversion
when we wanted to use the result of a SetArgumentMaybe node. To obviate this,
when we're done inlining something with SetArgumentMaybes, we SetLocal(undefined)
to the same set of locals. This caps the live range of the SetArgumentMaybe
and makes it so that extending the live range of the SetLocal is valid.
* dfg/DFGByteCodeParser.cpp:
(JSC::DFG::ByteCodeParser::handleVarargsInlining):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@245341 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
index 5b19a0e..33d8b0a 100644
--- a/JSTests/ChangeLog
+++ b/JSTests/ChangeLog
@@ -1,3 +1,20 @@
+2019-05-15 Saam Barati <sbarati@apple.com>
+
+ Bound liveness of SetArgumentMaybe nodes when maximal flush insertion phase is enabled
+ https://bugs.webkit.org/show_bug.cgi?id=197855
+ <rdar://problem/50236506>
+
+ Reviewed by Michael Saboff.
+
+ * stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js: Added.
+ (f0):
+ (bar):
+ (foo):
+ * stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js: Added.
+ (f1):
+ (f2):
+ (foo):
+
2019-05-14 Keith Miller <keith_miller@apple.com>
Fix issue with byteOffset on ARM64E
diff --git a/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js b/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js
new file mode 100644
index 0000000..9f8a9b8
--- /dev/null
+++ b/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js
@@ -0,0 +1,20 @@
+//@ runDefault("--useMaximalFlushInsertionPhase=1", "--jitPolicyScale=0", "--useConcurrentJIT=0")
+
+function f0() {
+}
+
+function bar() {
+ f0(...arguments);
+}
+
+const a = new Uint8Array(1);
+
+function foo() {
+ bar(0, 0);
+ a.find(()=>{});
+}
+
+
+for (let i=0; i<3; i++) {
+ foo();
+}
diff --git a/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js b/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js
new file mode 100644
index 0000000..d897b27
--- /dev/null
+++ b/JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js
@@ -0,0 +1,20 @@
+//@ runDefault("--useMaximalFlushInsertionPhase=1", "--validateGraphAtEachPhase=1")
+
+function f1() {
+}
+
+function f2() {
+}
+
+const a = [0];
+
+function foo() {
+ f1(...a);
+ for (let i = 0; i < 2; i++) {
+ f2([] > 0);
+ }
+}
+
+for (var i = 0; i < 1000000; ++i) {
+ foo();
+}
diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
index 3128631..5b4da47 100644
--- a/Source/JavaScriptCore/ChangeLog
+++ b/Source/JavaScriptCore/ChangeLog
@@ -1,3 +1,23 @@
+2019-05-15 Saam Barati <sbarati@apple.com>
+
+ Bound liveness of SetArgumentMaybe nodes when maximal flush insertion phase is enabled
+ https://bugs.webkit.org/show_bug.cgi?id=197855
+ <rdar://problem/50236506>
+
+ Reviewed by Michael Saboff.
+
+ Maximal flush insertion phase assumes it can extend the live range of
+ variables. However, this is not true with SetArgumentMaybe nodes, because
+ they are not guaranteed to demarcate the birth of a variable in the way
+ that SetArgumentDefinitely does. This caused things to break in SSA conversion
+ when we wanted to use the result of a SetArgumentMaybe node. To obviate this,
+ when we're done inlining something with SetArgumentMaybes, we SetLocal(undefined)
+ to the same set of locals. This caps the live range of the SetArgumentMaybe
+ and makes it so that extending the live range of the SetLocal is valid.
+
+ * dfg/DFGByteCodeParser.cpp:
+ (JSC::DFG::ByteCodeParser::handleVarargsInlining):
+
2019-05-14 Keith Miller <keith_miller@apple.com>
Fix issue with byteOffset on ARM64E
diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
index 841da18..6505ad5 100644
--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
@@ -1863,6 +1863,8 @@
registerOffset -= maxNumArguments; // includes "this"
registerOffset -= CallFrame::headerSizeInRegisters;
registerOffset = -WTF::roundUpToMultipleOf(stackAlignmentRegisters(), -registerOffset);
+
+ Vector<VirtualRegister> setArgumentMaybes;
auto insertChecks = [&] (CodeBlock* codeBlock) {
emitFunctionChecks(callVariant, callTargetNode, thisArgument);
@@ -1928,6 +1930,8 @@
}
Node* setArgument = addToGraph(numSetArguments >= mandatoryMinimum ? SetArgumentMaybe : SetArgumentDefinitely, OpInfo(variable));
+ if (numSetArguments >= mandatoryMinimum && Options::useMaximalFlushInsertionPhase())
+ setArgumentMaybes.append(variable->local());
m_currentBlock->variablesAtTail.setOperand(variable->local(), setArgument);
++numSetArguments;
}
@@ -1942,6 +1946,9 @@
// calling LoadVarargs twice.
inlineCall(callTargetNode, result, callVariant, registerOffset, maxNumArguments, kind, nullptr, insertChecks);
+ for (VirtualRegister reg : setArgumentMaybes)
+ setDirect(reg, jsConstant(jsUndefined()), ImmediateNakedSet);
+
VERBOSE_LOG("Successful inlining (varargs, monomorphic).\nStack: ", currentCodeOrigin(), "\n");
return true;
}
