Reland: "[wasmfx] Support arguments in stack wrapper" [v8/v8 : main]
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
PTAL, PS1 is the original change, PS2-3 is the fix (+ reusing IterateWasmFXArgBuffer in wrappers-inl.h as suggested by Jakob).
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted Code-Review+1![]( "Open in Gerrit")
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
PTAL at PS 4. Reducing the fixed frame size by 1 caused the pushes in arm64 to be misaligned.
Related details
- Clemens Backes
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Clemens Backes (Gerrit)
Clemens Backes voted Code-Review+1![]( "Open in Gerrit")
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
V8 LUCI CQ (Gerrit)
V8 LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Reland: "[wasmfx] Support arguments in stack wrapper"
This is a reland of commit 30cf6998b99e4a793d8d5d5aaca96fe0879dfaed
The issue was that the stack entry frame was treated as a wasm frame in
linkage.cc, causing it to have the wrong number of fixed slots. The
stack entry frame does not have an instance slot.
Drive-by: reuse IterateWasmFXArgBuffer in the wrapper code.
Original change's description:
> Now that stack wrappers are cached per signature, allow them to have
> arguments.
> The wrapper receives a pointer to the arg buffer, unpacks it and
> forwards the arguments to the initial func ref.
R=clem...@chromium.org
CC=jkum...@chromium.org,f...@chromium.org
- M src/compiler/backend/arm64/code-generator-arm64.cc
- M src/compiler/linkage.cc
- M src/compiler/linkage.h
- M src/compiler/wasm-compiler-definitions.cc
- M src/compiler/wasm-compiler.cc
- M src/compiler/wasm-compiler.h
- M src/wasm/module-compiler.cc
- M src/wasm/turboshaft-graph-interface.cc
- M src/wasm/turboshaft-graph-interface.h
- M src/wasm/wasm-code-manager.cc
- M src/wasm/wasm-code-manager.h
- M src/wasm/wasm-stack-wrapper-cache.cc
- M src/wasm/wrappers-inl.h
- M test/mjsunit/wasm/stack-switching-params.js
Code-Review: +1 by Clemens Backes
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
required_slots--;Hi Thibaud, could I ask why this change was only needed on arm64? Trying to figure out if ppc64/s390x also need a port of this.
Related details
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 1 comment![]( "Open in Gerrit")
required_slots--;Hi Thibaud, could I ask why this change was only needed on arm64? Trying to figure out if ppc64/s390x also need a port of this.
Hi Milad. In short: this is needed if these platforms expect 16 byte stack alignment.
This is related to the change in linkage.cc: this declares the fixed slot count of this frame type to be 3 instead of 4, which is correct, but to keep the stack 16-byte aligned we push a padding register here, and remove 1 from the number of slots that we need to reserve for the rest of the frame.
This was guarded by DCHECKs on arm64. If this did not break ppc64/s390x, either this is not needed, or you might want to consider adding similar DCHECKs.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
required_slots--;Thank you for the information. Our ports are taken mainly from arm (not arm64) and I'm assuming if arm needs the same stack alignment then we may not need to add this change either?
Related details
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thibaud Michaud (Gerrit)
Thibaud Michaud added 2 comments![]( "Open in Gerrit")
required_slots--;Thibaud MichaudHi Thibaud, could I ask why this change was only needed on arm64? Trying to figure out if ppc64/s390x also need a port of this.
Hi Milad. In short: this is needed if these platforms expect 16 byte stack alignment.
This is related to the change in linkage.cc: this declares the fixed slot count of this frame type to be 3 instead of 4, which is correct, but to keep the stack 16-byte aligned we push a padding register here, and remove 1 from the number of slots that we need to reserve for the rest of the frame.This was guarded by DCHECKs on arm64. If this did not break ppc64/s390x, either this is not needed, or you might want to consider adding similar DCHECKs.
Done
required_slots--;Thank you for the information. Our ports are taken mainly from arm (not arm64) and I'm assuming if arm needs the same stack alignment then we may not need to add this change either?
This change ensures that we keep the SP two-pointers aligned at all time within the frame, which I think is only a requirement on arm64 and not on arm.
I don't know what the requirements are on ppc64 and s390x.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
required_slots--;Thibaud MichaudThank you for the information. Our ports are taken mainly from arm (not arm64) and I'm assuming if arm needs the same stack alignment then we may not need to add this change either?
This change ensures that we keep the SP two-pointers aligned at all time within the frame, which I think is only a requirement on arm64 and not on arm.
I don't know what the requirements are on ppc64 and s390x.
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Milad Farazmand (Gerrit)
Milad Farazmand added 1 comment![]( "Open in Gerrit")
required_slots--;Thibaud MichaudThank you for the information. Our ports are taken mainly from arm (not arm64) and I'm assuming if arm needs the same stack alignment then we may not need to add this change either?
Milad FarazmandThis change ensures that we keep the SP two-pointers aligned at all time within the frame, which I think is only a requirement on arm64 and not on arm.
I don't know what the requirements are on ppc64 and s390x.
Thanks again for clarifying.
Done
Related details
- Thibaud Michaud
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |