Make some remaining X86 intrinsics safe #1908
Conversation
|
The prefetch intrinsics can also be made safe. |
|
It has a pointer parameter, can it still be made safe? Also, what about TSC, CPUID and the fence instructions? |
|
These intrinsics are all safe to call as long as the required target features are available. @rfcbot merge |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
@BurntSushi @m-ou-se @the8472 small ping for the rfc checkbox |
| pub unsafe fn _mm_prefetch<const STRATEGY: i32>(p: *const i8) { | ||
| pub fn _mm_prefetch<const STRATEGY: i32>(p: *const i8) { | ||
| static_assert_uimm_bits!(STRATEGY, 3); | ||
| // We use the `llvm.prefetch` intrinsic with `cache type` = 1 (data cache). | ||
| // `locality` and `rw` are based on our `STRATEGY`. | ||
| prefetch(p, (STRATEGY >> 2) & 1, STRATEGY & 3, 1); | ||
| unsafe { | ||
| prefetch(p, (STRATEGY >> 2) & 1, STRATEGY & 3, 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Seeing a safe function that takes a raw pointer is a bit surprising. It may be good to note in the documentation that this is safe because it does not interact with the AM and does not trap on invalid pointers (per https://llvm.org/docs/LangRef.html#llvm-prefetch-intrinsic).
There was a problem hiding this comment.
@sayantn Could you add a line in the docs to explain why this is safe despite taking a raw pointer?
|
@Amanieu proposal cancelled. |
|
Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 psst @Amanieu, I wasn't able to add the |
rust-rfcbot
added
finished-final-comment-period
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
|
Needs a rebase. |
In edition 2024 functions annotated with `target_feature(enable = "..")` are unsafe to call from contexts not so annotated, and otherwise safe. This was used in rust-lang/stdarch#1908 to mark RDRAND safe, and this change is expected to be in 1.93.0 and is already in nightly. Since uefi targets are only tested on nightly, we only saw this lint on uefi, resulting in a misattribution of the behavior and an incorrect comment. Thus acknowledge that the intrinsics are safe and mark `rdrand` itself safe (when called from an annotated context) and remove all the newly unused unsafe blocks. Link: https://doc.rust-lang.org/reference/attributes/codegen.html#r-attributes.codegen.target_feature.safety-restrictions.
5 participants
_mm512_reduce_mul_ph(missed)_bswap{,64}_mm_prefetch- It doesn't actually dereference the pointer argument, so it's safe_mm_{l,s,m}fence- These can't introduce inconsistencies, as they are fences._mm_pause- It's likestd::hint::spin_loop, so nothing unsafe here. Worst it can do is make the program slower