Bug 197405 - [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed frame
Summary: [JSC] Inlining Getter/Setter should care availability of ad-hocly constructed...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-04-30 01:28 PDT by Yusuke Suzuki
Modified: 2019-05-01 19:40 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.88 KB, patch)
2019-04-30 01:31 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.01 KB, patch)
2019-04-30 13:08 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2019-04-30 13:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2019-04-30 15:57 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (10.02 KB, patch)
2019-04-30 16:37 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-highsierra (8.58 MB, application/zip)
2019-04-30 18:33 PDT, EWS Watchlist
no flags Details
Patch (11.97 KB, patch)
2019-04-30 18:40 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.94 MB, application/zip)
2019-04-30 20:48 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.16 MB, application/zip)
2019-04-30 20:49 PDT, EWS Watchlist
no flags Details
Patch (16.12 KB, patch)
2019-05-01 18:17 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yusuke Suzuki 2019-04-30 01:28:24 PDT
Currenlty, we are setting up arguments for getter/setter callings before doing handleCall. But this is wrong. Let’s consider the following case,

  var counter = 0;
  var o = {
    get f() {
      return o
    },
    set f(v) {
      counter++;
      this.z = 0;
    }
  };
  function foo(o) {
    o.f = 0; // setter call. Inlining in DFG.
    return o.f; // getter call. Inlining in DFG.
  }
  noInline(foo);
  foo(o);

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)
    ...
    /* Multiple CallVariants. So let's switch on executables. */
    ...
    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>


If OSR exit occurs <HERE>, we construct Stack based on availability. But now, loc4 & loc5 (|this| and |arg1| for setter) availability can be pruned at the beginning of BB#2 since bc#7 do not need to make them live!
Comment 1 Yusuke Suzuki 2019-04-30 01:29:05 PDT
<rdar://problem/50008714>
Comment 2 Yusuke Suzuki 2019-04-30 01:31:48 PDT
Created attachment 368542 [details]
Patch

WIP
Comment 3 Yusuke Suzuki 2019-04-30 13:08:30 PDT
Created attachment 368594 [details]
Patch
Comment 4 Yusuke Suzuki 2019-04-30 13:10:11 PDT
Created attachment 368596 [details]
Patch
Comment 5 Yusuke Suzuki 2019-04-30 15:57:24 PDT
Created attachment 368622 [details]
Patch
Comment 6 Yusuke Suzuki 2019-04-30 16:37:35 PDT
Created attachment 368627 [details]
Patch
Comment 7 Saam Barati 2019-04-30 16:42:47 PDT
Comment on attachment 368627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368627&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
> +        // 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

there is no @21.
Comment 8 Saam Barati 2019-04-30 17:03:57 PDT
Comment on attachment 368627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368627&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1614
> +        // When inlining getter and setter calls, we setup the stack frame which does not appear in the bytecode.

"the stack frame" => "a stack frame"

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1615
> +        // But inlining call can have switch on executable. So we could have a graph like this.

"But inlining call can have switch on executable. So we could have a graph like this." => "Because Inlining can switch on executable, we could have a graph like this."

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1634
> +        //     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)

This has more detail than needed IMO. However, because you're changing this, it'd important to commend on how this gets changed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>> +        // 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
> 
> there is no @21.

I think the key just boils down to:
we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)
Comment 9 Saam Barati 2019-04-30 17:06:07 PDT
Comment on attachment 368627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368627&action=review

>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>> +        // 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
>> 
>> there is no @21.
> 
> I think the key just boils down to:
> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)

The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:


```
If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
```
Comment 10 Saam Barati 2019-04-30 17:07:20 PDT
Comment on attachment 368627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368627&action=review

>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>>> +        // 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
>>> 
>>> there is no @21.
>> 
>> I think the key just boils down to:
>> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
>> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)
> 
> The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:
> 
> 
> ```
> If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
> need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
> callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
> ```

maybe also continue to say in the first sentence why they can be pruned.
Comment 11 Yusuke Suzuki 2019-04-30 18:29:53 PDT
Comment on attachment 368627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368627&action=review

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1614
>> +        // When inlining getter and setter calls, we setup the stack frame which does not appear in the bytecode.
> 
> "the stack frame" => "a stack frame"

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1615
>> +        // But inlining call can have switch on executable. So we could have a graph like this.
> 
> "But inlining call can have switch on executable. So we could have a graph like this." => "Because Inlining can switch on executable, we could have a graph like this."

