Attachment 369898 Details for Bug 197855 – patch

[patch] patch

a-backup.diff (text/plain), 4.39 KB, created by Saam Barati on 2019-05-14 15:18:47 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Saam Barati

Created: 2019-05-14 15:18:47 PDT

Size: 4.39 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 245312)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-05-14  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 NOBODY (OOPS!).
>+
>+        * stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js: Added.
>+        (f1):
>+        (f2):
>+        (foo):
>+
> 2019-05-14  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Shrink sizeof(UnlinkedFunctionExecutable) more
>Index: JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js
>===================================================================
>--- JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js	(nonexistent)
>+++ JSTests/stress/set-argument-maybe-maximal-flush-should-not-extend-liveness.js	(working copy)
>@@ -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: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 245245)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,23 @@
>+2019-05-14  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 NOBODY (OOPS!).
>+
>+        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-13  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Compress miscelaneous JIT related data structures with Packed<>
>Index: Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(revision 245243)
>+++ Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp	(working copy)
>@@ -1863,6 +1863,8 @@ bool ByteCodeParser::handleVarargsInlini
>     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::handleVarargsInlini
>             }
>             
>             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::handleVarargsInlini
>     // 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;
> }

Flags:

msaboff: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 197855: 369794 | 369802 | 369823 | 369898