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

simd intrinsics with mask: accept unsigned integer masks, and fix some of the errors #137953

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Mar 3, 2025

It's not clear at all why the mask would have to be signed, it is anyway interpreted bitwise. The backend should just make sure that works no matter the surface-level type; our LLVM backend already does this correctly. The note of "the mask may be widened, which only has the correct behavior for signed integers" explains... nothing? Why can't the code do the widening correctly? If necessary, just cast to the signed type first...

Also while we are at it, fix the errors. For simd_masked_load/store, the errors talked about the "third argument" but they meant the first argument (the mask is the first argument there). They also used the wrong type for expected_element.

I have extremely low confidence in the GCC part of this PR.

See discussion on Zulip

@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2025

r? @Nadrieril

rustbot has assigned @Nadrieril.
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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Mar 3, 2025

The Miri subtree was changed

cc @rust-lang/miri

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

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

@rust-log-analyzer

This comment has been minimized.

@RalfJung RalfJung force-pushed the simd-intrinsic-masks branch from 48bc564 to 26cb001 Compare March 3, 2025 18:00
@RalfJung
Copy link
Member Author

RalfJung commented Mar 3, 2025

@bjorn3 anything we have to do here in cranelift?

@RalfJung RalfJung force-pushed the simd-intrinsic-masks branch from 26cb001 to fa396bc Compare March 4, 2025 07:32
@Nadrieril
Copy link
Member

r? codegen

@rustbot rustbot assigned workingjubilee and unassigned Nadrieril Mar 5, 2025
@RalfJung RalfJung force-pushed the simd-intrinsic-masks branch from fa396bc to edadfc4 Compare March 19, 2025 19:24
@rustbot
Copy link
Collaborator

rustbot commented Mar 19, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

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-compiler Relevant to the compiler team, which will review and decide on the PR/issue. 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.

None yet

7 participants