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

compiler: add simd_ctpop intrinsic #125266

Merged
merged 4 commits into from
May 21, 2024

Conversation

workingjubilee
Copy link
Member

Fairly straightforward addition.

cc @rust-lang/opsem new (extremely boring) intrinsic

@rustbot
Copy link
Collaborator

rustbot commented May 19, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
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 May 19, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 19, 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

@workingjubilee
Copy link
Member Author

workingjubilee commented May 19, 2024

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

Yes, that's the idea.

@calebzulawski
Copy link
Member

r? @RalfJung

looks good to me

@rustbot rustbot assigned RalfJung and unassigned jieyouxu May 19, 2024
@saethlin
Copy link
Member

saethlin commented May 19, 2024

I presume it's implausible for this intrinsic to have a fallback body because we can't actually write the generic bound?

Can we then add Miri support? I feel like it shouldn't be too hard, the boilerplate is already reusable in this function:

/// Calls the simd intrinsic `intrinsic`; the `simd_` prefix has already been removed.
/// Returns `Ok(true)` if the intrinsic was handled.
fn emulate_simd_intrinsic(

@workingjubilee
Copy link
Member Author

I presume it's implausible for this intrinsic to have a fallback body because we can't actually write the generic bound?

Can we then add Miri support? I feel like it shouldn't be too hard, the boilerplate is already reusable in this function:

/// Calls the simd intrinsic `intrinsic`; the `simd_` prefix has already been removed.
/// Returns `Ok(true)` if the intrinsic was handled.
fn emulate_simd_intrinsic(

I can add Miri support yeah.

I'm not sure what adding a fallback body would look like here, it might be possible to twist the code such that it actually works, tbh.

@workingjubilee
Copy link
Member Author

@saethlin hmm so wait I looked around and didn't see a magic wand to wave... am I supposed to just, for a fallback body, literally write a body for the intrinsic?

lemme think about that, it might work.

@workingjubilee
Copy link
Member Author

hm. I know how to add the appropriate generic bounds, they just might make someone mad.

@RalfJung
Copy link
Member

Happy to review a Miri implementation, but LLVM codegen is not something I can confidently review.
r? @nikic

We haven't yet used a fallback body for any SIMD intrinsic so maybe this is not the right time and place for that experiment. :) (It could delay the PR quite a bit, trying to get that to work.)

@rustbot rustbot assigned nikic and unassigned RalfJung May 19, 2024
@bjorn3
Copy link
Member

bjorn3 commented May 19, 2024

Would you mind adding cg_clif support too at

? Cranelift calls the instruction that needs to be used here popcnt:
let res = fx.bcx.ins().popcnt(val);

@calebzulawski
Copy link
Member

hm. I know how to add the appropriate generic bounds, they just might make someone mad.

They will leak into the std::simd interface, which we don't want

@workingjubilee
Copy link
Member Author

workingjubilee commented May 19, 2024

hm. I know how to add the appropriate generic bounds, they just might make someone mad.

They will leak into the std::simd interface, which we don't want

Ehn, we already have the relevant bounds or approximately so.

As Ralf said, it would delay things significantly, that's the real reason.

Yeah, I noticed that cg_clif and cg_gcc were already parsing the LLVM intrinsic, which made me surprised no one added this yet. 👀

I'll add the followups.

@RalfJung
Copy link
Member

Miri support should be pretty easy -- just another case here that maps to Op::Numeric(sym::ctpop). This completes the support for the "numeric" intrinsics, except for the nonzero variants.

For tests I think this function would be good.

@rustbot
Copy link
Collaborator

rustbot commented May 20, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

The Miri subtree was changed

cc @rust-lang/miri

@workingjubilee
Copy link
Member Author

workingjubilee commented May 20, 2024

I briefly glanced at cg_gcc but I do not understand the relevant code for the popcount implementation, which appears to have many edge-cases, and does not seem to dispatch to the fast vpopcount builtins anyways, only doing that for pattern-matching LLVMIR intrinsics. I hope it won't be too hard to add, but my apologies.

@saethlin
Copy link
Member

Neat, thanks for the Miri implementation.

@RalfJung
Copy link
Member

r=me on the Miri implementation and test, thanks. :)

let fn_ty = bx.type_func(&[vec_ty, bx.type_i1()], vec_ty);
let dont_poison_on_zero = bx.const_int(bx.type_i1(), 0);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be called poison_on_zero, which we are then setting to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

hmmm, plausible. I was going for a "nah nah" instead of a "yeah nah".

@nikic
Copy link
Contributor

nikic commented May 21, 2024

@bors r=RalfJung,nikic

@bors
Copy link
Contributor

bors commented May 21, 2024

📌 Commit 213351a has been approved by RalfJung,nikic

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#124570 (Miscellaneous cleanups)
 - rust-lang#124772 (Refactor documentation for Apple targets)
 - rust-lang#125011 (Add opt-for-size core lib feature flag)
 - rust-lang#125218 (Migrate `run-make/no-intermediate-extras` to new `rmake.rs`)
 - rust-lang#125225 (Use functions from `crt_externs.h` on iOS/tvOS/watchOS/visionOS)
 - rust-lang#125266 (compiler: add simd_ctpop intrinsic)
 - rust-lang#125348 (Small fixes to `std::path::absolute` docs)

Failed merges:

 - rust-lang#125296 (Fix `unexpected_cfgs` lint on std)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fd975f7 into rust-lang:master May 21, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone May 21, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request May 21, 2024
Rollup merge of rust-lang#125266 - workingjubilee:stream-plastic-love, r=RalfJung,nikic

compiler: add simd_ctpop intrinsic

Fairly straightforward addition.

cc `@rust-lang/opsem` new (extremely boring) intrinsic
@workingjubilee workingjubilee deleted the stream-plastic-love branch May 21, 2024 19:53
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jun 30, 2024
…, r=RalfJung,nikic

compiler: add simd_ctpop intrinsic

Fairly straightforward addition.

cc `@rust-lang/opsem` new (extremely boring) intrinsic
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-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.

9 participants