Bound liveness of SetArgumentMaybe nodes when maximal flush insertion phase is enabled
authorsbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 May 2019 20:30:16 +0000 (20:30 +0000)
committersbarati@apple.com <sbarati@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 15 May 2019 20:30:16 +0000 (20:30 +0000)
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: https://svn.webkit.org/repository/webkit/trunk@245341 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness-2.js [new file with mode: 0644]
JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp

index 5b19a0e..33d8b0a 100644 (file)
@@ -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 (file)
index 0000000..9f8a9b8
--- /dev/null
@@ -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 (file)
index 0000000..d897b27
--- /dev/null
@@ -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();
+}
index 3128631..5b4da47 100644 (file)
@@ -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
index 841da18..6505ad5 100644 (file)
@@ -1863,6 +1863,8 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
     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 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
             }
             
             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 @@ bool ByteCodeParser::handleVarargsInlining(Node* callTargetNode, VirtualRegister
     // 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;
 }