bytecodealliance / wasmtime

A fast and secure runtime for WebAssembly
https://wasmtime.dev/
Apache License 2.0
14.81k stars 1.24k forks source link

riscv64: Dynamically emit islands for return calls #8868

Closed afonso360 closed 1 week ago

afonso360 commented 1 week ago

👋 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.

alexcrichton commented 1 week ago

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 the ReturnCall instruction 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)

cfallin commented 1 week ago

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.

afonso360 commented 1 week ago

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.

bjorn3 commented 1 week ago

Maybe rename this PR?

afonso360 commented 1 week ago

This has been fuzzing for a few hours now without issues, so maybe we have an accurate limit? Guess we'll find out.