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

Implement RISC-V Zk intrinsics #1453

Merged
merged 12 commits into from
Aug 31, 2023

Conversation

coastalwhite
Copy link
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
Copy link
Collaborator

rustbot commented Aug 6, 2023

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.

crates/core_arch/src/riscv64/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
@coastalwhite
Copy link
Contributor Author

coastalwhite commented Aug 22, 2023

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.

crates/core_arch/src/riscv64/zk.rs Outdated Show resolved Hide resolved
crates/core_arch/src/riscv_shared/zk.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Aug 29, 2023

CI should be fixed now, can you rebase?

@coastalwhite
Copy link
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
Copy link
Member

Amanieu commented Aug 29, 2023

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
Copy link
Member

Amanieu commented Aug 29, 2023

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
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
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
Copy link
Member

Amanieu commented Aug 31, 2023

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
26 checks passed
@coastalwhite coastalwhite deleted the riscv-zk-ext-intrinsics branch September 16, 2023 14:57
@sayantn
Copy link
Contributor

sayantn commented May 16, 2024

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
Contributor Author

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

@sayantn
Copy link
Contributor

sayantn commented May 16, 2024

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
Copy link
Contributor

sayantn commented May 17, 2024

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
Copy link
Member

Amanieu commented May 17, 2024

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