Bug 204317 – [JSC] DFG terminal's liveness should respect caller's opcodeID

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

<rdar://problem/54857307>

[Yusuke Suzuki]

Created attachment 383835 [details]
Patch

It "works"...

[Yusuke Suzuki]

Talked with Saam and Phil, Let's introduce BeforeUse/AfterUse notation to bytecode liveness to avoid making some of them unnecessary live.
Add Comment

[Yusuke Suzuki]

Created attachment 384015 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Yusuke Suzuki]

(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.

[Saam Barati]

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?

[Yusuke Suzuki]

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

[Yusuke Suzuki]

Committed r252789: <https://trac.webkit.org/changeset/252789>