Bug 171262 – [JSC] Handle PhantomSpread in LoadVarargs as the same to the others

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] Handle PhantomSpread in LoadVarargs as the same to the others

[Yusuke Suzuki]

Created attachment 308074 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 308074 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:880
>                          InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame;

This is buggy part of this patch. But we cannot test it since the following code,

function a(...args)
{
    return b(args);
}

function b(args)
{
    return c(...args);
}

function c(a0, a1, a2, a3, a4)
{
    return a2;
}

a(0, 1, 2, 3, 4, 5);

will emit PutStack for args, and make it usual Spread & CreateRest instead of PhantomSpread.
Thus, we cannot observe this bug fixed in this patch.

[Saam Barati]

Comment on attachment 308074 [details]
Patch

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

>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:880
>>                          InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame;
> 
> This is buggy part of this patch. But we cannot test it since the following code,
> 
> function a(...args)
> {
>     return b(args);
> }
> 
> function b(args)
> {
>     return c(...args);
> }
> 
> function c(a0, a1, a2, a3, a4)
> {
>     return a2;
> }
> 
> a(0, 1, 2, 3, 4, 5);
> 
> will emit PutStack for args, and make it usual Spread & CreateRest instead of PhantomSpread.
> Thus, we cannot observe this bug fixed in this patch.

I'm confused. There exists no test for this? How is that possible?

[Yusuke Suzuki]

Comment on attachment 308074 [details]
Patch

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

>>> Source/JavaScriptCore/dfg/DFGArgumentsEliminationPhase.cpp:880
>>>                          InlineCallFrame* inlineCallFrame = candidate->origin.semantic.inlineCallFrame;
>> 
>> This is buggy part of this patch. But we cannot test it since the following code,
>> 
>> function a(...args)
>> {
>>     return b(args);
>> }
>> 
>> function b(args)
>> {
>>     return c(...args);
>> }
>> 
>> function c(a0, a1, a2, a3, a4)
>> {
>>     return a2;
>> }
>> 
>> a(0, 1, 2, 3, 4, 5);
>> 
>> will emit PutStack for args, and make it usual Spread & CreateRest instead of PhantomSpread.
>> Thus, we cannot observe this bug fixed in this patch.
> 
> I'm confused. There exists no test for this? How is that possible?

The problem is that `spread->child1()->origin.semantic.inlineCallFrame` could be different from `spread->origin.semantic.inlineCallFrame`.
If it happens with LoadVarargs, the observable bug occurs.
However, currently, this case does not occur. This is just because PutStack always escapes the spread in this case. So it is not testable.
But it's not logically correct and it breaks things if this phase is merged into Object Allocation Sinking.

[Saam Barati]

Oh I see, this is because we run putStackSinkingPhase after arguments elimination :(

[Saam Barati]

(In reply to Saam Barati from comment #5)
> Oh I see, this is because we run putStackSinkingPhase after arguments
> elimination :(

I wonder if we should run putStackSinkingPhase first, then run argumentsElimination, and then run putStackSinkingPhase again but only if argumentsElimination did anything.

We should obviously be able to transform your example program to not allocate Createrest/Spread.

Fil, Yusuke, any thoughts on this proposal?

[Saam Barati]

Comment on attachment 308074 [details]
Patch

r=me. I agree with this change even if we have the PutStack issue.

[Saam Barati]

I applied your change locally, along with this change to Plan:

```
        if (Options::usePutStackSinking())
            RUN_PHASE(performPutStackSinking);
        changed = false;
        RUN_PHASE(performArgumentsElimination);
        if (changed && Options::usePutStackSinking())
            RUN_PHASE(performPutStackSinking);
```

And ran this program:
```
"use strict";

function assert(b) {
    if (!b)
        throw new Error("Bad");
}

function foo(...args) {
    return bar(args);
}
noInline(foo);

function bar(args) {
    return baz(...args);
}

function baz(a, b) {
    return a + b;
}

for (let i = 0; i < 10000; ++i) {
    assert(foo(i, i+1) === (i + (i + 1)));
}
```

And your patch fixes the crash.

[Filip Pizlo]

(In reply to Saam Barati from comment #6)
> (In reply to Saam Barati from comment #5)
> > Oh I see, this is because we run putStackSinkingPhase after arguments
> > elimination :(
> 
> I wonder if we should run putStackSinkingPhase first, then run
> argumentsElimination, and then run putStackSinkingPhase again but only if
> argumentsElimination did anything.
> 
> We should obviously be able to transform your example program to not
> allocate Createrest/Spread.
> 
> Fil, Yusuke, any thoughts on this proposal?

I think that we need to combine the PutStack, ArgumentsElimination, and ObjectAllocationSinking phases should in the long term be combined into one phase, that assumes that nothing escapes as its initial bottom result and then builds up from there.

Otherwise trying to fixpoint these phases means we start at top and incremnetally optimize.

[Yusuke Suzuki]

(In reply to Filip Pizlo from comment #9)
> (In reply to Saam Barati from comment #6)
> > (In reply to Saam Barati from comment #5)
> > > Oh I see, this is because we run putStackSinkingPhase after arguments
> > > elimination :(
> > 
> > I wonder if we should run putStackSinkingPhase first, then run
> > argumentsElimination, and then run putStackSinkingPhase again but only if
> > argumentsElimination did anything.
> > 
> > We should obviously be able to transform your example program to not
> > allocate Createrest/Spread.
> > 
> > Fil, Yusuke, any thoughts on this proposal?
> 
> I think that we need to combine the PutStack, ArgumentsElimination, and
> ObjectAllocationSinking phases should in the long term be combined into one
> phase, that assumes that nothing escapes as its initial bottom result and
> then builds up from there.
> 
> Otherwise trying to fixpoint these phases means we start at top and
> incremnetally optimize.

Agreed.

[Yusuke Suzuki]

(In reply to Saam Barati from comment #8)
> I applied your change locally, along with this change to Plan:
> 
> ```
>         if (Options::usePutStackSinking())
>             RUN_PHASE(performPutStackSinking);
>         changed = false;
>         RUN_PHASE(performArgumentsElimination);
>         if (changed && Options::usePutStackSinking())
>             RUN_PHASE(performPutStackSinking);
> ```
> 
> And ran this program:
> ```
> "use strict";
> 
> function assert(b) {
>     if (!b)
>         throw new Error("Bad");
> }
> 
> function foo(...args) {
>     return bar(args);
> }
> noInline(foo);
> 
> function bar(args) {
>     return baz(...args);
> }
> 
> function baz(a, b) {
>     return a + b;
> }
> 
> for (let i = 0; i < 10000; ++i) {
>     assert(foo(i, i+1) === (i + (i + 1)));
> }
> ```
> 
> And your patch fixes the crash.

Yeah. I've added the above code to the test too to ensure this works if PutStack sinking phase is merged.

[Yusuke Suzuki]

Committed r215860: <http://trac.webkit.org/changeset/215860>