Wasmtime GitHub notifications bot (Aug 30 2023 at 13:41): Wasmtime GitHub notifications bot (Aug 30 2023 at 13:41):afonso360 opened PR #6933 from afonso360:riscv-pool-constants to bytecodealliance:main:
:wave: Hey,
This is a follow up to #6915 (and #5578 I guess). This PR enables loading ISLE constants from the constant pool.
Not all constants are loaded from the constant pool. Constants that are observed at emit time are still loaded inline since I couldn't find a way of registering them with the MachBuffer for loading.
This also causes a slight pessimization of the dynamic instruction count since we currently force the address to be fully loaded into a register before a load when resolving labels. This is easily fixable, but I'm going to do it in a follow-up PR.
Wasmtime GitHub notifications bot (Aug 30 2023 at 13:41):afonso360 requested abrown for a review on PR #6933.
Wasmtime GitHub notifications bot (Aug 30 2023 at 13:41):afonso360 requested wasmtime-compiler-reviewers for a review on PR #6933.
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:27):alexcrichton submitted PR review:
Nice! One comment about something that's perhaps preexisting, but this looks good to me regardless of that.
Constants that are observed at emit time are still loaded inline since I couldn't find a way of registering them with the MachBuffer for loading.
My guess is that if possible it'd be best to break up the macro-instruction into its components to handle this. That way instead of using macro-instructions the helpers in ISLE can be used. This may not always be applicable though.
I did look at a few of these in
emit.rsthough and they were related to things like sp adjustments and stack probe loops which I think could also reasonably be switched over to asserting that the constant fits in an instruction rather than requiring a pool. (so long as the range matches what one would reasonably expect for these situations)
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:27):alexcrichton submitted PR review:
Nice! One comment about something that's perhaps preexisting, but this looks good to me regardless of that.
Constants that are observed at emit time are still loaded inline since I couldn't find a way of registering them with the MachBuffer for loading.
My guess is that if possible it'd be best to break up the macro-instruction into its components to handle this. That way instead of using macro-instructions the helpers in ISLE can be used. This may not always be applicable though.
I did look at a few of these in
emit.rsthough and they were related to things like sp adjustments and stack probe loops which I think could also reasonably be switched over to asserting that the constant fits in an instruction rather than requiring a pool. (so long as the range matches what one would reasonably expect for these situations)
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:27):alexcrichton created PR review comment:
This is preexisting I think, and probably affects other backends too, but technically emitting a 64-bit constant here is over-approximating how large it needs to be, right? In that only the
ty-width sized bit-pattern ofcis all that matters so for a 32-bit constant the 64-bit size is overkill?No need to fix here since this preserves what was done before, but may be worth trying to fix later?
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:45):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:45):afonso360 created PR review comment:
My plan is to eventually fix #6922 and then we should be able to change this to only ever trigger for 64 bit constants, since all other constants should be able to be emitted with the previous rules (i.e. with constant materialization instructions).
Wasmtime GitHub notifications bot (Aug 30 2023 at 14:56):afonso360 updated PR #6933.
Wasmtime GitHub notifications bot (Aug 30 2023 at 15:00):afonso360 has enabled auto merge for PR #6933.
Wasmtime GitHub notifications bot (Aug 30 2023 at 16:33):afonso360 merged PR #6933.
Last updated: Oct 23 2024 at 20:03 UTC