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

Sync portable-simd to remove autosplats #91484

Merged
merged 17 commits into from
Dec 8, 2021

Conversation

workingjubilee
Copy link
Member

@workingjubilee workingjubilee commented Dec 3, 2021

This PR syncs portable-simd in up to rust-lang/portable-simd@a838552 in order to address the type inference breakages documented on nightly in #90904 by removing the vector + scalar binary operations (called "autosplats", "broadcasting", or "rank promotion", depending on who you ask) that allow {scalar} + &'_ {scalar} to fail in some cases, because it becomes possible the programmer may have meant {scalar} + &'_ {vector}.

A few quality-of-life improvements make their way in as well:

  • Lane counts can now go to 64, as LLVM seems to have fixed their miscompilation for those.
  • {i,u}8x64 to __m512i is now available.
  • a bunch of #[must_use] notes appear throughout the module.
  • Some implementations, mostly instances of impl core::ops::{Op}<Simd> for Simd that aren't {vector} + {vector} (e.g. {vector} + &'_ {vector}), leverage some generics and where bounds now to make them easier to understand by reducing a dozen implementations into one (and make it possible for people to open the docs on less burly devices).
  • And some internal-only improvements.

None of these changes should affect a beta backport, only actual users of core::simd (and most aren't even visible in the programmatic sense), though I can extract an even more minimal changeset for beta if necessary. It seemed simpler to just keep moving forward.

calebzulawski and others added 17 commits November 13, 2021 13:22
Update CONTRIBUTING.md for the fact that Travis is no longer used
Instead of implementing each "deref" pattern for every single scalar,
we can use type parameters for Simd operating on &Self.
We can use a macro, but keep it cleaner and more explicit.
Instead of implementing {Op}Assign traits for individual scalar type args
to Simd<_, _>, use parametric impls that reassert the bounds of the binary op.
Resolves my comment in rust-lang#197, at least for now; rust-lang#187 is pending but since these are already here, just commented, it seemed to make sense to me to re-enable them anyway.
In order to assure type soundness, these "base" impls
need to go directly on Simd<T, _> for every scalar type argument.
A bit of cleanup of ops.rs is still warranted.
Unfortunately, splatting impls currently break several crates.
Rust needs more time to review possible mitigations, so
drop the impls for the `impl Add<T> for Simd<T, _>` pattern, for now.
Generic `core::ops` for `Simd<T, _>`

In order to maintain type soundness, we need to be sure we only implement an operation for `Simd<T, _> where T: SimdElement`... and also valid for that operation in general. While we could do this purely parametrically, it is more sound to implement the operators directly for the base scalar type arguments and then use type parameters to extend the operators to the "higher order" operations.

This implements that strategy and cleans up `simd::ops` into a few submodules:
- assign.rs: `core::ops::*Assign`
- deref.rs:  `core::ops` impls which "deref" borrowed versions of the arguments
- unary.rs: encloses the logic for unary operators on `Simd`, as unary ops are much simpler

This is possible since everything need not be nested in a single maze of macros anymore. The result simplifies the logic and allows reasoning about what operators are valid based on the expressed trait bounds, and also reduces the size of the trait implementation output in rustdoc, for a huge win of 4 MB off the size of `struct.Simd.html`! This addresses a common user complaint, as the original was over 5.5 MB and capable of crashing browsers!

This also carries a fix for a type-inference-related breakage, by removing the autosplatting (vector + scalar binop) impls, as unfortunately the presence of autosplatting was capable of busting type inference. We will likely need to see results from a Crater run before we can understand how to re-land autosplatting.
@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 3, 2021
@workingjubilee
Copy link
Member Author

All of the minimized examples documented so far as of 2021-12-02 can be compiled by rustc with these changes to core.

@workingjubilee workingjubilee added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Dec 7, 2021
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

I'll leave beta-accepted to T-compiler (?) discussion, but I suspect it probably makes sense to just backport this wholesale, or do a targeted backport that cfg's out the submodule entirely -- those two strategies feel like the best options to me at this point.

@bors
Copy link
Contributor

bors commented Dec 7, 2021

📌 Commit eef4371 has been approved by Mark-Simulacrum

@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 7, 2021
@bors
Copy link
Contributor

bors commented Dec 8, 2021

⌛ Testing commit eef4371 with merge 11fb21f...

@bors
Copy link
Contributor

bors commented Dec 8, 2021

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 11fb21f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 8, 2021
@bors bors merged commit 11fb21f into rust-lang:master Dec 8, 2021
@petrochenkov
Copy link
Contributor

I get these errors after this PR (most likely):

   Compiling core v0.0.0 (C:\msys64\home\we\rust\library\core)
error[E0425]: cannot find function `simd_select_bitmask` in module `crate::intrinsics`
   --> library\core\src\..\..\portable-simd\crates\core_simd\src\masks\bitmask.rs:108:32
    |
108 |             crate::intrinsics::simd_select_bitmask(
    |                                ^^^^^^^^^^^^^^^^^^^ not found in `crate::intrinsics`
    |
help: consider importing this function
    |
2   | use crate::simd::intrinsics::simd_select_bitmask;
    |

error[E0425]: cannot find function `simd_bitmask` in module `crate::intrinsics`
   --> library\core\src\..\..\portable-simd\crates\core_simd\src\masks\bitmask.rs:119:42
    |
119 |         unsafe { Self(crate::intrinsics::simd_bitmask(value), PhantomData) }
    |                                          ^^^^^^^^^^^^ not found in `crate::intrinsics`
    |
help: consider importing this function
    |
2   | use crate::simd::intrinsics::simd_bitmask;
    |

For more information about this error, try `rustc --explain E0425`.
error: could not compile `core` due to 2 previous errors

I didn't investigate further yet.

@apiraino
Copy link
Contributor

apiraino commented Dec 9, 2021

Discussed today in T-compiler meeting on Zulip.

Beta backport is deferred after investigation of this comment and the best strategy to adopt for backport (as laid out in this comment)

@workingjubilee
Copy link
Member Author

workingjubilee commented Dec 9, 2021

Huh, that's... weird. What were you trying to build? Surely not just std?

@petrochenkov
Copy link
Contributor

@workingjubilee

Huh, that's... weird. What were you trying to build? Surely not just std?

A regular rustc build - ./x.py test --bless src/test/ui --test-args something.
I'm building with incremental = true, so I'd blame that if the error wasn't from name resolution when incremental compilation is not in effect yet.
I'm also using -Ctarget-cpu=native, so some cfg-related issues like #91315 are also possible.

I'll be able to investigate further later today or tomorrow, if it's something incremental then it should go away after just a fresh rebuild.

@workingjubilee
Copy link
Member Author

That explains it. Yes, I think this is triggered by your target having AVX512 and apparently the paths not being quite right on our end, now that I look at it... somehow missed in the million other things I had to adjust or reflag to make them work in the compiler, ugh.

@apiraino apiraino added T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 16, 2021
@pnkfelix
Copy link
Member

pnkfelix commented Dec 16, 2021

Discussed in T-compiler triage meeting.

  1. We believe Sync portable-simd to remove autosplats #91484 is a T-libs issue, not T-compiler one,
  2. Nonetheless, we do think some backport is justified here. We as a team recommend just removing the module entirely in a targeted backport.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2021
Sync portable-simd to fix libcore build for AVX-512 enabled targets

Fixes rust-lang#91484 (comment)
cc `@workingjubilee`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 18, 2021
Sync portable-simd to fix libcore build for AVX-512 enabled targets

Fixes rust-lang#91484 (comment)
cc `@workingjubilee`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 19, 2021
Sync portable-simd to fix libcore build for AVX-512 enabled targets

Fixes rust-lang#91484 (comment)
cc ``@workingjubilee``
@m-ou-se m-ou-se added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 5, 2022
@m-ou-se
Copy link
Member

m-ou-se commented Jan 5, 2022

We as a team recommend just removing the module entirely in a targeted backport.

Discussed it in the libs-api meeting. Backporting either this PR or a complete removal both seems fine. Marked as accepted. The backporter can decide whatever is easiest for them.

@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 5, 2022
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.59.0, 1.58.0 Jan 5, 2022
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jan 5, 2022
Stand-in for a backport of "Sync portable-simd to remove autosplats rust-lang#91484".
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 8, 2022
…ulacrum

[beta] backports

Backports these PRs:

* Fix HashStable implementation on InferTy rust-lang#91892
* Revert setting a default for the MACOSX_DEPLOYMENT_TARGET env var for linking rust-lang#91870
* Make rustdoc headings black, and markdown blue rust-lang#91534
* Disable LLVM newPM by default rust-lang#91190
* Deduplicate projection sub-obligations rust-lang#90423
*  Sync portable-simd to remove autosplats rust-lang#91484 by dropping portable_simd entirely (keeping the subtree, just from std/core)
*  Quote bat script command line rust-lang#92208
* Fix failing tests rust-lang#92201 (CI fix)

r? `@Mark-Simulacrum`
@workingjubilee workingjubilee deleted the simd-remove-autosplats branch March 25, 2022 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. merged-by-bors This PR was explicitly merged by bors. 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.