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

Improve SIMD type element count validation #80652

Merged
merged 5 commits into from
Feb 8, 2021

Conversation

calebzulawski
Copy link
Member

Resolves rust-lang/portable-simd#53.

These changes are motivated by stdsimd moving in the direction of const generic vectors, e.g.:

#[repr(simd)]
struct SimdF32<const N: usize>([f32; N]);

This makes a few changes:

  • Establishes a maximum SIMD lane count of 2^16 (65536). This value is arbitrary, but attempts to validate lane count before hitting potential errors in the backend. It's not clear what LLVM's maximum lane count is, but cranelift's appears to be much less than usize::MAX, at least.
  • Expands some SIMD intrinsics to support arbitrary lane counts. This resolves the ICE in the linked issue.
  • Attempts to catch invalid-sized vectors during typeck when possible.

Unresolved questions:

  • Generic-length vectors can't be validated in typeck and are only validated after monomorphization while computing layout. This "works", but the errors simply bail out with no context beyond the name of the type. Should these errors instead return LayoutError or otherwise provide context in some way? As it stands, users of stdsimd could trivially produce monomorphization errors by making zero-length vectors.

cc @bjorn3

@rust-highfive
Copy link
Collaborator

r? @estebank

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

65536, or a 16-bit value, was chosen "arbitrarily". Allow me to provide a justification, even if it is a mere post hoc rationale:

Rust supports byte (as in octet) addressing, and it is reasonable to say something like "a SIMD vector supported in a programming language should be fully addressable at the same level as the language". In addition, most SIMD vectors let you use byte-size lanes in hardware but few will go down to bits or even nybbles, so that is another angle which suggests we should focus on byte elements as our minimum. With a 16-bit number we could index every byte in a hypothetical 524,288-bit vector. This is 512*(2^10) bits, or 10 doublings past our current capability. Even the most gratuitous interpretations of Moore's law would suggest we would not be able to reach this in commodity hardware for another 20 years, and even supercomputers would take a while. For various reasons, in practice, vectors grow even more slowly than that, doubling in commodity hardware only every ~9 years or so (SSE for 128-bit vectors in 1999, AVX-512F in 2017). Even Arm SVE2 only allows a maximum vector length of 2048 bits, for which an 8-bit number is sufficient to index every byte.

While a possible implementation of a hypothetical "extensible vector" ISA, like a further extension of SVE2 to "SVE2.1" or "SVE3", or even the current alpha of the RISCV-V design, on a... very beefy chip, even by supercomputer standards... may allow a hypothetical 2^19 bit monster-vector to live and breathe even before the next 20 years pass, I am nonetheless content with saying that this 65536 lane limitation does not impose an unnecessary limitation on the capabilities of the Rust compiler for any futures I expect Rust to remain relevant in. I would even support an 8-bit limitation for now with an eye towards eventually lifting it, as it would complicate handling 4096-bit vectors and thus could realistically be considered a limitation in meeting the capabilities of silicon that might be printed even in the lifetimes of mere mortals, as that exceeds the capacity of even the most ambitious ISAs, but by a realistic amount. Note that SVE2 itself is not publicly available on any cores I am aware of, so as far as I know, and things like masking operations will already run into amusing consequences on 2048-bit lanes as-is, so this itself is definitely not a limitation today, either.

Finally, as noted in that issue, Cranelift supports 16-bit lane lengths.

@workingjubilee
Copy link
Member

Regarding errors, I believe we should try to catch zero-lane vectors specially if we can. Philosophically, the minimum number of lanes for a "SIMD" operation to make sense is not 0 or even 1, but 2.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 18, 2021

r? @nagisa

@rust-highfive rust-highfive assigned nagisa and unassigned estebank Jan 18, 2021
Copy link
Member

@nagisa nagisa left a comment

Choose a reason for hiding this comment

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

Overall looks pretty good to me I have a couple comments in-line but nothing major.

@bors try

compiler/rustc_typeck/src/check/check.rs Outdated Show resolved Hide resolved
compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved
@nagisa
Copy link
Member

nagisa commented Jan 22, 2021

@bors try

@bors
Copy link
Contributor

bors commented Jan 22, 2021

⌛ Trying commit 07db2bf with merge 84b21bfb06d318b7a01ab0f0a70d0190ffbca05e...

