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

Limit all types to 64 lanes #67

Merged
merged 3 commits into from
Feb 15, 2021
Merged

Limit all types to 64 lanes #67

merged 3 commits into from
Feb 15, 2021

Conversation

calebzulawski
Copy link
Member

@calebzulawski calebzulawski commented Feb 10, 2021

In rust-lang/rust#27731, @oli-obk indicated some concern over allowing monomorphization errors (which are definitely not user-friendly).

This PR extends the LanesAtMost64 trait to be implemented over all vector types. This adds a bit of complexity to the implementation.

This solution seems pretty good, but before going with it I want to be sure the following are true:

  • Vectors will be useful after stabilization even if the LanesAtMost64 trait never gets stabilized (much like the now-removed std::array::LengthAtMost32). Considering I didn't need to update the tests, I think this is true.
  • We can remove all traces of this trait without breaking the API
  • We can change the bound in the future to add more support (such as wider vectors, or non-powers-of-two Support non-power-of-two vector lengths. #63)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 10, 2021

So, once this is merged, the post-monomorphization error in the compiler is unreachable, right? If that is so, we should change it to a span_bug!

@calebzulawski
Copy link
Member Author

From stdsimd yes, but it's still possible if you use the repr_simd feature directly.

@workingjubilee
Copy link
Member

workingjubilee commented Feb 10, 2021

@oli-obk People writing code that depends on #![feature(repr_simd)] directly can always directly access the error in question, and there are multiple such nightly-only libraries and users of them, and many of them are quite fragile, being out-of-step with the compiler and dependent on highly unstable and infrequently developed nightly features, which is exactly why we're trying to design a replacement for those.

At the current moment, however, monomorphization errors are fairly common in such code when an error does occur, which is part of why we don't think it's clear that the original change is a huge diagnostics regression, even if we think it's worth fixing here.

@@ -14,6 +14,9 @@ mod intrinsics;
mod ops;
mod round;

mod lanes_at_most_64;
pub use lanes_at_most_64::*;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to actually publicly export this trait?

I guess ideally what we want is to be able to bound the const generic when that becomes possible.

If users do need to use this trait (such as in generic code) then maybe we could come up with a different name for it and seal it so that it's a bit less of a visible hack?

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

it needs to be visible for generics

name bikeshed begins

Copy link
Contributor

@KodrAus KodrAus Feb 10, 2021

Choose a reason for hiding this comment

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

If it needs to be visible and possibly stabilized maybe we could call it something like FixedSimd and turn it into a useful trait?

#[rustc_on_unimplemented="fixed SIMD vectors are implemented for types with lane sizes of 2, 4, 8, 16, 32, or 64"]
pub trait FixedSimd: private::Sealed {
    type T;
    const N: usize;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it needs to live into stable. At least I hope it doesn't.

But for now, on nightly, users do need to see it.

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 agree that it doesn't need to be stable. You can do quite a bit with generics without touching the trait: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=46c81ab903379f78b15b03ec98eab68e

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't want to stabilize it then any name seems ok to me 🙂 We can at least use the private::Sealed trick to prevent anybody else from implementing it.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need to use a glob import here? Seems like it risks exposing something in the public API unintentionally?

Copy link
Contributor

Choose a reason for hiding this comment

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

No significant risk.

When adding items you don't want to leave the crate you should be marking them as pub(crate) anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't mean it will happen. Doesn't seem worth the risk to me if you're relying on folks using pub(crate). I certainly don't always do that. The glob import doesn't seem worth it to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

For this particular glob, yes it's trivial to turn it into a plain import. The other globs import quite a bit and we rely on using pub and pub(crate) to not forget anything. I think it could go either way.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 11, 2021

@oli-obk People writing code that depends on #![feature(repr_simd)] directly can always directly access the error in question, and there are multiple such nightly-only libraries and users of them, and many of them are quite fragile, being out-of-step with the compiler and dependent on highly unstable and infrequently developed nightly features, which is exactly why we're trying to design a replacement for those.

At the current moment, however, monomorphization errors are fairly common in such code when an error does occur, which is part of why we don't think it's clear that the original change is a huge diagnostics regression, even if we think it's worth fixing here.

Ah that makes sense. Maybe we should adjust the error message then and point them to the a-little-bit-more-stable stdsimd types once they hit nightly?

Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

I assume diffing absolutely every single op is required due to the bound leaking through to them? Looks fine if so.

crates/core_simd/src/vectors_u8.rs Show resolved Hide resolved
@calebzulawski
Copy link
Member Author

calebzulawski commented Feb 13, 2021

I assume diffing absolutely every single op is required due to the bound leaking through to them? Looks fine if so.

Yeah, the ops can only be implemented when the type is valid, so the bounds leak through. Any implementations on a concrete type rather than generic over lane count doesn't need it.

@workingjubilee workingjubilee merged commit cbca211 into master Feb 15, 2021
@workingjubilee workingjubilee deleted the limit-lanes branch April 14, 2021 02:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants