Stream: git-wasmtime

Topic: wasmtime / PR #6955 riscv64: Use labels for branches when...


view this post on Zulip 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.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39):

afonso360 requested wasmtime-compiler-reviewers for a review on PR #6955.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 02 2023 at 11:39):

afonso360 requested abrown for a review on PR #6955.

view this post on Zulip 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-....

view this post on Zulip 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:

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2023 at 18:48):

alexcrichton created PR review comment:

As a follow-up, should Inst::INSTRUCTION_SIZE be deleted if the instruction size is going to become variable?

view this post on Zulip 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_f checks, for example, in the lower.isle or inst.isle bits 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_f is 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 if has_f is disabled then we'd generate instructions from the f instruction set?

(and again sorry I don't mean to pick on f here, 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)

view this post on Zulip 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:

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2023 at 19:15):

afonso360 submitted PR review.

view this post on Zulip 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 f extension 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)

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):

alexcrichton submitted PR review.

view this post on Zulip 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:

https://github.com/bytecodealliance/wasmtime/blob/66fd3d72c9d96b06df2dc5d930f12ccaa98f23f6/cranelift/codegen/src/isa/x64/mod.rs#L225-L233

that's since been removed for other reasons but I imagine there's a similar location to put this for the RISC-V backend.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 05 2023 at 19:18):

afonso360 submitted PR review.

view this post on Zulip 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_table offsets, which are going to be forced to emit uncompressed instructions.

view this post on Zulip Wasmtime GitHub notifications bot (Sep 06 2023 at 20:03):

afonso360 merged PR #6955.


Last updated: Dec 23 2024 at 12:05 UTC