[JSC] Handle PhantomSpread in LoadVarargs as the same to the others
Created attachment 308074 [details] Patch
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.
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?
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.
Oh I see, this is because we run putStackSinkingPhase after arguments elimination :(
(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?
Comment on attachment 308074 [details] Patch r=me. I agree with this change even if we have the PutStack issue.
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.
(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.
(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.
(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.
Committed r215860: <http://trac.webkit.org/changeset/215860>