Skip to content

rustc: Add an unstable simd_select_bitmask intrinsic #56789

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Dec 16, 2018

Conversation

alexcrichton
Copy link
Member

This is going to be required for binding a number of AVX-512 intrinsics
in the stdsimd repository, and this intrinsic is the same as
simd_select except that it takes a bitmask as the first argument
instead of a SIMD vector. This bitmask is then transmuted into a <NN x i8> argument, depending on how many bits it is.

cc rust-lang/stdarch#310

@rust-highfive
Copy link
Contributor

r? @michaelwoerister

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2018
Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

Two nits, but otherwise LGTM

let in_ty = arg_tys[0];
let m_len = match in_ty.sty {
ty::Int(i) => i.bit_width().unwrap(),
ty::Uint(i) => i.bit_width().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will crash with usize/isize? Probably not a big issue but could be avoided by computing the layout.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will indeed! That was semi-intentional, but I'll leave a comment in case a future reader is curious

Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to properly error here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm perhaps yeah, but this is all internal anyway so I don't think we really need to go too far out of our way...

@alexcrichton
Copy link
Member Author

@bors: r=rkruppe

@bors
Copy link
Collaborator

bors commented Dec 13, 2018

📌 Commit 5087aef has been approved by rkruppe

@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 Dec 13, 2018
Copy link
Contributor

@gnzlbg gnzlbg left a comment

Choose a reason for hiding this comment

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

LGTM.

//~^ ERROR `f32` is not an integral type

simd_select_bitmask("x", x, x);
//~^ ERROR `&str` is not an integral type
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to add a test for when the vector is not a SIMD type (probably for simd_select as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think #[repr(simd)] is gating that, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think @gnzlbg means the case when the second/third argument is not a repr(simd) type. That would exercise the require_simd! check in the intrinsic implementation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah indeed, tests added now!

let in_ty = arg_tys[0];
let m_len = match in_ty.sty {
ty::Int(i) => i.bit_width().unwrap(),
ty::Uint(i) => i.bit_width().unwrap(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to properly error here?

kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
…=rkruppe

rustc: Add an unstable `simd_select_bitmask` intrinsic

This is going to be required for binding a number of AVX-512 intrinsics
in the `stdsimd` repository, and this intrinsic is the same as
`simd_select` except that it takes a bitmask as the first argument
instead of a SIMD vector. This bitmask is then transmuted into a `<NN x
i8>` argument, depending on how many bits it is.

cc rust-lang/stdarch#310
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
…=rkruppe

rustc: Add an unstable `simd_select_bitmask` intrinsic

This is going to be required for binding a number of AVX-512 intrinsics
in the `stdsimd` repository, and this intrinsic is the same as
`simd_select` except that it takes a bitmask as the first argument
instead of a SIMD vector. This bitmask is then transmuted into a `<NN x
i8>` argument, depending on how many bits it is.

cc rust-lang/stdarch#310
bors added a commit that referenced this pull request Dec 14, 2018
Rollup of 14 pull requests (first batch)

Successful merges:

 - #56562 (Update libc version required by rustc)
 - #56609 (Unconditionally emit the target-cpu LLVM attribute.)
 - #56637 (rustdoc: Fix local reexports of proc macros)
 - #56658 (Add non-panicking `maybe_new_parser_from_file` variant)
 - #56695 (Fix irrefutable matches on integer ranges)
 - #56699 (Use a `newtype_index!` within `Symbol`.)
 - #56702 ([self-profiler] Add column for percent of total time)
 - #56708 (Remove some unnecessary feature gates)
 - #56709 (Remove unneeded extra chars to reduce search-index size)
 - #56744 (specialize: remove Boxes used by Children::insert)
 - #56748 (Update panic message to be clearer about env-vars)
 - #56749 (x86: Add the `adx` target feature to whitelist)
 - #56756 (Disable btree pretty-printers on older gdbs)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)

r? @ghost
@alexcrichton
Copy link
Member Author

@bors: r=rkruppe

@bors
Copy link
Collaborator

bors commented Dec 14, 2018

📌 Commit c6b645c76d3930461a73b3fd96de514099e19051 has been approved by rkruppe

@alexcrichton
Copy link
Member Author

@bors: r=rkruppe

@bors
Copy link
Collaborator

bors commented Dec 14, 2018

📌 Commit bb31d607d4154c0f9ff2c2e5877ba567cf962a6c has been approved by rkruppe

@bors
Copy link
Collaborator

bors commented Dec 14, 2018

☔ The latest upstream changes (presumably #56818) made this pull request unmergeable. Please resolve the merge conflicts.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 14, 2018
This is going to be required for binding a number of AVX-512 intrinsics
in the `stdsimd` repository, and this intrinsic is the same as
`simd_select` except that it takes a bitmask as the first argument
instead of a SIMD vector. This bitmask is then transmuted into a `<NN x
i8>` argument, depending on how many bits it is.

cc rust-lang/stdarch#310
@alexcrichton
Copy link
Member Author

@bors: r=rkruppe

Part of this landed in #56818 but I'd force pushed since this was rolled up, so this now includes just changes since the rollup

@bors
Copy link
Collaborator

bors commented Dec 14, 2018

📌 Commit ceee7f3 has been approved by rkruppe

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 14, 2018
@alexcrichton
Copy link
Member Author

@bors: rollup

Centril added a commit to Centril/rust that referenced this pull request Dec 16, 2018
…=rkruppe

rustc: Add an unstable `simd_select_bitmask` intrinsic

This is going to be required for binding a number of AVX-512 intrinsics
in the `stdsimd` repository, and this intrinsic is the same as
`simd_select` except that it takes a bitmask as the first argument
instead of a SIMD vector. This bitmask is then transmuted into a `<NN x
i8>` argument, depending on how many bits it is.

cc rust-lang/stdarch#310
bors added a commit that referenced this pull request Dec 16, 2018
Rollup of 20 pull requests

Successful merges:

 - #53506 (Documentation for impl From for AtomicBool and other Atomic types)
 - #56343 (Remove not used mod)
 - #56439 (Clearer error message for dead assign)
 - #56640 (Add FreeBSD unsigned char platforms to std::os::raw)
 - #56648 (Fix BTreeMap UB)
 - #56672 (Document time of back operations of a Linked List)
 - #56706 (Make `const unsafe fn` bodies `unsafe`)
 - #56742 (infer: remove Box from a returned Iterator)
 - #56761 (Suggest using `.display()` when trying to print a `Path`)
 - #56781 (Update LLVM submodule)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)
 - #56790 (Make RValue::Discriminant a normal Shallow read)
 - #56793 (rustdoc: look for comments when scraping attributes/crates from doctests)
 - #56826 (rustc: Add the `cmpxchg16b` target feature on x86/x86_64)
 - #56832 (std: Use `rustc_demangle` from crates.io)
 - #56844 (Improve CSS rule)
 - #56850 (Fixed issue with using `Self` ctor in typedefs)
 - #56855 (Remove u8 cttz hack)
 - #56857 (Fix a small mistake regarding NaNs in a deprecation message)
 - #56858 (Fix doc of `std::fs::canonicalize`)

Failed merges:

 - #56741 (treat ref-to-raw cast like a reborrow: do a special kind of retag)

r? @ghost
@bors bors merged commit ceee7f3 into rust-lang:master Dec 16, 2018
@alexcrichton alexcrichton deleted the simd_select_bitmask branch December 17, 2018 02:59
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants