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: deprecate intrinsics that cannot be used from Rust #1478

Merged
merged 1 commit into from
Nov 5, 2023

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Oct 9, 2023

This is the RISC-V version of #1454.

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2023

r? @Amanieu

(rustbot has picked a reviewer for you, use r? to override)

@Amanieu
Copy link
Member

Amanieu commented Oct 10, 2023

Since these are still unstable, do we want to just delete them instead of deprecating them?

@RalfJung
Copy link
Member Author

That would be fine for me, we might want to consult with some RISC-V stakeholders. Cc @coastalwhite (not sure whom else to ping)

Also, we should have some place to document why they are missing.

@coastalwhite
Copy link
Contributor

I think these instructions can be deleted. The riscv crate which is maintained by the Rust Embedded people also has them, and they just implement the assembly themselves. That crate is kind of standard if you are using these kind of instructions and don't want to touch assembly yourself. I don't really see the point of having these intrinsics in the standard library, especially if they are causing trouble.

This also kind of goes for pause, wfi and fence.i (which btw only exists if the Zfencei extension is present), etc. All of these instructions are available from assembly, and adding them here instead of using the riscv crate does not really create too much value. Maybe we should ping @luojia65 who originally implemented them to see why you might need them in stdarch.

@Amanieu is way more knowledgable about what is proper for stdarch, but I would say remove the Floating-Point CSR instructions.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 10, 2023 via email

@bjorn3
Copy link
Member

bjorn3 commented Oct 10, 2023

In the context of a kernel you have to set the rounding mode during startup to the default value if the boot protocol doesn't guarantee that it has this value and when switching tasks you have to restore the right floating point configuration for the task you are switching too. Kernels are generally compiled with usage of floating point instructions disabled, so there is no way for the compiler to observe that the rounding mode changed, rather it is effectively just a general purpose register which the compiler is not allowed to clobber, and thus there shouldn't be any UB.

@RalfJung
Copy link
Member Author

Sure there's always some super special situations that are exceptions. Technically this needs guarantees we do not provide (rustc will not synthesize float operations if you don't use any float operations), but practically this will work.

However I assume mostly people using these intrinsics are not currently aware that they have to be in such a super special case or else what they are doing is wrong.

@bjorn3
Copy link
Member

bjorn3 commented Oct 11, 2023

Technically this needs guarantees we do not provide (rustc will not synthesize float operations if you don't use any float operations), but practically this will work.

If you enable the soft float abi and disable the floating point instruction target features (as you would for a kernel) then having the compiler emit float instructions would be a miscompilation that can cause crashes or worse.

@RalfJung
Copy link
Member Author

Sure, if the feature is entirely disabled we do promise not to use those instructions.

@bors
Copy link
Contributor

bors commented Oct 29, 2023

☔ The latest upstream changes (presumably 68b3ddf) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 4, 2023

Since these are still unstable, do we want to just delete them instead of deprecating them?

I did that now.

@Amanieu Amanieu merged commit 39e3779 into rust-lang:master Nov 5, 2023
26 checks passed
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