Skip to content

Make RISC-V bit manipulation intrinsics match C API - #2247

Open
TechnoPorg wants to merge 2 commits into
rust-lang:mainfrom
TechnoPorg:push-oltzmvxwsyno
Open

TechnoPorg wants to merge 2 commits into
rust-lang:mainfrom
TechnoPorg:push-oltzmvxwsyno

Conversation

@TechnoPorg

@TechnoPorg TechnoPorg commented Oct 8, 2026 •

Copy link
Copy Markdown

As per rust-lang/rust#114544 and the recent libs team decision (https://hackmd.io/@rust-libs/SJFLHWcozl), this switches the RISC-V bit manipulation intrinsics to use u32/u64 instead of usize, as this aligns better with how they're actually applied.

I've also opted to move all the implemented bit manipulation intrinsics into zb.rs, since they happen to be pulled in under the Zkn umbrella (https://docs.riscv.org/reference/isa/v20260120/unpriv/scalar-crypto.html, https://docs.riscv.org/reference/isa/v20260120/unpriv/b-st-ext.html) but aren't "cryptography" intrinsics in the same way.

I have not touched any of the P extension, but I can if desired.

cc @tgross35

@rustbot

rustbot commented Oct 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the pull request, and welcome! The Rust Project has assigned @davidtwco (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: @Amanieu, @adamgemmell, @davidtwco, @folkertdev, @sayantn
  • @Amanieu, @adamgemmell, @davidtwco, @folkertdev, @sayantn expanded to Amanieu, adamgemmell, davidtwco, folkertdev, sayantn
  • Random selection from Amanieu, adamgemmell, davidtwco, folkertdev, sayantn

@tgross35

tgross35 commented Oct 9, 2026

Copy link
Copy Markdown
Member

@rustbot label +I-libs-nominated

I think this is the direction that we've heard more favor towards, but the meeting should confirm this is okay since there was a bit of back and forth and we haven't had a ton of feedback. This also serves as a heads up that it will probably be headed for stabilization in the near future, in case there's anything to discuss there.

For other opinions, pinging maintainers @kito-cheng @michaelmaitland @robin-randhawa-sifive @topperc, the question is whether these should take fixed-size integers or usize. See rust-lang/rust#114544 for a bit more context.

@rustbot rustbot added the I-libs-nominated Nominated for discussion in a libs team meeting. label Oct 9, 2026
@topperc

topperc commented Oct 9, 2026

Copy link
Copy Markdown

The bitmanip C intrinsic interface in clang and gcc has uint32_t intrinsics for RV32 and RV64 and uint64_t intrinsics for RV64. This allows some source code compatibility between RV32 and RV64. I also assumed that bit manipulation code would be written with a specific number of bits in mind.

Looking at this patch, it looks like u32 is RV32 only and u64 is RV64 only. Is that right?

@TechnoPorg

Copy link
Copy Markdown
Author

Looking at this patch, it looks like u32 is RV32 only and u64 is RV64 only. Is that right?

This matches the code that was there before, but I'll look to see whether LLVM exposes any intrinsics that let us mix and match.

@tgross35

tgross35 commented Oct 9, 2026

Copy link
Copy Markdown
Member

If RV64 supports both then that actually sounds like a nicer option. Where is this defined, though? https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc#scalar-bit-manipulation-extension-intrinsics for example lists both __riscv_clmulh_32 and __riscv_clmulh_64, but says there's no emulation of the 32-bit version on RV64.

@topperc

topperc commented Oct 9, 2026

Copy link
Copy Markdown

If RV64 supports both then that actually sounds like a nicer option. Where is this defined, though? https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc#scalar-bit-manipulation-extension-intrinsics for example lists both __riscv_clmulh_32 and __riscv_clmulh_64, but says there's no emulation of the 32-bit version on RV64.

You're right we're not completely consistent about it. I think clmulh.i32 and clmulr.i32 do work on RV64 in the LLVM backend they just didn't get exposed to frontend.

__riscv_clz_32, __riscv_ctz_32, __riscv_cpop_32, __riscv_orc_b_32, __riscv_ror_32, __riscv_rol_32, __riscv_rev8_32, __riscv_brev8_32, __riscv_clmul_32 should all work for RV64.

__riscv_zip_32 and __riscv_unzip_32 aren't on RV64 because the underlying zip/unzip instructions are RV32 only.

__riscv_clmulh_32, __riscv_clmulr_32 aren't supported on RV64. It is doable, but requires 3-4 instructions.

__riscv_xperm4_32, __riscv_xperm8_32 aren't supported on RV64 because we'd need additional instructions to modify the indices to emulate the out of bounds indices being 0 behavior. As I write this, I guess we just need to zero extend rs1? Maybe it's not so bad.

@tgross35

Copy link
Copy Markdown
Member

Thanks for bringing that up, I think that may be a good option for us as well.

I'm going to bring this up at rust-lang/rust#114544 and move the nomination label there since some other naming discussion has already happened there.

@tgross35 tgross35 removed the I-libs-nominated Nominated for discussion in a libs team meeting. label Oct 11, 2026
Comment thread crates/core_arch/src/riscv32/zb.rs Outdated
Comment thread crates/core_arch/src/riscv32/zb.rs Outdated
@tgross35

Copy link
Copy Markdown
Member

Would it be possible create zb.rs and move everything relevant there in one commit, then do name and signature updates in a second commit? Currently there's some back and forth, e.g. 964ad8c does both of those things which makes it tough to see the API changes.

@tgross35 tgross35 assigned tgross35 and unassigned davidtwco Oct 19, 2026
@tgross35

Copy link
Copy Markdown
Member

I have not touched any of the P extension, but I can if desired.

Not here, but all of our risc-v API could use a review that it matches https://github.com/riscv-non-isa/riscv-c-api-doc/blob/main/src/c-api.adoc. So a separate PR updating those would be welcome once this merges.

@TechnoPorg

TechnoPorg commented Oct 20, 2026 •

Copy link
Copy Markdown
Author

@tgross35 any idea what's going on with the libc SYS_futex and Intel ICX errors in CI? The only thing that changed between force pushes was some doc stuff and the order of commits, so it feels unrelated.

@TechnoPorg TechnoPorg changed the title Make RISC-V bit manip intrinsics use u32/u64 instead of usize Make RISC-V bit manipulation intrinsics match C API Oct 20, 2026
@tgross35

Copy link
Copy Markdown
Member

It's a T3 platform that had some syscall changes rust-lang/libc@181ab01, std will need to be updated. Also those platforms need some maintainers...

This branch has not been deployed

No deployments
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.

6 participants