Add an attribute to check the number of lanes in a SIMD vector after monomorphization

[calebzulawski]

Allows std::simd to drop the LaneCount<N>: SupportedLaneCount trait and maintain good error messages.

r? @RalfJung

cc @workingjubilee @programmerjake

[calebzulawski]

Hmm, that's not good. I'm probably using the visitor wrong. I'll investigate tomorrow

[RalfJung]

I think I understand the goal here, but I'm not sure what exactly one has to do to actually find all the types that could be relevant. Iterating the signature and the locals feels ad-hoc, and also redundant since all arguments and the return value also appear as MIR locals. And then you try to recursively iterate the type and its fields... no idea if that makes sense, sorry. I also don't have the time right now to dig into this.

r? types
(I guess that's the right group of people?)

Also Cc @BoxyUwU @lcnr @compiler-errors since this is essentially adding a compiler hack to work around const generics being painful, IIUC. (Would be good to reference an issue with context and prior discussion describing why the SupportedLaneCount trait should be dropped.)

[programmerjake]

for background on dropping the lane count traits: rust-lang/portable-simd#364 (comment)

[lcnr]

https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/mono.20check.20that.20walks.20all.20types/near/539909904 this is the related zulip thread?

Going to ignore the fact that I personally feel somewhat iffy about this change and feel in favor of a SimdType: HasSupportedLaneCount/LaneCount<N>: SupportedLaneCount approach. That's a lang/feature design question and I don't have a good way to judge it. I do feel like it would be disappointing to end up with a worse design to avoid being blocked on some const generics feature, so I would personally like the discussion for whether this is desirable to go into the issues and requirements for the current design. I do expect that the current stable min_const_generics is insufficient for generic code right now.

---

I think visiting all locals (and their fields) is very... questionable 😅

You'vbe mentioned putting this check into layout_of where it would belong imo.

We already have similar errors for arrays, e.g. [u8; usize::MAX], and I would expect that we treat these errors the same

[jdonszelmann]

this should indeed get a new-style attribute parser

View changes since this review

[calebzulawski]

Adding the LaneCount<N>: SupportedLaneCount bound to the type makes std::simd somewhat painful to use with generics. I've written some functions that convert between different numbers of lanes and sometimes the solver just gives up. In practice, no one ever really tries to use unreasonably large vectors, but we need to reject them because at a certain point LLVM stops handling them well. Compared to the rest of the API, it ends up being one of the most burdensome parts with nearly zero benefit to the average user. I'm glad arrays aren't [T, N] where Length<N>: SupportedArrayLength!

I could do this in layout, and perhaps it's more precise there. I think the compromise is that layout errors don't track a source span, but maybe that can be added.

[RalfJung]

I like the analogy with "this type is too big for your target".

[RalfJung]

If this is a backend limitation, why is the maximum lane count configured by the library in the type? The max size is a hard-coded constant in the compiler, after all.

[calebzulawski]

This is probably the most relevant zulip thread: https://rust-lang.zulipchat.com/#narrow/channel/257879-project-portable-simd/topic/Post-mono.20errors.20for.20better.20UX

A little bit of info in this issue as well, though mostly outdated as we now support non-power-of-two lengths: rust-lang/portable-simd#364

If I were able to somehow embed a const assert rather than a post-mono error I would be much happier with that, but AFAIK that's still not possible

[calebzulawski]

If this is a backend limitation, why is the maximum lane count configured by the library in the type? The max size is a hard-coded constant in the compiler, after all.

Unfortunately I tried that in the past (#80652) and failed crater because some crates are using #[repr(simd)]. Also, I could see a particular architecture permitting a much longer length than is portable to other architectures.

[RalfJung]

Unfortunately I tried that in the past (#80652) and failed crater because some crates are using #[repr(simd)].

It's a nightly-only feature so I am not too bothered by that.
Also, doesn't that mean they use a too-big-vector that doesn't work well?

EDIT: Looking at that PR, it seems the failure was mostly about non-power-of-2 vectors, not too big vectors?

I could see a particular architecture permitting a much longer length than is portable to other architectures.

That seems to be an argument in favor of having this in the compiler. (If we want to accept such non-portability; the max size currently only depends on the target pointer width.)

[calebzulawski]

For std::simd, we have settled on 64 elements as a reasonable maximum size. With larger vectors, you can see weird unrolling and writing vectors to memory particularly on architectures with smaller vectors. I wouldn't want to enforce 64 elements for the #[repr(simd)] layout, though, when some architectures have 1024+ bit vectors. We definitely don't want to cap the vector size at the native architecture size, either, when e.g. neon only supports 128 bit vectors but LLVM unrolls 512 bit vectors well. So I think setting a size particular to std::simd is probably best.

[programmerjake]

For std::simd, we have settled on 64 elements as a reasonable maximum size.

only for when we need separate trait impls for each size, when we don't need the trait impls I think 16384 is better since that is actually the hardware size on some RISC-V CPUs.

[rustbot]

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_hir/src/attrs

cc @jdonszelmann

[calebzulawski]

I switched to doing the check in layout calculation and there's no guesswork with making sure you don't miss checking the limit. The layout error is definitely worse (it doesn't even have a span). I tried giving it a span, but the only span available is the struct item, which IMO is also not very useful. Maybe a future change can pipe through a span that points to where the type is actually used.

[jdonszelmann]

r=me for the attribute stuff. I don't think I can judge the rest

[programmerjake]

maybe add a cross-crate test? idk how easy that is though

[calebzulawski]

yeah, I'm not sure how to do that, or what the criteria for that would be

[jdonszelmann]

You use aux crates. A cross crate test seems useful. https://rustc-dev-guide.rust-lang.org/tests/compiletest.html?highlight=Aux#building-auxiliary-crates

[calebzulawski]

I'm happy if someone can approve the layout error generation, and we leave the discussion of whether we should use this attribute or another solution for the RFC process before stabilization

[RalfJung]

IIRC the error spans for too big types are much better than what you describe here, so this can probably be fixed by making OversizedSimdType more like SizeOverflow in how it is treated.

Directly emitting emit_fatal from inside layout computation is indeed kind of crude, but there's no reason to do that I think other than "it's a lot more work to wire this up properly".

(I get that this is pre-existing, so this is for a future PR. But you seemed to think that this is somehow fundamental to raising the error during layout generation, when it isn't. We have all the infrastructure needed for better errors there, it looks like someone just cut some corners when adding these SIMD errors.)

[lcnr]

I believe returning the info to the caller in the same way as size errors should be done in this PR

[rustbot]

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

HIR ty lowering was modified

cc @fmease

[calebzulawski]

We have all the infrastructure needed for better errors there, it looks like someone just cut some corners when adding these SIMD errors.)

I suspect that was me :) Though at the time (and on stable today) the backends didn't report spans for layout errors.

I changed the layout errors to report properly.

[lcnr]

why does this test exist? it's a copy of tests/ui/simd/simd-lane-limit-err.rs, is it not?

[programmerjake]

because it's testing a different length because repr(simd, packed) does some unusual stuff with non-powers-of-2

[lcnr]

please add a comment explaining that then

[lcnr]

please add a cross-crate test and also make sure to track the fact that simd len is now a post-mono error somewhere so that it's mentioned when stabilizing this

r=me after that

[calebzulawski]

I changed the tests to be cross-crate. I figure we'll document the post-mono error in std::simd once we apply the attribute