1316827 - Wasm baseline: Improve double stack adjustments following calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Lars T Hansen [:lth]]

The code for function calling is poorly abstracted (it is abstracted along the wrong lines / abstracted too much) and so ends up potentially adjusting SP twice, once in endCall to pop arguments actually passed to functions and once after the call to pop the values that were spilled by the sync() before the call.

Probably closely related to bug 1316806, which seeks to avoid the blanket sync() and to pass arguments by parallel assignment.

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

Hey :lth, I'm start ramping up with wasm and SpiderMonkey; this looks a relatively easy bug for a total newbie.
Do you have some spare cycle to briefly guide me through the code, please?

Comment 2

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Bug 1316827 - WASM: the stack is now free'd once, even when an adjustment is needed.

Review commit: https://reviewboard.mozilla.org/r/97968/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/97968/

Comment 3

[Michelangelo De Simone [Offline] [:mds]]

Alright, I hope I haven't screwed things up too much. :)
The JS test harness seems clear.

Comment 4

[Lars T Hansen [:lth]]

Comment on attachment 8817754 [details]
Bug 1316827 - WASM: the stack is now free'd once, even when an adjustment is needed.

https://reviewboard.mozilla.org/r/97968/#review98276

Thank you!

::: js/src/wasm/WasmBaselineCompile.cpp:2200
(Diff revision 1)
>  
>          call.frameAlignAdjustment = ComputeByteAlignment(masm.framePushed() + sizeof(Frame),
>                                                           JitStackAlignment);
>      }
>  
> -    void endCall(FunctionCall& call)
> +    void endCall(FunctionCall& call, size_t stackSpace = 0)

My preference would probably be for this argument not to be optional.

Comment 5

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8817754 [details]
Bug 1316827 - WASM: the stack is now free'd once, even when an adjustment is needed.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/97968/diff/1-2/

Comment 6

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Lars T Hansen [:lth] (On PTO until 27 Dec) from comment #4)

> > -    void endCall(FunctionCall& call)
> > +    void endCall(FunctionCall& call, size_t stackSpace = 0)
> 
> My preference would probably be for this argument not to be optional.

Done. Thank you for your review!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/da29c78960eb
WASM: the stack is now free'd once, even when an adjustment is needed. r=lth

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da29c78960eb