1599722 - WebAssembly should address stack arguments from FP

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, enhancement, P2)

Description

[Dmitry Bezhetskov]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Comment 1

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - remove tls from frame

Comment 2

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - add storing tls on cross-instance calls

Depends on D54905

Comment 3

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - allocate two words before the call and store tls

Comment 4

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - fix build for arm64

Depends on D58240

Comment 5

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - store tls & maintain tls in frame iterations

Depends on D58241

Comment 6

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - Store caller TLS on cross-instances calls and use it while iterate through wasm frame

Comment 7

[Dmitry Bezhetskov]

I've removed the TLS slot! But this requires us two additional slots right after the arguments:
To see the difference let's consider 2 examples. The first one demonstrates behavior before removing TLS and the second one shows behaviour after.

We will call foo(a, b, c, d, e, f, g, h) and will assume that this is an imported function.
Let us pass a..f via registers and g and h via stack.

Before:

  h
-----
  g
-----
retpc
-----
  tls
-----
  fp
-----

After:

  h
----------
  g
----------
caller tls
----------
callee tls
----------
retpc
----------
  fp
----------

We allocate two additional slots before the call for

1. preserving caller TLS because we change it on import calls. We can pass it before the arguments - yes. We don't need to push it on direct calls in Baseline, for example, because Baseline doesn't change WasmTlsReg on direct calls, but we don't save the extra slot because of the alignment of sp. Before that we just reload WasmTlsReg from our local copy from the frame (frame->tls), now as we removed it we couldn't do that and need to preserve it.
2. saving callee TLS to use it later for frame iteration.
   This allocation is merged into allocation space for the call so it's just increasing size in sub rsp, const.

We allocate these slots always! because the callee doesn't know (and shouldn't know) how it was called and have to read its params from stack deterministically.

Comment 8

[Dmitry Bezhetskov]

don't know what the stack layout in previous comment is bold. Argh, and I can't change it.

Comment 9

[Luke Wagner [:luke]]

Are the two extra slots only for cross-instance calls?

Comment 10

[Dmitry Bezhetskov]

Sadly for each call, we need to allocate two additional slots as two extra arguments passed only via stack.
Callee needs to access its parameters deterministically it doesn't know how it was called. So it just reads first arg by [rsp + 0x10].
But, it is the only allocation we don't push anything on same-instance calls.

Comment 11

[Dmitry Bezhetskov]

Hmm, I think I can reduce it to one slot for each call and use two additional slots for possibly cross-instance calls.
Just preserve caller TLS only on possibly cross-instance calls, on direct calls we don't need to do it.

BTW: current Ion implementation clobbers TLS reg on direct calls so we need to save that reg on direct calls in Ion too.

Comment 12

[Luke Wagner [:luke]]

Sorry, just to clarify my understanding of what you're saying, can we have:

• direct calls only push the stack args and the now-2-word wasm::Frame (returnPC and callerFP)
• entry, indirect and import calls push a 4-word wasm::TlsFrame (which contains callerTLS/calleeTLS)

If that's correct, then the latter case is mostly no worse than it is today (the calleeTLS being saved in the wasm::Frame and the callerTLS being pushed right before the call. And then we can optimize the import/indirect cases in the future when we have the thunks.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dmitry Bezhetskov from comment #8)

don't know what the stack layout in previous comment is bold. Argh, and I can't change it.

