Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39): Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39):afonso360 opened PR #6955 from afonso360:riscv-use-branch-labels to bytecodealliance:main:
:wave: Hey,
This PR is a preparation for enabling the C (Compressed instructions) extension on the RISC-V backend. This extension means that we can emit a bunch of instructions as compressed instructions (2 bytes instead of 4) and essentially makes this a variable(-ish) length ISA.
The first step is to try to remove as many fixed offset jumps as possible and replace them with label based counterparts that should take into account if an instruction has been emitted as compressed or not.
Additionally and somewhat unrelated this PR also enables by default the minimum set of flags with what we expect in the backend. We currently target RISC-V GC which is the general set of extensions for application processors.
Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39):afonso360 requested wasmtime-compiler-reviewers for a review on PR #6955.
Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39):afonso360 requested abrown for a review on PR #6955.
Wasmtime GitHub notifications bot (Sep 02 2023 at 11:43):afonso360 edited PR #6955:
:wave: Hey,
This PR is a preparation for enabling the C (Compressed instructions) extension on the RISC-V backend. This extension means that we can emit a bunch of instructions as compressed instructions (2 bytes instead of 4) and essentially makes this a variable(-ish) length ISA.
The first step is to try to remove as many fixed offset jumps as possible and replace them with label based counterparts that should take into account if an instruction has been emitted as compressed or not.
Additionally and somewhat unrelated this PR also enables by default the minimum set of flags with what we expect in the backend. We currently target RISC-V GC which is the general set of extensions for application processors. This is also aligned with the rust toolchain target for Cranelift/Wasmtime which is
riscv64gc-....
Wasmtime GitHub notifications bot (Sep 05 2023 at 18:48):alexcrichton submitted PR review:
Some minor follow-up questions, but looks good to me :+1:
Wasmtime GitHub notifications bot (Sep 05 2023 at 18:48):alexcrichton created PR review comment:
As a follow-up, should
Inst::INSTRUCTION_SIZEbe deleted if the instruction size is going to become variable?
Wasmtime GitHub notifications bot (Sep 05 2023 at 18:48):alexcrichton created PR review comment:
Question that's somewhat unrelated to this PR but this inspired me to ask. IIRC I don't remember seeing lots of
has_fchecks, for example, in thelower.isleorinst.islebits we have for RISC-V. I'm not sure whether that extension is itself relevant here, but this seems like a bug perhaps?For example if
has_fis disabled then I'd expect the backend would either automatically pick other instructions or if it's a require extension then we'd generate an error during construction of the ISA saying "required features are not enabled". Otherwise I think today what might happen is that ifhas_fis disabled then we'd generate instructions from thefinstruction set?(and again sorry I don't mean to pick on
fhere, I don't actually know what any of these extensions are, I just picked it randomly and this question applies to all of these enabled-by-default ones here)
Wasmtime GitHub notifications bot (Sep 05 2023 at 18:48):alexcrichton submitted PR review:
Some minor follow-up questions, but looks good to me :+1:
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:15):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:15):afonso360 created PR review comment:
Otherwise I think today what might happen is that if has_f is disabled then we'd generate instructions from the f instruction set?
Yeah that is exactly what happens. I don't think we ever check these base features in the backend only the non default ones.
Adding those checks when constructing the ISA is a really good idea, I'm going to do that in a follow up PR.
The
fextension is support for single precision floating point, the rest of them are equally basic stuff like that (I'm also going to add that to the flag description)
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):alexcrichton submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):alexcrichton created PR review comment:
Ok cool makes sense! If it helps this is the example I was thinking of that used to be in the x64 backend:
that's since been removed for other reasons but I imagine there's a similar location to put this for the RISC-V backend.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):afonso360 submitted PR review.
Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):afonso360 created PR review comment:
I think we should rename it to something like
UNCOMPRESSED_INSTRUCTION_SIZE. It's still used in a couple of places for example when checking if we need to emit an island.It is also used when calculating
br_tableoffsets, which are going to be forced to emit uncompressed instructions.
Wasmtime GitHub notifications bot (Sep 06 2023 at 20:03):afonso360 merged PR #6955.
Last updated: Nov 22 2024 at 17:03 UTC