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!
<rdar://problem/50008714>
Created attachment 368542 [details] Patch WIP
Created attachment 368594 [details] Patch
Created attachment 368596 [details] Patch
Created attachment 368622 [details] Patch
Created attachment 368627 [details] Patch
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 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 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 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 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 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
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
Created attachment 368644 [details] Patch
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 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 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
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 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
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 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
(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.
(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 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.
(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 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?
I think you want the NodeOrigin of your SetLocals to be what they were before: - semantic in caller - forExit in callee
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.
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).
(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.
Created attachment 368744 [details] Patch
Committed r244864: <https://trac.webkit.org/changeset/244864>