Fwiw, Bugzilla supports Markdown syntax, so the --- were interpreted as a header. I wrapped both sections within ``` so they appear as code instead.

Comment 14

[Dmitry Bezhetskov]

(In reply to Luke Wagner [:luke] from comment #12)

Sorry, just to clarify my understanding of what you're saying, can we have:

• direct calls only push the stack args and the now-2-word wasm::Frame (returnPC and callerFP)
• entry, indirect and import calls push a 4-word wasm::TlsFrame (which contains callerTLS/calleeTLS)

If that's correct, then the latter case is mostly no worse than it is today (the calleeTLS being saved in the wasm::Frame and the callerTLS being pushed right before the call. And then we can optimize the import/indirect cases in the future when we have the thunks.

Luke, now PR uses the following scheme:

We saved 5 slots for the case in the picture, so I can assume that removing tls give us some small speedup because of same-instance calls are the most frequent calls in benchmarks.
Also, some instructions like that mov 0x20(%rsp),%r14 was removed b/c now we don't reload tls every time we need it.
At least it should be no worse than the previous approach.

Comment 15

[Dmitry Bezhetskov]

Arhg, images didn't load.
Image of Baseline direct call before: https://ibb.co/XYnXNLV
Image of Baseline direct call after: https://ibb.co/B2jpmDG

Image of Baseline cross call before: https://ibb.co/VQcd0mG
Image of Baseline cross call after: https://ibb.co/k9rSFLc

Comment 16

[Dmitry Bezhetskov]

(In reply to Benjamin Bouvier [:bbouvier] from comment #13)

(In reply to Dmitry Bezhetskov from comment #8)

don't know what the stack layout in previous comment is bold. Argh, and I can't change it.

Fwiw, Bugzilla supports Markdown syntax, so the --- were interpreted as a header. I wrapped both sections within ``` so they appear as code instead.

Thanks a lot, Benjamin.

Comment 17

[Luke Wagner [:luke]]

(Oops, I forgot to reply after reading, sorry.) Yes, that sounds good.

Comment 18

[Dmitry Bezhetskov]

Attachment: Bug 1599722 make baseline free from frame's tls

This is the preparation patch for removing tls slot from the Frame.
I will untie baseline, then Ion and then remove slot from the Frame and fix frame's iteration.

     In this PR:
     I allocated private stack slot in Local area and use it for preserving tls ptr when baseline need it.

Comment 19

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - Change ABI in Baseline and Ion to read arguments via FP

Change addressing for wasm Baseline compiler.
It is needed to implement thunks effectively,
more precisely to not to copy arguments when we call a thunk.

Comment 20

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - Changed baseline addressing mode for incoming stack argument from SP based to FP based

Comment 21

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - Make Ion address stack arguments from FP

Comment 22

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - add support for other architectures

Depends on D73030

Comment 23

[Dmitry Bezhetskov]

This is the base doc for this work.
https://docs.google.com/document/d/1oi6ROZJuE-hBb21Tgq-XcUegPktecF52gADzBJcYL34/

Comment 24

[Raul Gurzau (:RaulG)]

Tried to land D72298 but it seems it needs a rebase:

Reason:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

applying /tmp/tmpC0_1e3
js/src/wasm/WasmBaselineCompile.cpp
Hunk #3 FAILED at 1669.
1 out of 5 hunks FAILED -- saving rejects to file js/src/wasm/WasmBaselineCompile.cpp.rej
abort: patch command failed: exited with status 256

Comment 25

[Dmitry Bezhetskov]