Fixed.

>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1634
>> +        //       45:<!0:->   ExitOK(MustGen, R:Stack(-13), W:SideState, bc#0, ExitValid)
> 
> This has more detail than needed IMO. However, because you're changing this, it'd important to commend on how this gets changed.

OK, simplified.

>>>>> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1638
>>>>> +        // 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
>>>> 
>>>> there is no @21.
>>> 
>>> I think the key just boils down to:
>>> we need MovHints in the callee's code origin at the start of our code because those can't be pruned.
>>> The caller's code, because these are manufactured locals, can always prune them (which may have some other bugs the more I think about it)
>> 
>> The wording in this paragraph should address what this fixes. It shouldn't say that we still do the wrong thing in the present tense. Maybe something like:
>> 
>> 
>> ```
>> If we prune OSR availability at bc#7 in the caller, we can prune @19 and @21. When we begin executing the callee, we
>> need OSR exit to be aware of where it can recover the arguments to the setter, loc4 and loc5. The MovHints in the inlined
>> callee make it so that if we exit at <HERE>, we can recover loc4/loc5.
>> ```
> 
> maybe also continue to say in the first sentence why they can be pruned.

Fixed.

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1653
>      if (arityFixupCount) {

Discussed with Saam on IRC, we should move the above and arity fixup into callee.
Comment 12 EWS Watchlist 2019-04-30 18:33:26 PDT
Comment on attachment 368627 [details]
Patch

Attachment 368627 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12046604

New failing tests:
workers/bomb.html
Comment 13 EWS Watchlist 2019-04-30 18:33:28 PDT
Created attachment 368643 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 14 Yusuke Suzuki 2019-04-30 18:40:56 PDT
Created attachment 368644 [details]
Patch
Comment 15 Saam Barati 2019-04-30 19:57:13 PDT
Comment on attachment 368644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368644&action=review

r=me

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1694
> +                VirtualRegister argumentToSet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffsetAfterFixup));

nit: as we spoke about on IRC, we think this can be:
m_inlineStackTop->remapOperand(virtualRegisterForArgument(index))
Comment 16 Saam Barati 2019-04-30 19:58:17 PDT
Comment on attachment 368644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368644&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1648
> +            VirtualRegister argumentToGet = callerStackTop->remapOperand(virtualRegisterForArgument(index, registerOffset));

nit: I think same thing here w.r.t remapOperand.
Comment 17 EWS Watchlist 2019-04-30 20:48:39 PDT
Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12047953

New failing tests:
http/tests/preload/download_resources_from_header_iframe.html
Comment 18 EWS Watchlist 2019-04-30 20:48:51 PDT
Created attachment 368649 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 19 EWS Watchlist 2019-04-30 20:49:42 PDT
Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12047895

New failing tests:
imported/w3c/web-platform-tests/streams/readable-streams/general.dedicatedworker.html
imported/w3c/web-platform-tests/streams/readable-streams/general.html
Comment 20 EWS Watchlist 2019-04-30 20:49:44 PDT
Created attachment 368650 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 21 EWS Watchlist 2019-04-30 21:10:07 PDT
Comment on attachment 368644 [details]
Patch

Attachment 368644 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/12047773

