1485162 - remove confusing StackDecrementForCall() overloads

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement)

Description

[Luke Wagner [:luke]]

There are 3 overloads for StackDecrementForCall(): one shared with all Ion that makes sense, and 2 that do special stuff and superficially look the same as the shared one.  I'm always confusing these so I think it's better to have all of WasmStubs.cpp just use the one common one.  This requires being a bit explicit about what has been pushed for the alreadyPushed argument, but I think that's actually good.

Comment 1

[Luke Wagner [:luke]]

Attachment: kill-overloads

Comment 2

[Luke Wagner [:luke]]

Attachment: rm-dead-field

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9002933 [details] [diff] [review]
kill-overloads

Review of attachment 9002933 [details] [diff] [review]:
-----------------------------------------------------------------

Ha nice, I've been confused by these overloads in the past.

::: js/src/wasm/WasmStubs.cpp
@@ +983,5 @@
>      AssertExpectedSP(masm);
>  
>      GenerateFunctionPrologue(masm, funcTypeId, Nothing(), offsets);
>  
> +    unsigned framePushed = StackDecrementForCall(WasmStackAlignment,

In every single other case, it was obvious that masm.framePushed() is 0 because of a visible previous call to setFramePushed(0). Here, it's more subtle; there's an assertion at the end of GenerateFunctionPrologue, but this knowledge is hidden within the function's body. Could we re-assert here instead? or comment?

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 9002934 [details] [diff] [review]
rm-dead-field

Review of attachment 9002934 [details] [diff] [review]:
-----------------------------------------------------------------

Sweet, thanks.

Comment 5

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #3)
> Could we re-assert here instead? or comment?

Good point!  It is a rather subtle detail so I improved all the calls so that they are either (1) using masm.framePushed(), (2) using sizeof(Frame) with a comment "// pushed by prologue".  (Can't assert b/c the constant is usually computed before the Generate*Prologue(), so masm.framePushed() is 0 before and sizeof(Frame)+framePushed after.)

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/249688402db2
remove dead CodeGeneratorShared field (r=bbouvier)
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ff39f587a0d
Baldr: remove confusing StackDecrementForCall() overloads (r=bbouvier)

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/249688402db2
https://hg.mozilla.org/mozilla-central/rev/5ff39f587a0d