(In reply to Raul Gurzau (:RaulG) from comment #24)

Tried to land D72298 but it seems it needs a rebase:

Reason:
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

applying /tmp/tmpC0_1e3
js/src/wasm/WasmBaselineCompile.cpp
Hunk #3 FAILED at 1669.
1 out of 5 hunks FAILED -- saving rejects to file js/src/wasm/WasmBaselineCompile.cpp.rej
abort: patch command failed: exited with status 256

Thanks Raul, now it is fixed.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/60966c66ef94
Changed baseline addressing mode for incoming stack argument from SP based to FP based r=wingo,lth

Comment 27

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/60966c66ef94

Comment 28

[Dmitry Bezhetskov]

Need to land more patches to achieve the ticket's target.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/892ab673d138
Make Ion address stack arguments from FP r=lth,wingo

Comment 30

[Natalia Csoregi [:nataliaCs]]

Backed our for bustage on MoveResolver.h

backout: https://hg.mozilla.org/integration/autoland/rev/88e75626be1564343c58d91324ad601ddf4df35d

push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=892ab673d1384b5ce3dcc53dc6fd71317a10e1bb&searchStr=build&selectedTaskRun=SgyNlDjVRdWIKiPRU2EKJw-0

failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301384902&repo=autoland&lineNumber=6518

[task 2020-05-08T09:01:27.940Z] 09:01:27 INFO - In file included from /builds/worker/checkouts/gecko/js/src/jit/MacroAssembler.h:19:
[task 2020-05-08T09:01:27.940Z] 09:01:27 INFO - In file included from /builds/worker/checkouts/gecko/js/src/jit/x64/MacroAssembler-x64.h:11:
[task 2020-05-08T09:01:27.942Z] 09:01:27 ERROR - /builds/worker/checkouts/gecko/js/src/jit/MoveResolver.h:62:3: error: bad implicit conversion constructor for 'MoveOperand'
[task 2020-05-08T09:01:27.943Z] 09:01:27 INFO - MoveOperand(const Address& addr, Kind kind = MEMORY)
[task 2020-05-08T09:01:27.944Z] 09:01:27 INFO - ^
[task 2020-05-08T09:01:27.945Z] 09:01:27 INFO - /builds/worker/checkouts/gecko/js/src/jit/MoveResolver.h:62:3: note: consider adding the explicit keyword to the constructor
[task 2020-05-08T09:01:27.946Z] 09:01:27 INFO - MoveOperand(const Address& addr, Kind kind = MEMORY)
[task 2020-05-08T09:01:27.947Z] 09:01:27 INFO - ^
[task 2020-05-08T09:01:27.948Z] 09:01:27 INFO - explicit
[task 2020-05-08T09:01:27.949Z] 09:01:27 INFO - 1 error generated.
[task 2020-05-08T09:01:27.950Z] 09:01:27 INFO - /builds/worker/checkouts/gecko/config/rules.mk:750: recipe for target 'Unified_cpp_js_src_gdb0.o' failed
[task 2020-05-08T09:01:27.951Z] 09:01:27 ERROR - make[4]: *** [Unified_cpp_js_src_gdb0.o] Error 1
[task 2020-05-08T09:01:27.952Z] 09:01:27 INFO - make[4]: Leaving directory '/builds/worker/workspace/obj-build/js/src/gdb'
[task 2020-05-08T09:01:27.954Z] 09:01:27 INFO - /builds/worker/checkouts/gecko/config/recurse.mk:74: recipe for target 'js/src/gdb/target-objects' failed
[task 2020-05-08T09:01:27.955Z] 09:01:27 ERROR - make[3]: *** [js/src/gdb/target-objects] Error 2
[task 2020-05-08T09:01:27.956Z] 09:01:27 INFO - make[3]: *** Waiting for unfinished jobs....

Comment 31

[Dmitry Bezhetskov]

Thanks, now it's fixed.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56c09d42f411
Make Ion address stack arguments from FP r=lth,wingo

Comment 33

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/56c09d42f411

Comment 34

[Phabricator Automation]

Comment on attachment 9132206 [details]
Bug 1599722 make baseline free from frame's tls

Revision D66201 was moved to bug 1637868. Setting attachment 9132206 [details] to obsolete.

Comment 35

[Dmitry Bezhetskov]

Attachment: Bug 1599722 - Address incoming stack args from FP in wasm stubs r=lth,wingo

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c99c48ce561c
Address incoming stack args from FP in wasm stubs r=lth,wingo

Comment 37

[Atila Butkovits]

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

Comment 38

[Lars T Hansen [:lth]]

Dmitry, are we done with this one? It's flagged as "leave-open" but has no dependencies and there's been no activity for quite a long time.

Comment 39

[Dmitry Bezhetskov]

yes, I think we are done here.