Attachment 368622 Details for Bug 197405 – Patch

[patch] Patch

bug-197405-20190430155723.patch (text/plain), 9.40 KB, created by Yusuke Suzuki on 2019-04-30 15:57:24 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-04-30 15:57:24 PDT

Size: 9.40 KB

patch

obsolete

>Subversion Revision: 244763
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 5778ff4fbe3694bc0bc14140680e34a04ed56f5b..9beef73275df147ab353ea4e123af8f0c16c81df 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,44 @@
>+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!).
>+
>+        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.
>+
>+        * dfg/DFGByteCodeParser.cpp:
>+        (JSC::DFG::ByteCodeParser::inlineCall):
>+        (JSC::DFG::ByteCodeParser::handleGetById):
>+        (JSC::DFG::ByteCodeParser::handlePutById):
>+
> 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..5b3979f02d7bd634c05aba1685cb1f6b4c038011 100644
>--- a/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp
>@@ -1608,6 +1608,46 @@ 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
>+        //     ...
>+        //     # Setting up arguments.
>+        //     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
>+        //     ...
>+        //     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
>+        // ad-hocly by DFG and it won't be shown in the bytecode. If <HERE> causes OSR exit, we fail to find loc4.
>+        // 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 we do not move them actually, we do not need to schedule SetLocals emission.
>+        for (int index = 0; index < argumentCountIncludingThis; ++index) {
>+            VirtualRegister argument = m_inlineStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));
>+            addToGraph(MovHint, OpInfo(argument.offset()), getDirect(argument));
>+        }
>+        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.
>@@ -4371,8 +4411,7 @@ void ByteCodeParser::handleGetById(
>     //    the dreaded arguments object on the getter, the right things happen. Well, sort of -
>     //    since we only really care about 'this' in this case. But we're not going to take that
>     //    shortcut.
>-    int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
>-    set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
>+    set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
> 
>     // We've set some locals, but they are not user-visible. It's still OK to exit from here.
>     m_exitOK = true;
>@@ -4555,9 +4594,8 @@ void ByteCodeParser::handlePutById(
>             m_inlineStackTop->remapOperand(
>                 VirtualRegister(registerOffset)).toLocal());
>     
>-        int nextRegister = registerOffset + CallFrame::headerSizeInRegisters;
>-        set(VirtualRegister(nextRegister++), base, ImmediateNakedSet);
>-        set(VirtualRegister(nextRegister++), value, ImmediateNakedSet);
>+        set(virtualRegisterForArgument(0, registerOffset), base, ImmediateNakedSet);
>+        set(virtualRegisterForArgument(1, registerOffset), value, ImmediateNakedSet);
> 
>         // We've set some locals, but they are not user-visible. It's still OK to exit from here.
>         m_exitOK = true;
>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..28896f6b405e3170b0224424509c3041dde7657f
>--- /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", "--useConcurrentJIT=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