-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
rustc_scalable_vector(N)
#143924
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
base: main
Are you sure you want to change the base?
rustc_scalable_vector(N)
#143924
Conversation
|
rustbot has assigned @compiler-errors. Use |
|
Some changes occurred in compiler/rustc_attr_parsing Some changes occurred in compiler/rustc_attr_data_structures Some changes occurred in compiler/rustc_passes/src/check_attr.rs Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred to the platform-builtins intrinsics. Make sure the cc @antoyo, @GuillaumeGomez, @bjorn3, @calebzulawski, @programmerjake Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred in compiler/rustc_codegen_ssa Some changes occurred in compiler/rustc_codegen_gcc changes to the core type system Some changes occurred to the CTFE machinery |
|
I've changed this back to a draft and marked it as |
cf9474d to
d58c634
Compare
This comment was marked as resolved.
This comment was marked as resolved.
0c22701 to
3ad0898
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
5c92874 to
3edf1b6
Compare
This comment has been minimized.
This comment has been minimized.
3edf1b6 to
4f6b823
Compare
workingjubilee
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not intend to have repr(simd) survive this year, so I do not think this should be added.
Could you elaborate? |
|
I intend to replace it with an approach based on lang items for a variety of reasons, one of them being that to start with, the |
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial questions from my first pass of reading.
| Fixed, | ||
| } | ||
|
|
||
| fn vector_type_layout<FieldIdx, VariantIdx, F>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absorbs the former contents of simd_type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, simd_type and now scalable_vector_type just forward to this
library/core/src/intrinsics/simd.rs
Outdated
| /// Replacement for `transmute`, specifically for use with scalable SIMD types. | ||
| #[rustc_intrinsic] | ||
| #[rustc_nounwind] | ||
| pub unsafe fn simd_reinterpret<Src, Dst>(src: Src) -> Dst; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. For what purpose? Is there some way in which this is not transmute_unchecked? Is it just to attempt to bypass MIR implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a leftover from when these types weren't Sized, I've changed it to transmute_unchecked in the test that uses it, and we'll need to update the stdarch intrinsic generator to use it too once we start generating the intrinsics.
compiler/rustc_public/src/abi.rs
Outdated
| // FIXME(repr_scalable): Scalable vectors are `Sized` while the `sized_hierarchy` | ||
| // feature is not yet fully implemented |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, what is specifically missing for us to be able to make this uh... less-Sized? It's "these are Sized but not const Sized right?" So this should still return false after that? Arguably the problem here isn't that the answer of false is wrong, it's that the signature furnishes a dubious answer for the question once we're deeper into the sized_hierarchy impl solidifying, because it won't be one well-answered by a simple bool anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly that, these types will remain Sized, just not const Sized, and so technically this will still return true but what is_unsized is actually asking becomes a big more ambiguous and we'll probably change this function when we introduce const sizedness to clear that up
| /// Are vector registers used? | ||
| enum UsesVectorRegisters { | ||
| /// e.g. `neon` | ||
| FixedVector, | ||
| /// e.g. `sve` | ||
| ScalableVector, | ||
| No, | ||
| } | ||
|
|
||
| /// Determines whether the combination of `mode` and `repr` will use fixed vector registers, | ||
| /// scalable vector registers or no vector registers. | ||
| fn uses_vector_registers(mode: &PassMode, repr: &BackendRepr) -> UsesVectorRegisters { | ||
| match mode { | ||
| PassMode::Ignore | PassMode::Indirect { .. } => false, | ||
| PassMode::Cast { pad_i32: _, cast } => { | ||
| cast.prefix.iter().any(|r| r.is_some_and(|x| x.kind == RegKind::Vector)) | ||
| || cast.rest.unit.kind == RegKind::Vector | ||
| PassMode::Ignore | PassMode::Indirect { .. } => UsesVectorRegisters::No, | ||
| PassMode::Cast { pad_i32: _, cast } | ||
| if cast.prefix.iter().any(|r| r.is_some_and(|x| x.kind == RegKind::Vector)) | ||
| || cast.rest.unit.kind == RegKind::Vector => | ||
| { | ||
| UsesVectorRegisters::FixedVector | ||
| } | ||
| PassMode::Direct(..) | PassMode::Pair(..) | ||
| if matches!(repr, BackendRepr::SimdVector { .. }) => | ||
| { | ||
| UsesVectorRegisters::FixedVector | ||
| } | ||
| PassMode::Direct(..) | PassMode::Pair(..) => matches!(repr, BackendRepr::SimdVector { .. }), | ||
| PassMode::Direct(..) | PassMode::Pair(..) | ||
| if matches!(repr, BackendRepr::ScalableVector { .. }) => | ||
| { | ||
| UsesVectorRegisters::ScalableVector | ||
| } | ||
| _ => UsesVectorRegisters::No, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, I didn't realize this function existed. It is... answering a different question than it claims to. Not something we have to fix today, just making a note to myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specifically, a function that uses a floating-point type may also use a vector register in the actual hardware, so it's more like "should this use the vector-by-value ABI?" which is... a mouthful, so I'm not sure that's a better actual function name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've not made any changes here but happy to rename the function if we wanted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you are diffing its callsites anyways to account for "it's not a bool anymore"? Yeah, if passes_vectors_by_value sounds good then that might be nice, but I'm not particularly fussed here, I can always submit a PR myself after this one lands.
|
I don't think that making scalable vectors a non-const Sized type will work with SME since the size of scalable vectors can change depending on attributes placed on a function (ACLE spec). This is because there are 2 vector lengths (normal & streaming) and the length can changed based on CPU state. In C, the CPU state is controlled via function attributes, specifically:
Additionally, we will need to add compile-time checks to ensure scalable vectors aren't passed as arguments or returned across a mode change (ACLE spec). This essentially means that we can't support the use of scalable vectors in generic function signatures. I think the better approach is to go back to the original design in #118917 where scalable vectors are essentially extern types to which we add some capabilities. This avoids issues with generics since these types can never be passed as arguments or returned in a generic context. This design can still make use of the Sized type hierarchy since we still need the distinction between MetaSized and Unsized, with scalable vectors being the latter. |
tbh that sounds more like what we want is for Arm's scalable vectors to have a const-generic argument that's implicitly set by the surrounding function's mode which is either known (normal and locally-streaming functions), or a const-generic that's implicitly set by the caller (streaming-compatible functions). passing/returning scalable vectors between incompatible modes would then just produce a type error due to the const-generic argument not matching, but you can pass scalable vectors between compatible modes since the compiler can see the const-generic matches. |
|
@Amanieu My question is essentially as @programmerjake said: is there a reason that the SME and SVE modes are not two-or-more types, then? Whether the mode is a generic parameter or not is not too important to me, but it does seem intuitive to treat it as such. It's not like we can do FFI without knowing the mode, right? |
|
Documentation we'll want to reference while discussing this: |
|
@programmerjake I am sure Amanieu will provide more information, but one thing he pointed out when I asked in another context is that we would have to inhibit usage of SVE and SME types in the same function, because LLVM doesn't support switching modes except on function boundaries. However, it seems we will have to implement compile-time handling regarding functions for SVE and SME either way, so I suspect using the tools we already have for such would be better. We already have 3 types of functions which each require compile-time checking, after all. |
|
I don't think having 2 separate types for SVE and SME vectors is going to work. The main issue is that it's absolutely not possible to have scalable locals of 2 different modes in the same function. LLVM doesn't support it, and for good reason since even calculating the stack offsets for locals would involve several SME/SVE mode switches. The problem is that since these types would be runtime-sized, you would be able to smuggle those types into a function in various ways other than parameters and return values, and there's no way you can enforce this in a generic context. |
|
The main advantage of making SVE vectors fully unsized is that it allows us to reliably determine, before monomorphization, whether the signature of a function has SVE locals, parameters or return values. This means that:
The downsides are:
Overall I think the advantages outweigh the inconvenients and result in a simpler implementation in the compiler. It is also forward-compatible with making these types runtime-sized in the future. |
|
Hmmmm. Only requiring signature checking does help... I think I'm not overly attached to a specific direction but I'd like to have the FIXMEs reflect our current inclination, at least. |
|
I'm happy to land this as it is for now (with scalable vectors being |
Maybe I'm missing something, but how does making the vectors fully unsized let us determine that? You can still write generic functions that can be instantiated by fully unsized types: #![feature(extern_types)]
#![feature(sized_hierarchy)]
use std::marker::PointeeSized;
unsafe extern "C" {
type Bar;
fn get_bar() -> &'static Bar;
}
fn foo<T: PointeeSized>(x: &T) {}
fn main() {
foo::<Bar>(unsafe { get_bar() });
}In this example, The other disadvantage of making them fully unsized is that they can't be locals or return types or implement I've been thinking of the sizedness of these types as a distinct problem from what we do about streaming-vs-non-streaming and all of the issues related to these types needing the SVE target feature. If they are non-const Sized, then we can make these locals/return types/ |
|
Oh, I understand now what you meant by making the types fully unsized enabling us to determine whether there are locals, parameters or return types: because the generic parameter won't implement Nevertheless, I think my point is still the same, that we could make them non-const Sized and usable as locals and return types in SVE-enabled functions, and consider the limitations on generic instantiation a separate thing, giving us a bit more usability. |
Extend parsing of `ReprOptions` with `rustc_scalable_vector(N)` which optionally accepts a single literal integral value - the base multiple of lanes that are in a scalable vector. Can only be applied to structs. Co-authored-by: Jamie Cunliffe <[email protected]>
Extend well-formedness checking and HIR analysis to prohibit the use of scalable vectors in structs, enums, unions, tuples and arrays. LLVM does not support scalable vectors being members of other types, so these restrictions are necessary. Co-authored-by: Jamie Cunliffe <[email protected]>
6cdc8f7 to
d8b3d28
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
Introduces `BackendRepr::ScalableVector` corresponding to scalable vector types annotated with `repr(scalable)` which lowers to a scalable vector type in LLVM. Co-authored-by: Jamie Cunliffe <[email protected]>
LLVM doesn't handle stores on `<vscale x N x i1>` for `N != 16`, a type used internally in SVE intrinsics. Spilling to the stack to create debuginfo will cause errors during instruction selection. These types that are an internal implementation detail to the intrinsic, so users should never see them types and won't need any debuginfo. Co-authored-by: Jamie Cunliffe <[email protected]>
Scalable vectors cannot be members of ADTs and thus cannot be kept over await points in async functions.
Scalable vector types only work with the relevant target features enabled, so require this for any function with the types in its signature.
d8b3d28 to
6a9fb63
Compare
|
It's also unclear whether we will ever be able to enable the compiler to have more ambiguously-sized argument and return types in the future. I think we eventually will want to explicitly ban what we want to ban rather than relying on some other element of the language "doing it for us". If nothing else, for the sake of producing a better error message, at least. That doesn't have to be addressed here, obviously. |
| // FIXME(rustc_scalable_vector): Scalable vectors are `Sized` while the | ||
| // `sized_hierarchy` feature is not yet fully implemented. After `sized_hierarchy` is | ||
| // fully implemented, scalable vectors will remain `Sized`, they just won't be | ||
| // `const Sized` - whether `is_unsized` continues to return `true` at that point will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // `const Sized` - whether `is_unsized` continues to return `true` at that point will | |
| // `const Sized` - whether `is_unsized` continues to return `false` at that point will |
| // FIXME(eddyb) there should be a size cap here | ||
| // (probably what clang calls "illegal vectors"). | ||
| } | ||
| BackendRepr::ScalableVector { .. } => unreachable!("scalable vectors are unsupported"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can guess why you used panic in one case and unreachable in the other, but I'd prefer it was consistent in either direction.
| BackendRepr::ScalableVector { .. } => unreachable!("scalable vectors are unsupported"), | |
| BackendRepr::ScalableVector { .. } => panic!("scalable vectors are unsupported"), |
| false | ||
| } | ||
| BackendRepr::ScalableVector { .. } => { | ||
| unreachable!("scalable vectors are unsupported") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| unreachable!("scalable vectors are unsupported") | |
| panic!("scalable vectors are unsupported") |
| /// `N` in `rustc_scalable_vector(N)` - the element count of the scalable vector | ||
| ElementCount(u128), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can just be u64. We will only parse 0..=u64::MAX and we will reject with an error things larger than 2.pow(16).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel almost like we should say "factor" instead of "count" since the "actual count" is unknown until runtime but uses the N provided here as a factor, but then syntactically the count is whatever was given in the attribute, but then... anyway, either is good.
|
|
||
| #[rustc_scalable_vector(2)] | ||
| struct TyConstPtr(*const u8); | ||
| //~^ ERROR: element type of a scalable vector must be a primitive scalar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The full error is here:
error: element type of a scalable vector must be a primitive scalar
--> $DIR/illformed-element-type.rs:19:1
|
LL | struct TyConstPtr(*const u8);
| ^^^^^^^^^^^^^^^^^
|
= help: only `u*`, `i*`, `f*`, `*const`, `*mut` and `bool` types are accepted
This suggests that *const u8 would be accepted, but this is rejected. Either the help needs to be changed or the code in rustc needs to be changed.
| })) | ||
| } | ||
|
|
||
| BackendRepr::ScalableVector { .. } => Err(Heterogeneous), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like the wrong answer, they should be definitionally homogeneous, but maybe it's because we produce incorrect code if we answer as homogeneous?
| RustcScalableVector { | ||
| /// The base multiple of lanes that are in a scalable vector, if provided. `element_count` | ||
| /// is not provided for representing tuple types. | ||
| element_count: Option<Box<u128>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment from earlier about only having a u64 here anyways.
Since we will reject starting around 65536 we could also constrain further to u32. That might let us potentially avoid finagling boxes.
| // and casts it to a `svbool4_t`/`<vscale x 4 x i1>`. Therefore, it's important that | ||
| // the `<vscale x 4 x i32>` never spills because that'll cause errors during | ||
| // instruction selection. Spilling to the stack to create debuginfo for these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fascinating. because the corresponding <vscale x 4 x i1> predicate vector might also be spilled as a result of the <vscale x 4 x i32> vector being spilled?
| if matches!(ty.kind(), ty::Adt(def, _) if def.repr().scalable()) | ||
| && !matches!(adt_def.repr().scalable, Some(ScalableElt::Container)) | ||
| { | ||
| // Scalable vectors can only be fields of structs if the type has an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sometimes I feel like I use typo nits as "have I read this?" markers, because the default Viewed checkbox hides the file... I often want to go back and cross-reference.
| // Scalable vectors can only be fields of structs if the type has an | |
| // Scalable vectors can only be fields of structs if the type has a |
| BackendRepr::SimdVector { .. } => { | ||
| return Err(CannotUseFpConv); | ||
| } | ||
| BackendRepr::ScalableVector { .. } => unreachable!(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| BackendRepr::ScalableVector { .. } => unreachable!(), | |
| BackendRepr::ScalableVector { .. } => panic!("scalable vectors are unsupported"), |
| #[rustc_scalable_vector] | ||
| struct ScalableTuple(ScalableU8, ScalableU8, ScalableU8); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the recursively nested composition case of ScalableTupleTuple also wellformed? I didn't catch it in the illformed cases.
|
That last scan-through should be all, unless something is revealed (made apparent?) by the next iteration, so @rustbot author |
Supercedes #118917.
Initial experimental implementation of rust-lang/rfcs#3838. Introduces a
rustc_scalable_vector(N)attribute that can be applied to types with a single[$ty]field (foru{16,32,64},i{16,32,64},f{32,64},bool).rustc_scalable_vectortypes are lowered to scalable vectors in the codegen backend.As with any unstable feature, there will necessarily be follow-ups as we experiment and find cases that we've not considered or still need some logic to handle, but this aims to be a decent baseline to start from.
See #145052 for request for a lang experiment.