Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv64: Use labels for branches whenever possible #6955

Merged
merged 5 commits into from
Sep 6, 2023

Conversation

afonso360
Copy link
Contributor

@afonso360 afonso360 commented Sep 2, 2023

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

@afonso360 afonso360 added the cranelift:area:riscv64 Issues related to the RISC-V 64 backend. label Sep 2, 2023
@afonso360 afonso360 requested a review from a team as a code owner September 2, 2023 11:38
@afonso360 afonso360 requested review from abrown and removed request for a team September 2, 2023 11:39
@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:meta Everything related to the meta-language. labels Sep 2, 2023
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor follow-up questions, but looks good to me 👍

Comment on lines +41 to +44
let _has_m = setting.add_bool("has_m", "has extension M?", "", true);
let _has_a = setting.add_bool("has_a", "has extension A?", "", true);
let _has_f = setting.add_bool("has_f", "has extension F?", "", true);
let _has_d = setting.add_bool("has_d", "has extension D?", "", true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool makes sense! If it helps this is the example I was thinking of that used to be in the x64 backend:

// Check for compatibility between flags and ISA level
// requested. In particular, SIMD support requires SSSE3.
if !cfg!(miri) && shared_flags.enable_simd() {
if !isa_flags.has_sse3() || !isa_flags.has_ssse3() {
return Err(CodegenError::Unsupported(
"SIMD support requires SSE3 and SSSE3 on x86_64.".into(),
));
}
}

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

@@ -1382,11 +1387,12 @@ impl MachInstEmit for Inst {
sink.bind_label(label_true, &mut state.ctrl_plane);
Inst::load_imm12(rd, Imm12::TRUE).emit(&[], sink, emit_info, state);
Inst::Jal {
dest: BranchTarget::offset(Inst::INSTRUCTION_SIZE * 2),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@afonso360 afonso360 added this pull request to the merge queue Sep 6, 2023
Merged via the queue into bytecodealliance:main with commit cad026f Sep 6, 2023
@afonso360 afonso360 deleted the riscv-use-branch-labels branch September 6, 2023 20:03
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Sep 6, 2023
…6955)

* riscv64: Use label based jump in Icmp

* riscv64: Use labels in LoadExtData

* riscv64: Use labels in BrTable

* riscv64: Delete BranchTarget::offset

* riscv64: Enable default extensions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:riscv64 Issues related to the RISC-V 64 backend. cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants