Wasmtime GitHub notifications bot (Apr 01 2024 at 07:00): Wasmtime GitHub notifications bot (Apr 01 2024 at 07:00):jameysharp opened PR #8275 from jameysharp:fix-retcall-indirect to bytecodealliance:main:
x64 now uses a dedicated codegen strategy for tail-calls which writes the callee's stack arguments directly into their final location, but aarch64 and riscv64 still use our previous strategy of setting up for a normal function call and then moving the new stack frame up to overwrite the old one. In order to emit correct code for a normal function call, we need to fake a normal Call/CallIndirect opcode rather than ReturnCall/ReturnCallIndirect.
The new tests are a combination of x64's compile-tests for return-call-indirect.clif, plus a large-stack-frame test from aarch64/riscv64's return-call.clif modified to do an indirect call.
Wasmtime GitHub notifications bot (Apr 01 2024 at 07:00):jameysharp requested fitzgen for a review on PR #8275.
Wasmtime GitHub notifications bot (Apr 01 2024 at 07:00):jameysharp requested wasmtime-compiler-reviewers for a review on PR #8275.
Wasmtime GitHub notifications bot (Apr 01 2024 at 07:44):github-actions[bot] commented on PR #8275:
Subscribe to Label Action
cc @cfallin, @fitzgen
<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "isle"Thus the following users have been cc'd because of the following labels:
- cfallin: isle
- fitzgen: isle
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
Wasmtime GitHub notifications bot (Apr 01 2024 at 15:50):fitzgen submitted PR review:
Can we have some runtests exercising this stuff as well?
Wasmtime GitHub notifications bot (Apr 01 2024 at 16:14):jameysharp commented on PR #8275:
Can we have some runtests exercising this stuff as well?
We do have
cranelift/filetests/filetests/runtests/return-call-indirect.clif, we just didn't have the indirect case tested in compile-tests for aarch64 and riscv64. Do you want me to just add a similar stack-arguments test there? It wouldn't have caught this bug because we havepreserve_frame_pointers=truein that test, since x64 currently requires it, but there's no harm adding it. I could do the same in the x64 compile tests for that matter.
Wasmtime GitHub notifications bot (Apr 01 2024 at 16:25):fitzgen commented on PR #8275:
I think it would be good to have, yeah
Wasmtime GitHub notifications bot (Apr 01 2024 at 18:12):jameysharp updated PR #8275.
Wasmtime GitHub notifications bot (Apr 01 2024 at 18:12):jameysharp has enabled auto merge for PR #8275.
Wasmtime GitHub notifications bot (Apr 01 2024 at 19:18):jameysharp merged PR #8275.
Last updated: Nov 22 2024 at 16:03 UTC