Wasmtime GitHub notifications bot (Jun 24 2024 at 17:22): Wasmtime GitHub notifications bot (Jun 24 2024 at 17:22):afonso360 opened PR #8868 from afonso360:riscv-inst-size-2 to bytecodealliance:main:
:wave: Hey,
This PR fixes #8866. It is also a follow up to #8850, in that PR I forgot to add some of the fields into the frame layout which end up having a codegen effect for the return call instruction.
I've updated these fields with a somewhat real but worse case scenario. This ends up generating a lot of instructions though.
Wasmtime GitHub notifications bot (Jun 24 2024 at 17:22):afonso360 requested cfallin for a review on PR #8868.
Wasmtime GitHub notifications bot (Jun 24 2024 at 17:22):afonso360 requested wasmtime-compiler-reviewers for a review on PR #8868.
Wasmtime GitHub notifications bot (Jun 24 2024 at 17:26):cfallin submitted PR review:
LGTM, thanks!
I'm idly wondering whether we need a better mechanism for island deadlines with such a large worst-case: number of instructions * max instruction size becomes a very very loose upper bound with such high variance in pseudoinst size. I don't think there's a good way to split return-call sequences into multiple pseudoinsts though. Definitely don't want to go back to a two-pass "compute the size first" mechanism, that will pessimize the common case. Hmm...
Wasmtime GitHub notifications bot (Jun 24 2024 at 18:53):alexcrichton commented on PR #8868:
The test failures here show something that I was a little worried about in that the jump distance of riscv64 is pretty limited so with the very large max worst case times the number of instructions it means that islands are getting emitted much more frequently, even for relatively small functions.
IIRC though I think it's an option to, in the implementation of lowering of
ReturnCall, that island checks are explicitly inserted? That way theReturnCallinstruction itself could calculate the size it needs, perform custom island-checking logic, and otherwise the maximum size of an instruction could go back to being significantly smaller (e.g. 5 or so instructions)
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:01):cfallin commented on PR #8868:
Yes, there is a mechanism for this, and we use it for
br_tables in aarch64 here; that's probably the better option to replicate for return-call pseudoinsts.
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:25):afonso360 updated PR #8868.
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:28):afonso360 commented on PR #8868:
Yeah, that's a much better solution! The calculation for the exact number of instructions was fairly complex so I emitted it into a fake buffer and used that. If that is too much overhead, maybe we can find some conservative limit for estimating the isle distance?
Also, the new instruction size limit is so low that I'm worried we are going to find some combination of an existing instruction that is larger than that. I'm going to run the fuzzer for a bit, lets see if it finds a larger instruction.
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:34):cfallin submitted PR review:
New approach LGTM, thanks! Good idea to check the reduced limit.
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:37):afonso360 updated PR #8868.
Wasmtime GitHub notifications bot (Jun 24 2024 at 19:40):Maybe rename this PR?
Last updated: Oct 23 2024 at 20:03 UTC