New failing tests:
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-tls-context
Comment 22 Yusuke Suzuki 2019-04-30 21:43:45 PDT
(In reply to Build Bot from comment #21)
> Comment on attachment 368644 [details]
> Patch
> 
> Attachment 368644 [details] did not pass jsc-ews (mac):
> Output: https://webkit-queues.webkit.org/results/12047773
> 
> New failing tests:
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> tls-context

Looking this now. It seems this is real.
Comment 23 Yusuke Suzuki 2019-04-30 21:44:13 PDT
(In reply to Yusuke Suzuki from comment #22)
> (In reply to Build Bot from comment #21)
> > Comment on attachment 368644 [details]
> > Patch
> > 
> > Attachment 368644 [details] did not pass jsc-ews (mac):
> > Output: https://webkit-queues.webkit.org/results/12047773
> > 
> > New failing tests:
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > tls-context
> 
> Looking this now. It seems this is real.

Moving arity fixup causes this issue. I'm investigating what is going on.
Comment 24 Yusuke Suzuki 2019-04-30 23:53:47 PDT
(In reply to Yusuke Suzuki from comment #23)
> (In reply to Yusuke Suzuki from comment #22)
> > (In reply to Build Bot from comment #21)
> > > Comment on attachment 368644 [details]
> > > Patch
> > > 
> > > Attachment 368644 [details] did not pass jsc-ews (mac):
> > > Output: https://webkit-queues.webkit.org/results/12047773
> > > 
> > > New failing tests:
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > > tls-context
> > 
> > Looking this now. It seems this is real.
> 
> Moving arity fixup causes this issue. I'm investigating what is going on.

In loop_hint, something super unrelated local is live in DFG and dead in bytecode, which leads to crash in OSR entry.
Comment 25 Yusuke Suzuki 2019-05-01 00:49:59 PDT
(In reply to Yusuke Suzuki from comment #24)
> (In reply to Yusuke Suzuki from comment #23)
> > (In reply to Yusuke Suzuki from comment #22)
> > > (In reply to Build Bot from comment #21)
> > > > Comment on attachment 368644 [details]
> > > > Patch
> > > > 
> > > > Attachment 368644 [details] did not pass jsc-ews (mac):
> > > > Output: https://webkit-queues.webkit.org/results/12047773
> > > > 
> > > > New failing tests:
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-call-ic
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-slow-memory
> > > > wasm.yaml/wasm/function-tests/memory-access-past-4gib.js.wasm-no-cjit-yes-
> > > > tls-context
> > > 
> > > Looking this now. It seems this is real.
> > 
> > Moving arity fixup causes this issue. I'm investigating what is going on.
> 
> In loop_hint, something super unrelated local is live in DFG and dead in
> bytecode, which leads to crash in OSR entry.

I believe this is bug in DFGLiveCatchVariablePreservationPhase, I'll discuss it tomorrow (precisely, today, lol).
Comment 26 Yusuke Suzuki 2019-05-01 02:25:43 PDT
Comment on attachment 368644 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368644&action=review

> Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:1697
>              }

These SetLocals are somewhat "state bootstrapping" ones. Previously, we set caller's CodeOrigin. So these SetLocals are always putting them in locals, and it is not shown as put to |this|
Now, we have code origin belonging to the caller. So |this| put / move is shown as SetLocal(arg0). And then, we insert invalid Flush before this in DFGLiveCatchVariablePreservationPhase.
I have one question. Why does DFGLiveCatchVariablePreservationPhase flushes all the |this| in the seen InlinedCallFrame regardless of the liveness at the catch?
Comment 27 Saam Barati 2019-05-01 08:38:35 PDT
I think you want the NodeOrigin of your SetLocals to be what they were before:
- semantic in caller
- forExit in callee
Comment 28 Saam Barati 2019-05-01 08:41:02 PDT
I guess that actually goes against the pruning issue. How you have it now makes sense actually. Let’s figure out what the issue is.
Comment 29 Yusuke Suzuki 2019-05-01 14:00:41 PDT
Discussed with Saam and Phil, we should tell some interesting liveness information at op_enter for locals that are materialized by DFG (getter / setter arguments, arity fixups).
Comment 30 Yusuke Suzuki 2019-05-01 17:54:37 PDT
(In reply to Yusuke Suzuki from comment #29)
> Discussed with Saam and Phil, we should tell some interesting liveness
> information at op_enter for locals that are materialized by DFG (getter /
> setter arguments, arity fixups).

Discussed with Saam about DFGLiveCatchVariablePreservationPhase, and I think the fundamental problem is DFGLiveCatchVariablePreservationPhase's InlineCallFrame handling. I'll upload the patch with this fix soon.
Comment 31 Yusuke Suzuki 2019-05-01 18:17:40 PDT
Created attachment 368744 [details]
Patch
Comment 32 Yusuke Suzuki 2019-05-01 19:40:59 PDT
Committed r244864: <https://trac.webkit.org/changeset/244864>