@bors
Copy link
Contributor

bors commented Jan 22, 2021

☀️ Try build successful - checks-actions
Build commit: 84b21bfb06d318b7a01ab0f0a70d0190ffbca05e (84b21bfb06d318b7a01ab0f0a70d0190ffbca05e)

@calebzulawski
Copy link
Member Author

In the last stdsimd meeting we decided it would be best to limit #[repr(simd)] to only powers-of-two, since that's all cranelift supports. If/when support for non-powers-of-two is added, we can remove this restriction.

Since repr_simd is still an unstable feature I think we should be fine to change that, but I do think a crater run might be a good idea to make sure.

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 10.24s
tidy check
tidy error: following path contains more than 1458 entries, you should move the test to some relevant subdirectory (current: 1464): /checkout/src/test/ui
tidy error: following path contains more than 2669 entries, you should move the test to some relevant subdirectory (current: 2671): /checkout/src/test/ui/issues
Found 435 error codes
Found 0 error codes with no tests
Done!
some tidy checks failed

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
   Compiling regex v1.4.3
   Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
    Finished release [optimized] target(s) in 9.93s
tidy check
tidy error: following path contains more than 1458 entries, you should move the test to some relevant subdirectory (current: 1464): /checkout/src/test/ui
tidy error: following path contains more than 2669 entries, you should move the test to some relevant subdirectory (current: 2671): /checkout/src/test/ui/issues
Found 435 error codes
Found 0 error codes with no tests
Done!
some tidy checks failed

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Suite("src/test/assembly") not skipped for "bootstrap::test::Assembly" -- not in ["src/tools/tidy"]
Check compiletest suite=assembly mode=assembly (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 29 tests
iiiiiiiiiiiiiiiiiiiiiiiiiiiii

Suite("src/test/incremental") not skipped for "bootstrap::test::Incremental" -- not in ["src/tools/tidy"]
 finished in 0.069 seconds
Check compiletest suite=incremental mode=incremental (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
---
Suite("src/test/debuginfo") not skipped for "bootstrap::test::Debuginfo" -- not in ["src/tools/tidy"]
Check compiletest suite=debuginfo mode=debuginfo (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)

running 116 tests
iiiiiiiiii.i.i..i...i..ii...i.i....ii..........iiii.........i.....i...i.......ii.i.ii.....iiii.....i 100/116
test result: ok. 78 passed; 0 failed; 38 ignored; 0 measured; 0 filtered out; finished in 2.34s

 finished in 2.405 seconds
Suite("src/test/ui-fulldeps") not skipped for "bootstrap::test::UiFullDeps" -- not in ["src/tools/tidy"]
---
Testing error-index stage2
doc tests for: /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md


command did not execute successfully: "/checkout/obj/build/bootstrap/debug/rustdoc" "-Winvalid_codeblock_attributes" "-Dwarnings" "-Znormalize-docs" "--test" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md" "--test-args" ""

stdout ----

running 982 tests
---
test /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0779 (line 15704) ... ok

failures:

---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0074::_::Note__this_error_code_is_no_longer_emitted_by_the_compiler_ (line 1559) stdout ----
error[E0075]: SIMD vector length must be a power of two
 --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:1563:1
  |
6 | struct Bad<T>(T, T, T);

error: aborting due to previous error

For more information about this error, try `rustc --explain E0075`.
For more information about this error, try `rustc --explain E0075`.
Couldn't compile the test.
---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0074::_::Note__this_error_code_is_no_longer_emitted_by_the_compiler_ (line 1568) stdout ----
error[E0075]: SIMD vector length must be a power of two
 --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:1572:1
  |
6 | struct Good(u32, u32, u32);

error: aborting due to previous error

For more information about this error, try `rustc --explain E0075`.
For more information about this error, try `rustc --explain E0075`.
Couldn't compile the test.
---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0076 (line 1619) stdout ----
error[E0075]: SIMD vector length must be a power of two
 --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:1623:1
  |
6 | struct Good(u32, u32, u32); // ok!

error: aborting due to previous error

For more information about this error, try `rustc --explain E0075`.
For more information about this error, try `rustc --explain E0075`.
Couldn't compile the test.
---- /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md - Rust_Compiler_Error_Index::E0077 (line 1644) stdout ----
error[E0075]: SIMD vector length must be a power of two
 --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:1648:1
  |
6 | struct Good(u32, u32, u32); // ok!

error: aborting due to previous error

For more information about this error, try `rustc --explain E0075`.

@KodrAus
Copy link
Contributor

KodrAus commented Jan 25, 2021

@bors try

@bors
Copy link
Contributor

bors commented Jan 25, 2021

⌛ Trying commit a4bab7c with merge aed835460397ceb814007e0d97479d7c98844d78...

@bors
Copy link
Contributor

bors commented Jan 26, 2021

☀️ Try build successful - checks-actions
Build commit: aed835460397ceb814007e0d97479d7c98844d78 (aed835460397ceb814007e0d97479d7c98844d78)

@KodrAus
Copy link
Contributor

KodrAus commented Jan 26, 2021

@craterbot check

We'd like to do a crater run to see what the impact of our changes to #[repr(simd)] are here. I thought a check-only run should be sufficient, since errors are likely to surface as monomorphization failures.

@craterbot
Copy link
Collaborator

👌 Experiment pr-80652 created and queued.
🤖 Automatically detected try build aed835460397ceb814007e0d97479d7c98844d78
⚠️ Try build based on commit f4eb5d9, but latest commit is a4bab7c. Did you forget to make a new try build?
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 26, 2021
@bjorn3
Copy link
Member

bjorn3 commented Feb 6, 2021

Spurious

  • paranoia fails to link to paranoia_sys
  • typ overflows while evaluating a trait bound

Real regressions

vek (various versions) and vrust (version 0.0.1) use size 3 vectors and vrust also uses a size 9 vector to represent a 3x3 matrix. vek only uses #[repr(simd)] when both the nightly and repr_simd features are enabled.

@calebzulawski
Copy link
Member Author

vrust appears to be unmaintained and has no reverse dependencies, but vek seems quite active and a bunch of crates depend on it. Still, 2 crates doesn't seem so bad. Unsurprisingly, both are doing something that should really be a wrapper over std::simd once it exists. Thoughts on how to proceed?

@bjorn3
Copy link
Member

bjorn3 commented Feb 6, 2021

Given that vek only uses #[repr(simd)] when nightly features are enabled, I did suggest to inform the author(s) of vek. Once they release a new version with this fixed I think it would be fine to land this PR. If they don't release a new version within a reasonable amount of time, we will have to decide if it is acceptable to break it.

@calebzulawski
Copy link
Member Author

That sounds like a good plan, if that sounds good to everyone I can open an issue over there.

@scottmcm
Copy link
Member

scottmcm commented Feb 6, 2021

Remember that this is nightly-only, so any forewarning about breakage is just about being nice, not required.

Something like opening an issue on their repos now then merging a week or later sounds more than sufficient to me.

@calebzulawski
Copy link
Member Author

vek has been updated, so I believe we should be ready to go!

@nagisa
Copy link
Member

nagisa commented Feb 7, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Feb 7, 2021

📌 Commit a4bab7c has been approved by nagisa

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

bors commented Feb 7, 2021

⌛ Testing commit a4bab7c with merge bb587b1...

@bors
Copy link
Contributor

bors commented Feb 8, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing bb587b1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 8, 2021
@bors bors merged commit bb587b1 into rust-lang:master Feb 8, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 8, 2021
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #80652!

Tested on commit bb587b1.
Direct link to PR: #80652

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Feb 8, 2021
Tested on commit rust-lang/rust@bb587b1.
Direct link to PR: <rust-lang/rust#80652>

💔 miri on windows: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
💔 miri on linux: test-pass → test-fail (cc @oli-obk @eddyb @RalfJung).
@RalfJung
Copy link
Member

RalfJung commented Feb 8, 2021

Attempts to catch invalid-sized vectors during typeck when possible.

So "invalid" here means "not a power of two or too big"?

@oli-obk @lcnr FYI -- this seems to be a new kind of post-monomorphization error (see OP).

@lcnr
Copy link
Contributor

lcnr commented Feb 8, 2021

Are you leaking this behavior to stable or does it require unstable features to trigger?

Preventing post-monomorphization errors here is definitely possible and desirable

@bjorn3
Copy link
Member

bjorn3 commented Feb 8, 2021

#[repr(simd)] is unstable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE with unusual vector sizes