Support repr(simd) on ADTs containing a single array field #63531
Conversation
|
Some changes occurred in diagnostic error codes |
rust-highfive
added
the
S-waiting-on-review
| for fi in &def.non_enum_variant().fields { | ||
| if fi.ty(tcx, substs) != f0_ty { | ||
| tcx.sess.fatal(&format!( | ||
| "monomorphising heterogeneous SIMD type `{}`", ty |
There was a problem hiding this comment.
Are these new post-monomorphization errors... if so, why that?
There was a problem hiding this comment.
Not really, typeck errors before any of these can trigger, at least, as long as everything is correct there.
I added these to help during development. If typeck fails to error, an error here is better than an error down the line. That's why these are ICEs instead of something nicer.
There was a problem hiding this comment.
Alright; this sounds good. Could you add comments to the places where you added these .fatal errors noting that these should be caught in typeck?
(Also, why isn't bug!(...) being used for ICEs here?)
There was a problem hiding this comment.
(Also, why isn't bug!(...) being used for ICEs here?)
No idea, the code was using fatal error for ICES already so I just also used that - the existing fatal error even has a test..
Could you add comments to the places where you added these .fatal errors noting that these should be caught in typeck?
Sure.
There was a problem hiding this comment.
So i've done that.
The already existing error that fails if the element type isn't a machine type is currently not caught in typeck (this was already the case), and can fail if a generic SIMD vector is instantiated with an inappropriate element type. The way this is handled is that libcore should have a trait that bounds the accepted types so that this cannot happen (these features are all perma unstable).
There is a new monomorphization type error here now that we support generic lengths, and that's if the length passed to the vector is zero. We can only catch this once we know the actual value of the vector length, which right now is only at monomorphization time.
Either libcore would bound the length of the vector, once we have bounds for const generics, or we should start accepting zero-element vectors (I don't see a reason to forbid this), but that is probably better done in a different PR.
There was a problem hiding this comment.
@Centril so is this issue resolved?
There was a problem hiding this comment.
I leave that to @eddyb -- the only thing I would note is that we don't want to add new monomorphization errors in stable but such a consideration seems far off since this is just for experimentation.
There was a problem hiding this comment.
There is a huge difference between monomorphization time errors that affect Rust users, and adding code to catch logic errors in rustc early.
I regularly get LLVM errors when using Rust. Their output is completely useless, and the first step into debugging those is requiring users to re-compile a Rust toolchain with LLVM assertions enabled, and debugging back to Rust from there.
I'd much rather have a monomorphization time error just saying "This generic intrinsics does not support that type", and wish others would as well.
There was a problem hiding this comment.
I think I know what needs to be cleared up: the fatal was moved from one place to another, it's not new (AFAIK).
There was a problem hiding this comment.
@gnzlbg I think you misunderstood me; I was making a general remark about user exposed monmorphization time errors, not internal ones for intrinsics... so we should be good.
gnzlbg
force-pushed
the
array_simd
branch
4 times, most recently
from
9f77ec4 to
c9d6e43
Compare
This PR allows using `#[repr(simd)]` on ADTs containing a single array field: ```rust #[repr(simd)] struct S0([f32; 4]); #[repr(simd)] struct S1<const N: usize>([f32; N]); #[repr(simd)] struct S2<T, const N: usize>([T; N]); ``` This should allow experimenting with portable packed SIMD abstractions on nightly that make use of const generics.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
| @@ -1039,6 +1039,29 @@ fn generic_simd_intrinsic( | |||
| llret_ty: &'ll Type, | |||
| span: Span | |||
| ) -> Result<&'ll Value, ()> { | |||
| // Given a SIMD vector type `x` return the element type and the number of | |||
| // elements in the vector. | |||
| fn simd_type_size(bx: &Builder<'a, 'll, 'tcx>, x: Ty<'tcx>) -> (Ty<'tcx>, usize) { | |||
| @@ -1761,6 +1761,7 @@ pub fn check_simd(tcx: TyCtxt<'_>, sp: Span, def_id: DefId) { | |||
| match e.sty { | |||
| ty::Param(_) => { /* struct<T>(T, T, T, T) is ok */ } | |||
There was a problem hiding this comment.
Was this added later? It effectively invalidates the checks below...
Even more reason we should have a single #[lang = "simd"] instead of many #[repr(simd)].
There was a problem hiding this comment.
I think this was added at some point last year to support generic vectors.
There was a problem hiding this comment.
For the time being at least we probably want to type check SIMD vectors early, e.g., #[repr(simd)] struct Foo(Vec<i32>, Vec<i32>); and #[repr(simd)] struct Bar<const N: usize>([Vec<i32>; N]); should fail to type check.
If we move to a single:
#[lang = "simd"]
struct Simd<T, const N: usize>([T; N]) where [T; N]: SomeTrait;in the future then we might just accept anything and leave rejecting invalid inputs to SomeTrait.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
rfcbot
removed
the
proposed-final-comment-period
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
☔ The latest upstream changes (presumably #64513) made this pull request unmergeable. Please resolve the merge conflicts. |
|
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. The RFC will be merged soon. |
rfcbot
added
finished-final-comment-period
scottmcm
added
S-waiting-on-author
|
@gnzlbg Now that the FCP's done, can you rebase please? |
|
While unfortunate, unless someone picks this up and finishes the rebase, we'll likely be closing it when the next round of triage comes around. @gnzlbg -- can you provide at least some feedback or sense of timeline on when we can expect progress here? Maybe we can ask for help from someone who's familiar with this patch who can help rebase it? |
|
@Mark-Simulacrum :( isn't closing it a waste of review and implementation effort? This is an useful feature, all this discussion for nothing.. I petition to keep it open until it's rebased and merged |
|
@dlight The PR can always be reopened when someone has the time to finish the work. For example, you could pick it up and finish it yourself. |
|
I should have time to finish it this afternoon, I started rebasing a couple of time but got too many conflicts. |
|
Ping from triage |
JohnCSimon
added
the
S-inactive
|
Hello from triage |
Support repr(simd) on ADTs containing a single array field This is a squash and rebase of `@gnzlbg's` rust-lang#63531 I've never actually written code in the compiler before so just fumbled my way around until it would build 😅 I imagine there'll be some work we need to do in `rustc_codegen_cranelift` too for this now, but might need some input from `@bjorn3` to know what that is. cc `@rust-lang/project-portable-simd` ----- This PR allows using `#[repr(simd)]` on ADTs containing a single array field: ```rust #[repr(simd)] struct S0([f32; 4]); #[repr(simd)] struct S1<const N: usize>([f32; N]); #[repr(simd)] struct S2<T, const N: usize>([T; N]); ``` This should allow experimenting with portable packed SIMD abstractions on nightly that make use of const generics.
Support repr(simd) on ADTs containing a single array field This is a squash and rebase of `@gnzlbg's` rust-lang#63531 I've never actually written code in the compiler before so just fumbled my way around until it would build 😅 I imagine there'll be some work we need to do in `rustc_codegen_cranelift` too for this now, but might need some input from `@bjorn3` to know what that is. cc `@rust-lang/project-portable-simd` ----- This PR allows using `#[repr(simd)]` on ADTs containing a single array field: ```rust #[repr(simd)] struct S0([f32; 4]); #[repr(simd)] struct S1<const N: usize>([f32; N]); #[repr(simd)] struct S2<T, const N: usize>([T; N]); ``` This should allow experimenting with portable packed SIMD abstractions on nightly that make use of const generics.
This PR allows using
#[repr(simd)]on ADTs containing asingle array field:
This should allow experimenting with portable packed SIMD
abstractions on nightly that make use of const generics.
It also has a fly-by cleanup that removes
simd_typeandsimd_sizemethods.r? @eddyb