Skip to content

Fix safety doc in intrinsics::simd#159322

Open
yilin0518 wants to merge 1 commit into
rust-lang:mainfrom
yilin0518:fix_simd_1
Open

Fix safety doc in intrinsics::simd#159322
yilin0518 wants to merge 1 commit into
rust-lang:mainfrom
yilin0518:fix_simd_1

Conversation

@yilin0518

Copy link
Copy Markdown
Contributor

The Safety documentation for simd_gather and simd_scatter incorrectly described unmasked values in T as requiring readable or writable memory.

The memory access is performed through the corresponding enabled pointer lane in ptr, so this updates the documentation to state that requirement explicitly.

@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

Some changes occurred to the platform-builtins intrinsics. Make sure the
LLVM backend as well as portable-simd gets adapted for the changes.

cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 15, 2026
@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: libs
  • libs expanded to 12 candidates
  • Random selection from 6 candidates

@rust-log-analyzer

This comment has been minimized.

@RalfJung

RalfJung commented Jul 15, 2026 via email

Copy link
Copy Markdown
Member

@RalfJung

RalfJung commented Jul 15, 2026

Copy link
Copy Markdown
Member

Looking at the diff, I think the old and new wording are equivalent. An "unmasked value" is likely meant to be interpreted as a value in a lane that has not been "masked out", i.e. in an enabled lane. But I agree that the old wording is confusing.

So r=me on the change, but CI failed because the doc link is not right.
@rustbot author
r? me

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2026
@rustbot

rustbot commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot assigned RalfJung and unassigned JohnTitor Jul 15, 2026
@yilin0518

yilin0518 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Is there LLVM documentation confirming the exact requirements here?

I found the possible corresponding documentation: llvm-masked-gather-intrinsics and llvm-masked-scatter-intrinsics, in which it emphasizes that ptr should be readable/writable if the value in mask is !0.

@yilin0518

yilin0518 commented Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

@RalfJung I commit the fixed commit, it should have solved the link problem.

Besides, I open a topic in Zulip: Maybe some safety doc need fixs in intrinsics::simd. In this topic I refered another functions: simd_masked_load and simd_masked_store. I think the safety section of these function should add an extra entry: Enabled ptr elements must be safe to read/write as if by core::ptr::read/write. Can you provide any suggestion? Thank you!

Also, I think simd_funnel_shl, simd_funnel_shr, simd_shl, simd_shr need to be modified: change less than <int>::BITS to in 0..<int>::BITS. If really need, I can provide a new PR to fix these functions.

Comment thread library/core/src/intrinsics/simd/mod.rs Outdated
/// Unmasked values in `T` must be readable as if by `<ptr>::read` (e.g. aligned to the element
/// type).
/// Each pointer in `ptr` whose corresponding value in `mask` is `!0` must be readable as if by
/// [`crate::ptr::read`] (e.g. aligned to the element type).

@RalfJung RalfJung Jul 15, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the crate:: will show up in the rendered text, that's not great.
Something like [`ptr::read`][crate::ptr::read] should work.

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You dud not copy what I suggested and did not fix the problem I think.

@yilin0518 yilin0518 Jul 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, thank you for your remind. I think I do the right thing this time.😭

@RalfJung

Copy link
Copy Markdown
Member

Also note that this is still marked as waiting-for-author. As the bot says, please say @rustbot ready if you want another review. Sometimes reviewers will guess that the PR is ready for review but that doesn't always work.

@yilin0518

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2026
@RalfJung

Copy link
Copy Markdown
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 15, 2026
@yilin0518

Copy link
Copy Markdown
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants