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

Correct the simd_masked_{load,store} intrinsic docs #119203

Merged
merged 1 commit into from
Feb 21, 2024

Conversation

farnoy
Copy link
Contributor

@farnoy farnoy commented Dec 21, 2023

Explains the uniform pointer being used for these two operations and how elements are offset from it.

@rustbot
Copy link
Collaborator

rustbot commented Dec 21, 2023

r? @thomcc

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

@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 Dec 21, 2023
@@ -243,12 +243,13 @@ extern "platform-intrinsic" {
///
/// `T` must be a vector.
///
/// `U` must be a vector of pointers to the element type of `T`, with the same length as `T`.
/// `U` must be a pointer to the element type of `T`
Copy link
Member

Choose a reason for hiding this comment

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

Why not make the argument of type *const U then? It's not necessary to make it so generic.

Same for the store operation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good idea. I'll explore this

Copy link
Member

Choose a reason for hiding this comment

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

So this is left to a future PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we'll need to change how we validate these monomorphizations and there will be an opportunity when we move these checks out of the codegen crate.

@RalfJung
Copy link
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 Dec 28, 2023
@farnoy
Copy link
Contributor Author

farnoy commented Jan 9, 2024

@rustbot ready

I will squash the commits when it's approved

@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 Jan 9, 2024
@RalfJung
Copy link
Member

RalfJung commented Jan 9, 2024

LGTM, r=me after squashing.

@bors delegate+

@bors
Copy link
Contributor

bors commented Jan 9, 2024

✌️ @farnoy, you can now approve this pull request!

If @RalfJung told you to "r=me" after making some further change, please make that change, then do @bors r=@RalfJung

@thomcc thomcc 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 Jan 22, 2024
@thomcc
Copy link
Member

thomcc commented Feb 1, 2024

I'm going to be away for a few months, so I'm rerolling my PRs so that folks don't have to wait for me. Sorry/thanks.

r? libs

@rustbot rustbot assigned Mark-Simulacrum and unassigned thomcc Feb 1, 2024
@RalfJung
Copy link
Member

RalfJung commented Feb 2, 2024

r? @RalfJung

@farnoy this is just waiting for you to squash the commits. :)

@rustbot rustbot assigned RalfJung and unassigned Mark-Simulacrum Feb 2, 2024
@farnoy farnoy force-pushed the simd-masked-intrinsic-docfix branch from 77c3d65 to 14a4551 Compare February 20, 2024 16:05
@rustbot
Copy link
Collaborator

rustbot commented Feb 20, 2024

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

@farnoy
Copy link
Contributor Author

farnoy commented Feb 20, 2024

Missed the notifications.

@bors r=@RalfJung

@bors
Copy link
Contributor

bors commented Feb 20, 2024

📌 Commit 14a4551 has been approved by RalfJung

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 20, 2024
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 20, 2024
@saethlin
Copy link
Member

@bors rollup=always (docs-only change)

///
/// `V` must be a vector of integers with the same length as `T` (but any element size).
///
/// For each element, if the corresponding value in `mask` is `!0`, read the corresponding
/// pointer from `ptr`.
/// pointer offset from `ptr`.
/// The first element is loaded from `ptr`, the second from `ptr.wrapping_offset(1)` and so on.
Copy link
Member

Choose a reason for hiding this comment

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

i'd expect it to use offset, not wrapping_offset

Copy link
Member

Choose a reason for hiding this comment

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

The entire point of this is to be able to mask off loads that would be out-of-bounds. If it used offset, that would defeat the purpose.

Copy link
Member

Choose a reason for hiding this comment

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

The entire point of this is to be able to mask off loads that would be out-of-bounds. If it used offset, that would defeat the purpose.

no, because offset is only called for those indexes that are not masked off -- sort-of. LLVM IR semantics are quite lacking here:

The ‘llvm.masked.load’ intrinsic is designed for conditional reading of selected vector elements in a single IR operation. It is useful for targets that support vector masked loads and allows vectorizing predicated basic blocks on these targets. Other targets may support this intrinsic differently, for example by lowering it into a sequence of branches that guard scalar load operations. The result of this operation is equivalent to a regular vector load instruction followed by a ‘select’ between the loaded and the passthru values, predicated on the same mask. However, using this intrinsic prevents exceptions on memory access to masked-off lanes.

Copy link
Member

Choose a reason for hiding this comment

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

no, because offset is only called for those indexes that are not masked off

Even then I couldn't use this to do loads where the first elements are out of bounds.

The LLVM docs are unfortunately unclear on whether the pointer arithmetic has "inbounds" semantics or not, but the note about suppressing exceptions seems to indicate that out-of-bounds should be allowed.

Copy link
Member

Choose a reason for hiding this comment

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

I'm proposing to clarify the LangRef: llvm/llvm-project#82469.

bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 20, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119203 (Correct the simd_masked_{load,store} intrinsic docs)
 - rust-lang#121277 (Refactor trait implementations in `core::convert::num`.)
 - rust-lang#121322 (Don't ICE when hitting overflow limit in fulfillment loop in next solver)
 - rust-lang#121323 (Don't use raw parameter types in `find_builder_fn`)
 - rust-lang#121344 (Expand weak alias types before collecting constrained/referenced late bound regions + refactorings)
 - rust-lang#121350 (Fix stray trait mismatch in `resolve_associated_item` for `AsyncFn`)
 - rust-lang#121352 (docs: add missing "the" to `str::strip_prefix` doc)

Failed merges:

 - rust-lang#121340 (bootstrap: apply most of clippy's suggestions)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fc5f6f8 into rust-lang:master Feb 21, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 21, 2024
Rollup merge of rust-lang#119203 - farnoy:simd-masked-intrinsic-docfix, r=RalfJung

Correct the simd_masked_{load,store} intrinsic docs

Explains the uniform pointer being used for these two operations and how elements are offset from it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

9 participants