Skip to content

Implement RISC-V Zk intrinsics - #1453

Merged
Amanieu merged 12 commits into
rust-lang:masterfrom
coastalwhite:riscv-zk-ext-intrinsics
Aug 31, 2023
Merged

Implement RISC-V Zk intrinsics#1453
Amanieu merged 12 commits into
rust-lang:masterfrom
coastalwhite:riscv-zk-ext-intrinsics

Conversation

@coastalwhite

Copy link
Copy Markdown
Contributor

This is the first PR for rust-lang/rust#114544.

This PR changes the core::arch::{riscv32, riscv64} to be unstable under the new riscv_ext_intrinsics feature-gate and adds all the instructions for the zk extension except for the Entropy Source and the Zkt extensions. For these, it is not yet known how to deal with them.

@rustbot

rustbot commented Aug 6, 2023

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Amanieu (or someone else) soon.

Comment thread crates/core_arch/src/riscv64/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
@coastalwhite

coastalwhite commented Aug 22, 2023

Copy link
Copy Markdown
Contributor Author

I have replaced most implementations with

  1. rustc_legacy_const_generics
  2. LLVM intrinsics where possible

I have also removed the pack and brev8 intrinsics.

@coastalwhite
coastalwhite requested a review from Amanieu August 22, 2023 11:52
Comment thread crates/core_arch/src/riscv64/zk.rs Outdated
Comment thread crates/core_arch/src/riscv_shared/zk.rs Outdated
@coastalwhite
coastalwhite requested a review from Amanieu August 25, 2023 17:04
@Amanieu

Amanieu commented Aug 29, 2023

Copy link
Copy Markdown
Member

CI should be fixed now, can you rebase?

@coastalwhite
coastalwhite force-pushed the riscv-zk-ext-intrinsics branch from b274247 to 7cf295a Compare August 29, 2023 17:02
@coastalwhite

Copy link
Copy Markdown
Contributor Author

The current version of the docker Ubuntu image only supports the package qemu-user with version 6.2. This version does not yet support the zk extension. The newer version does work with this extension. Another problem is that the ShangMi instructions don't seem to be supported by riscv64-unknown-linux-gnu yet.

What do we do here? Do we wait for the Ubuntu upstream to update? I am going to take a look into the other extensions.

@Amanieu

Amanieu commented Aug 29, 2023

Copy link
Copy Markdown
Member

The disassembler used in CI is old and doesn't recognize the opcodes. You can try upgrading the CI image to see if this fixes it. Otherwise you can just comment out the assert_instr tests, we will re-enable them when we upgrade the CI image.

@Amanieu

Amanieu commented Aug 29, 2023

Copy link
Copy Markdown
Member

The docker image uses 22.04, try upgrading to 23.04: https://github.com/rust-lang/stdarch/blob/master/ci/docker/riscv64gc-unknown-linux-gnu/Dockerfile

@coastalwhite

Copy link
Copy Markdown
Contributor Author

I tried 23.04 and 23.10. Both are not new enough sadly. I will comment out the assert_instr and open an issue to track the CI being updated.

@coastalwhite

Copy link
Copy Markdown
Contributor Author

I think things should be all good now. If you are okay with everything, you can merge it.

What do you think about the pollentropy instruction (/CSR) that is part of the Zkr extension? LLVM does not have an intrinsic for it and the returned data is divided into several bitfields. Adding it would mean including inline assembly again. Should I do that? If so, I would prefer to do that in a separate PR.

@Amanieu

Amanieu commented Aug 31, 2023

Copy link
Copy Markdown
Member

I don't think that should be exposed as an intrinsic. If people really need it then they can just use inline assembly.

@Amanieu
Amanieu merged commit f17c175 into rust-lang:master Aug 31, 2023
@coastalwhite
coastalwhite deleted the riscv-zk-ext-intrinsics branch September 16, 2023 14:57
@sayantn

sayantn commented May 16, 2024

Copy link
Copy Markdown
Contributor

I stumbled onto an issue today. rustc is failing to inline these intrinsic (intrinsics should be completely inlined). Seeing godbolt outputs, seems like rustc cannot ensure that the instruction (aes64esm, for example) is present even when there is the target-feature for that (zkne in this case)

@coastalwhite

Copy link
Copy Markdown
Contributor Author

It seems like it does inline if you remove the #[target_feature(enable = "zkne")].

@sayantn

sayantn commented May 16, 2024

Copy link
Copy Markdown
Contributor

That's where I got the idea that rustc is not able to confirm if the feature actually is supported or not. This kind of behavior is also seen if you use an intrinsic, but did not have a target_feature for the corresponding feature

@sayantn

sayantn commented May 17, 2024

Copy link
Copy Markdown
Contributor

I found some info on these LLVM intrinsics. Seems like LLVM doesn't have pattern matching for zkne and zknd yet, so it cannot infer (probably) if aes64esm instruction exists just by looking at target_features

@Amanieu

Amanieu commented May 17, 2024

Copy link
Copy Markdown
Member

The problem is caused by LLVM's inlining logic: it refuses to inline a function that has different target features. Basically this boils down to RISC-V not having a custom implementation of areInlineCompatible in LLVM which other targets like ARM, AArch64 and X86 have. Those custom implementation allow inlining as long as the target features of the callee are a subset of those in the caller.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants