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

riscv: K extension (part 1), floating-point control and state register #1278

Merged
merged 5 commits into from
Feb 6, 2022

Conversation

luojia65
Copy link
Contributor

@luojia65 luojia65 commented Jan 30, 2022

This pull request includes:

  • Zksh, Zksed extension in RISC-V K extension. There are four functions: sm3p0, sm3p1, sm4ed and sm4ks function.
  • Floating point control and state register operations. They are three read instructions frcsr, frrm, frflags, and three swap instructions fscsr, fsrm, fsflags as is defined in the RISC-V unprivileged specification.
  • Typo fixes in previous documents (thanks @dramforever).

The features above are all defined as safe Rust functions. Four K extension functions operates like arithmetic functions and does not visit on any memory addresses. The floating point control operations only reads or writes to floating point control configurations of the current hart.

Note: choose of type i32 in sm4ks<const BS: i32>(x: u32, k: u32) -> u32 is required by an internal macro statis_assert_imm2. Fixed: uses BS: u8 and static_assert! internal macro.

r? @Amanieu

@Amanieu
Copy link
Member

Amanieu commented Feb 5, 2022

You don't need to restrict yourself to i32 for the immediate. This was only done for ARM/x86 to match the C intrinsics. For example the Wasm intrinsics uses usize with static_assert!.

Use `static_assert!` macro to constrain BS: u8

Thanks @Amanieu
@luojia65
Copy link
Contributor Author

luojia65 commented Feb 6, 2022

Thank you for your advice @Amanieu! I fixed with static_assert! macro in newer commits.

@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2022

For the crypto intrinsics we should try to use LLVM intrinsics instead of inline assembly since it allows LLVM to schedule the instructions for better performance. These are supported in LLVM 14 which we should be upgrading to soon (rust-lang/rust#93577).

It's fine to use inline assembly for now though.

@Amanieu Amanieu merged commit 597b8e8 into rust-lang:master Feb 6, 2022
@luojia65
Copy link
Contributor Author

luojia65 commented Feb 6, 2022

Yes, thanks for mentioning, these lines of intrinsics should be changed into LLVM intrinsics in newer versions of stdarch.

@luojia65 luojia65 deleted the riscv-crypto-1 branch February 6, 2022 13:24
@pthariensflame
Copy link

Are the floating-point CSR intrinsics okay to have? AArch32 lacks them because of some worry about violating LLVM’s assumptions.

@luojia65
Copy link
Contributor Author

luojia65 commented Feb 6, 2022

@pthariensflame It might be useful in developing precise mathematical analysis software, where rounding modes would be necessary to decrease errors on strict floating point calculations. It provides a way to manually set or read the rounding mode and error flags under nightly; however if it resulted in a non-compliance to higher programming language assumptions under LLVM or other compiler framework, I suggest it would be designed in other ways (external crates instead, etc.) to achieve the above purpose like switching rounding modes, instead of building it into core::arch module (in this case these functions would be removed, if someone proved this way right).

@Amanieu
Copy link
Member

Amanieu commented Feb 6, 2022

The problem is that currently LLVM assumes that the rounding mode is always the default unless you use special intrinsics for floating-point operations (which rustc doesn't currently expose).

I think it's fine to leave them in for now, but we should review this before stabilization. See #781 for a similar discussion on x86.

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.

3 participants