Let's consider the following example, "use strict"; function assertImpl(cond) { if (!cond) throw new Error(); } function assert() { assertImpl.apply(undefined, arguments); } noInline(assert); When compiling `throw`, we emit a terminal and put Phantom/PhantomLocal based on the bytecode liveness. At this point, we use `op_tail_call_varargs`'s bytecode index for the liveness in "assert" function. And this involves `arguments`. But this is not necessary in fact.
<rdar://problem/54857307>
Created attachment 383835 [details] Patch It "works"...
Talked with Saam and Phil, Let's introduce BeforeUse/AfterUse notation to bytecode liveness to avoid making some of them unnecessary live. Add Comment
Created attachment 384015 [details] Patch
Comment on attachment 384015 [details] Patch Attachment 384015 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/13270399 New failing tests: imported/blink/fast/events/panScroll-crash.html
Created attachment 384021 [details] Archive of layout-test-results from ews212 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews212 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
(In reply to Build Bot from comment #6) > Created attachment 384021 [details] > Archive of layout-test-results from ews212 for win-future > > The attached test failures were seen while running run-webkit-tests on the > win-ews. > Bot: ews212 Port: win-future Platform: > CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit Checked, but looks unrelated.
Comment on attachment 384015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384015&action=review r=me > Source/JavaScriptCore/ChangeLog:29 > + in DFG, but this is wasting. "wasting" => "wasteful". Maybe also say "because varargs forwarding phase could eliminate the allocation." > Source/JavaScriptCore/ChangeLog:33 > + is not profitable, and (2) we need to be careful to make stack arguments live to allow materialization of arguments objects. why would (2) not Just Work? It seems more principled to apply this to all calls and see if there is any bug fallout. I don't see why we should special case varargs calls. > Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:43 > +// <- AfterUse > +// Use by exception handlers nice! > Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:46 > + BeforeUse = 0, is = 0 needed for anything? > Source/JavaScriptCore/dfg/DFGGraph.h:916 > + InlineCallFrame* inlineCallFrame = origin.inlineCallFrame(); > + CodeBlock* codeBlock = baselineCodeBlockFor(inlineCallFrame); > + auto instruction = codeBlock->instructions().at(bytecodeIndex.offset()); nit: if you keep this code, you should put it inside the "if". However, I think the more robust thing is for all inlined things, we used BeforeUse. > Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:94 > +# After calling, calling bytecode is claiming input registers are not used. are these comments needed?
Comment on attachment 384015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=384015&action=review Thanks! >> Source/JavaScriptCore/ChangeLog:29 >> + in DFG, but this is wasting. > > "wasting" => "wasteful". Maybe also say "because varargs forwarding phase could eliminate the allocation." Fixed. >> Source/JavaScriptCore/ChangeLog:33 >> + is not profitable, and (2) we need to be careful to make stack arguments live to allow materialization of arguments objects. > > why would (2) not Just Work? It seems more principled to apply this to all calls and see if there is any bug fallout. I don't see why we should special case varargs calls. Talked with Saam. Maybe, doing for op_call is OK. But we did not verify that all callable bytecode uses are not used after the call (for example, getter inlining by op_get_by_id). So, for now, we just do this only for op_call_varargs / op_tail_call_varargs b/c we verified that they are OK, and they are the only bytecodes which get benefit from this change. >> Source/JavaScriptCore/bytecode/BytecodeLivenessAnalysis.h:46 >> + BeforeUse = 0, > > is = 0 needed for anything? Not necessary. Removed. >> Source/JavaScriptCore/dfg/DFGGraph.h:916 >> + auto instruction = codeBlock->instructions().at(bytecodeIndex.offset()); > > nit: if you keep this code, you should put it inside the "if". > > However, I think the more robust thing is for all inlined things, we used BeforeUse. Talked with Saam. We need to ensure that bytecode does not use `uses` after inlining. And we ensured that this is true for these three bytecodes, but we do not verify it for the other bytecodes. And if a new bytecode is introduced, we need to verify it. So this white-list is necessary. >> Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm:94 >> +# After calling, calling bytecode is claiming input registers are not used. > > are these comments needed? Ditto
Committed r252789: <https://trac.webkit.org/changeset/252789>