Attachment 368594 Details for Bug 197405 – Patch

[patch] Patch

bug-197405-20190430130829.patch (text/plain), 6.01 KB, created by Yusuke Suzuki on 2019-04-30 13:08:30 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-04-30 13:08:30 PDT

Size: 6.01 KB

patch

obsolete

>Subversion Revision: 244763
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5778ff4fbe3694bc0bc14140680e34a04ed56f5b..79d73b20421946c138f352619d1c107af94b1ed9 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-04-30  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
>+        https://bugs.webkit.org/show_bug.cgi?id=197405
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::inlineCall):
>+
> 2019-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         Unreivewed, fix FTL implementation of r244760
>diff --git a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>index be99551e91d58697dcf2754b26982ca48bc775f2..c03bac09bb75068b380f1b901bfef33ecd20c682 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -1608,6 +1608,48 @@ void ByteCodeParser::inlineCall(Node* callTargetNode, VirtualRegister result, Ca
>         calleeVariable->mergeShouldNeverUnbox(true);
>     }
> 
>+    switch (kind) {
>+    case InlineCallFrame::GetterCall:
>+    case InlineCallFrame::SetterCall: {
>+        // When inlining getter and setter calls, we setup the stack frame which does not appear in the bytecode.
>+        // But inlining call can have switch on executable. So we could have a graph like this.
>+        //
>+        // BB#0
>+        //     ...
>+        //     19:<!0:-> MovHint(Check:Untyped:@18, MustGen, loc4, W:SideState, ClobbersExit, bc#1, ExitValid)
>+        //     20:< 1:-> SetLocal(Check:Untyped:@18, loc4(J~/FlushedJSValue), W:Stack(-5), bc#1, exit: bc#3, ExitValid)  predicting None
>+        //     21:<!0:-> MovHint(Check:Untyped:@18, MustGen, loc5, W:SideState, ClobbersExit, bc#3, ExitValid)
>+        //     22:< 1:-> SetLocal(Check:Untyped:@18, loc5(K~/FlushedJSValue), W:Stack(-6), bc#3, exit: bc#6, ExitValid)  predicting None
>+        //     ...
>+        //     30:< 1:-> GetSetter(Check:Untyped:@29, JS|PureInt, R:GetterSetter_setter, Exits, bc#7, ExitValid)
>+        //     ...
>+        //     37:< 1:-> GetExecutable(Check:Untyped:@30, JS|PureInt, Exits, bc#7, ExitValid)
>+        //     ...
>+        //     41:<!0:-> Switch(Check:Untyped:@37, MustGen, SwitchCell, Weak:Cell: 0x1057c3680 (%Ei:FunctionExecutable), StructureID: 18859:#2, default:#3, W:SideState, Exits, bc#7, ExitValid)
>+        //
>+        // BB#2
>+        //     42:<!0:-> GetLocal(JS|MustGen|PureInt, loc12(O~/FlushedJSValue), R:Stack(-13), bc#7, ExitValid)  predicting None
>+        //     43:<!0:-> MovHint(Check:Untyped:@42, MustGen, loc12, W:SideState, ClobbersExit, bc#7, ExitValid)
>+        //     44:< 1:-> SetLocal(Check:Untyped:@42, loc12(P!/FlushedJSValue), W:Stack(-13), bc#7, ExitInvalid)  predicting None
>+        //     --> f#DMIaZz:<0x1057a0390, bc#7, SetterCall, closure call, numArgs+this = 2, numFixup = 0, stackOffset = -16 (loc0 maps to loc16)>
>+        //       45:<!0:->   ExitOK(MustGen, R:Stack(-13), W:SideState, bc#0, ExitValid)
>+        //       ...
>+        //       <HERE>
>+        //
>+        // BB#2's first node is @42, which origin is bc#7 of the caller (not callee f#DMIaZz). So OSR availability phase can prune @19 and @21 liveness, because these stack values are set up
>+        // adhocly by DFG and it won't be shown in the bytecode. If <HERE> causes OSR exit, we fail to find loc4 and loc5. Here, we emit MovHint in BB#2 to tell f#DMIaZz in BB#2 the frame layout,
>+        // as if we are doing similarly for arity fixup. Since these locals are already flushed to the appropriate place, we do not need to emit SetLocal later.
>+        for (int index = 0; index < argumentCountIncludingThis; ++index) {
>+            Node* value = get(virtualRegisterForArgument(index, registerOffset));
>+            VirtualRegister argumentToSet = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));
>+            addToGraph(MovHint, OpInfo(argumentToSet.offset()), value);
>+        }
>+        break;
>+    }
>+    default:
>+        break;
>+    }
>+
>     if (arityFixupCount) {
>         // Note: we do arity fixup in two phases:
>         // 1. We get all the values we need and MovHint them to the expected locals.
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index aafd97331929c0a502e20e66cc2677cfceda3ed0..874d903c9bc49d9ac7c5533fc13a8e2c6a65a45c 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-04-30  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
>+        https://bugs.webkit.org/show_bug.cgi?id=197405
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/getter-setter-inlining-should-emit-movhint.js: Added.
>+        (foo):
>+        (test):
>+        (i.o.get f):
>+        (i.o.set f):
>+
> 2019-04-29  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         normalizeMapKey should normalize NaN to one PureNaN bit pattern to make MapHash same
>diff --git a/JSTests/stress/getter-setter-inlining-should-emit-movhint.js b/JSTests/stress/getter-setter-inlining-should-emit-movhint.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..616fdf06d7c48834be19606f002e911faa870ad2
>--- /dev/null
>+++ b/JSTests/stress/getter-setter-inlining-should-emit-movhint.js
>@@ -0,0 +1,30 @@
>+//@ runDefault("--useRandomizingFuzzerAgent=1", "--usePolymorphicCallInliningForNonStubStatus=1", "--seedOfRandomizingFuzzerAgent=2896922505", "--useLLInt=0")
>+function foo(o) {
>+    o.f = 0;
>+    return o.f;
>+}
>+noInline(foo);
>+
>+let counter = 0;
>+
>+function test(o, value) {
>+    var result = foo(o);
>+    if (result < value)
>+        throw new Error(result);
>+    if (counter < value)
>+        throw new Error(counter);
>+    Array.of(arguments);
>+}
>+
>+for (var i = 0; i < 100000; ++i) {
>+    var o = {
>+        get f() {
>+            return o
>+        },
>+        set f(v) {
>+            counter++;
>+            this.z = 0;
>+        }
>+    };
>+    test(o, i, i);
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 197405: 368542 | 368594 | 368596 | 368622 | 368627 | 368643 | 368644 | 368649 | 368650 | 368744