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

Change LanesAtMost32 to LanesAtMost64 (or possibly remove the lane limit altogether) #90

Closed
calebzulawski opened this issue Apr 8, 2021 · 24 comments

Comments

@calebzulawski
Copy link
Member

We would like to support at least 64-lane vectors (to allow AVX-512 vectors of u8) but there is an aarch64 codegen issue (rust-lang/rust#84020).

Once that issue is solved we can increase the lane limit.

@workingjubilee
Copy link
Member

I believe we should consider an alternative to the use of actual numbers in the name if we are going to wind up changing them around based on these codegen vagaries.

@Lokathor
Copy link
Contributor

Lokathor commented Apr 9, 2021

since it's a dumb temporary limit just call it LanesAtMost

@workingjubilee
Copy link
Member

alternatively we can use a const fn and change the internal implementation without having to remove impls for things, which is still backwards-incompatible but in a different way! :D

@calebzulawski
Copy link
Member Author

I'm not actually positive it's a temporary limit, unless we're planning on requiring all backends to support up to usize::MAX lanes (or okay with monomorphization errors). Also it's not clear what we're going to do about non-power-of-two lane counts.

But I agree about renaming it. Maybe something more like ValidLanes?

@thomcc
Copy link
Member

thomcc commented Apr 9, 2021

OTOH, having the count in the name explains what you have to do to fix the error

@workingjubilee
Copy link
Member

We discussed this in the meeting and decided that since we really don't want to go below 32, because we consider AVX2 support important, it's fine to leave it with this name as it is.

Also, that the aarch64 codegen problem should be considered as blocking stabilization until we understand why.

@calebzulawski calebzulawski changed the title Change LanesAtMost32 to LanesAtMost64 Change LanesAtMost32 to LanesAtMost64 (or possibly remove the lane limit altogether) May 24, 2021
@calebzulawski
Copy link
Member Author

Updated the title of this issue to reflect some expressed sentiment that maybe this trait shouldn't exist at all.

We should probably revisit in the future, but it may be appropriate to just produce a monomorphization error. The compiler currently produces one if the size exceeds some implementation-defined value (65535 lanes, I think), but the error is not very descriptive. If we can improve the error reporting, maybe we can remove this trait altogether.

@BurntSushi
Copy link
Member

@calebzulawski IIRC, monomorphization errors are undesirable. As I had understood it, I had thought rustc was developed in such a way as to totally avoid surfacing monomorphization errors to end users. But I don't have a ton of context on that. It might be wise to ping the compiler team on something like this. (If only to discover whether monomorphization errors are a "hard no" or not.)

@calebzulawski
Copy link
Member Author

Yeah, that would definitely be a good idea. Realistically it's extremely unlikely to encounter the error in "normal" code, but if it's an absolute no we need to keep the trait. We could also explore using a lane count type smaller than usize, though that seems hacky.

@calebzulawski
Copy link
Member Author

Actually, I forgot the trait is also preventing sizes that aren't a power of two... so fairly easy to get wrong. Still worth discussing, I think.

@programmerjake
Copy link
Member

well, technically arrays also have monomorphization errors: [0i32; !0] should fail to compile. I think a monomorphization error is fine for the case of a huge lane count, but for now we should keep LanesAtMost32 and rename it to LanesArePowerOf2 (or similar), and strive to eventually remove it completely (or just leave it as a trait you can use but remove it from all the bounds in core::simd, we aren't the only ones who like powers-of-2).

@BurntSushi
Copy link
Member

well, technically arrays also have monomorphization errors: [0i32; !0] should fail to compile

It does fail? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=03be5c230f87259db3ed325707a24a44

Or are you saying that error is reported at monomorphization time? I can't tell.

In any case, what matters is whether the relevant teams consider that a bug or not (if it's being reported at monomorphization time). I don't know the answer to that question.

I think a monomorphization error is fine for the case of a huge lane count

As I said, the relevant teams (compiler?) should be consulted about this. cc @rust-lang/compiler

@programmerjake
Copy link
Member

well, technically arrays also have monomorphization errors: [0i32; !0] should fail to compile

It does fail? https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=03be5c230f87259db3ed325707a24a44

Or are you saying that error is reported at monomorphization time? I can't tell.

I'm saying the error is reported at monomorphization time (or at least will be if sufficient obfuscating generics are added).

@BurntSushi
Copy link
Member

Yes, we shouldn't assume that existing behavior is intended and plow forward. That's not a good strategy for cooperation.

@programmerjake
Copy link
Member

programmerjake commented May 24, 2021

Yes, we shouldn't assume that existing behavior is intended and plow forward. That's not a good strategy for cooperation.

well, for arrays at least, I remember reading that the error on array sizes being more than isize::MAX is intended to be a monomorphization error, though I can't find a reference after some superficial googling.

That said, +1 for cooperation!

@oli-obk
Copy link
Contributor

oli-obk commented May 25, 2021

@lqd is working on improving diagnostics during monomorphization time. They are undesirable though. We would definitely prefer these errors to be handled via bounds, but there is no consensus yet on the various aspects of that. Even with improvements to post monomorphization diagnostics, they have subpar UX. Check builds and IDEs won't show them. And they don't occur in libraries that define them, but on crates that use them.

Ideas for how to represent the bounds that you need and how to handle them in general are very welcome. If you come to the conclusion that post monomorphization errors are preferrable, that is also important to know.

@calebzulawski
Copy link
Member Author

I agree that the trait bound is the "rusty" way to do it. The trait bound actually never comes up in the API in non-generic code. In generics the problem is threefold:

  • The trait must be appropriately named to make it clear what the error is if you are missing the bound.
  • If we intend on changing the vector length limitations as the library improves, and change the name of the trait to match, the trait must be unstable.
  • If the trait is unstable, it's extremely hard to work with vectors in generics.

I think if we can come up with a name that is descriptive enough to produce a good error message, but flexible enough to stay the same as we reduce limitations (add non-powers-of-two, increase maximum length, etc), we can just stick with a trait.

@programmerjake
Copy link
Member

SupportedLaneCount?

@oli-obk
Copy link
Contributor

oli-obk commented May 26, 2021

I mean... ideally you don't want a trait, but a formula, but const_evaluatable_checked is nowhere near ready. Neither impl, design nor consensus wise

@calebzulawski
Copy link
Member Author

Since this is just a true/false situation a trait might not be perfect but it's pretty good. Once const_evaluatable_checked is done we can use it for a generic trait implementation instead of enumerating each valid type.

@calebzulawski
Copy link
Member Author

@workingjubilee @programmerjake I think we can close this now?

@workingjubilee
Copy link
Member

I think while we changed the name, the driving thrust of this ticket was actually only supporting lanes of a certain count? So we should reorient this instead.

@calebzulawski
Copy link
Member Author

Towards the end of the discussion I think we settled on using a trait to limit lane counts, rather than a monomorphisation error.

@calebzulawski
Copy link
Member Author

Closing in favor of #63. We've settled on the LaneCount<N>: SupportedLaneCount bound (unless const generic bounds are implemented). Also, 64-lane vectors are now supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants