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

Support for a scalable simd representation #118917

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

JamieCunliffe
Copy link
Contributor

As requested here the changes required to allow for scalable vector types to be represented.

A few of the restrictions on the types that are stated in the RFC haven't yet been implemented. I'll make a start on those, but I thought it would be worth getting some comments on some of the fundamental changes in here sooner rather than later.

r? @Amanieu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 13, 2023
@Noratrieb
Copy link
Member

cc @rust-lang/types for awareness about proposed scalable SIMD types, which are proposed to contain several major type system special cases. See the RFC linked in the description for more details.

@@ -122,7 +122,14 @@ pub(super) fn check_fn<'a, 'tcx>(
hir::FnRetTy::DefaultReturn(_) => body.value.span,
hir::FnRetTy::Return(ty) => ty.span,
};
fcx.require_type_is_sized(declared_ret_ty, return_or_body_span, traits::SizedReturnType);

if !declared_ret_ty.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

This needs a TODO to make sure it's not accidentally committed.

Copy link
Member

Choose a reason for hiding this comment

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

This should allow scalable simd parameters without requiring params_can_be_unsized.

Copy link
Member

@compiler-errors compiler-errors Jun 27, 2024

Choose a reason for hiding this comment

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

Yeah, but it specifically says:

for now just remove the check for testing purposes.

So it either needs a TODO to remove this once "testing purposes" are over, or the comment should be reworked to explain why it's correct that makes it seem less temporary.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

Just some questions that I had, I know this is still a draft -- just want to not forget + also be subscribed to this PR 😜

@@ -583,7 +583,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
infer::BoundRegionConversionTime::FnCall,
fn_sig.output(),
);
self.require_type_is_sized_deferred(output, expr.span, traits::SizedReturnType);
if !output.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -265,7 +265,10 @@ fn typeck_with_fallback<'tcx>(

for (ty, span, code) in fcx.deferred_sized_obligations.borrow_mut().drain(..) {
let ty = fcx.normalize(span, ty);
fcx.require_type_is_sized(ty, span, code);
// ScalableSIMD: Justify this.
Copy link
Member

Choose a reason for hiding this comment

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

Needs a TODO or a very detailed explanation here

Copy link
Member

Choose a reason for hiding this comment

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

This check should be moved to the call sites of require_type_is_sized_deferred.

@@ -2874,6 +2888,10 @@ impl<'tcx> Ty<'tcx> {
/// This is mostly useful for optimizations, as these are the types
/// on which we can replace cloning with dereferencing.
pub fn is_trivially_pure_clone_copy(self) -> bool {
if self.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this true?

Copy link
Member

Choose a reason for hiding this comment

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

Actually, for the record, this is unsound. For a type to implement Copy, it must implement Sized, since Sized is one of Copy's supertraits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's one of the problems we have with these types. We need them to be Copy but we can't implement the trait due to the bounds. These types should be trivially copyable as they are just a register and (for SVE anyway) can be copied with a mov instruction. It was a discussion with @Amanieu a while ago that lead to this approach, I'm definitely open to alternative suggestions as to how this could be done though.

Copy link
Member

Choose a reason for hiding this comment

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

This should probably be moved to is_copy_modulo_regions. Possibly further up, since it might depend on what the callers of that function are checking.

Copy link
Member

Choose a reason for hiding this comment

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

is_copy_modulo_regions must agree with the implementation of Copy. As I said before, these types cannot implement Copy soundly while also being unsized.

Copy link
Member

Choose a reason for hiding this comment

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

Then perhaps this exception should be moved up to the callers of is_copy_modulo_regions where we can get a better idea of what special handling is needed.

Copy link
Contributor

@lcnr lcnr Apr 6, 2024

Choose a reason for hiding this comment

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

Given that these types also have the requirements to only be used in registers, I would expect that trying to use too many if these variables at the same time is likely also not supported 🤔 Looking at the RFC I don't fully understand the requirements for these types, both the questions by RalfJ, e.g. https://github.com/rust-lang/rfcs/pull/3268/files#r1500263066, but also why is it ok to reference them if they may only be stored in registers?

How does LLVM model svfloat64_t types? I would expect them to encounter many of the same issues? edit: I found https://llvm.org/devmtg/2021-11/slides/2021-OptimizingCodeForScalableVectorArchitectures.pdf 🤔 they made a breaking change to how they store type sizes?

Copy link
Contributor

@lcnr lcnr Apr 6, 2024

Choose a reason for hiding this comment

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

if references to these simd registers are apparently ok and you also want their scope to be limited, could something like teh following work (where these simd registers are extern types with special behavior during codegen)

// An invariant token,similar to `scoped_threads` in std
#[derive(Copy, Clone)
struct Scope<'scope>(Phantomdata<*mut &'scope ()>);
fn sve_scope<F: for<'scope> Fn(Scope<'scope>) -> R>(f: F) -> R;

extern {
    type svbool<'scope>;
}

impl<'scope> Scope<'scope> {
    fn whilelt_b64(self, ..) -> &'scope mut svbool<'scope>;
    // ...
}

Given that I don't fully understand the constraints here there may definitely be issues with this approach

Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

@@ -199,6 +199,8 @@ declare_features! (
(internal, prelude_import, "1.2.0", None, None),
/// Used to identify crates that contain the profiler runtime.
(internal, profiler_runtime, "1.18.0", None, None),
/// Allows the use of scalable SIMD types.
(unstable, repr_scalable, "CURRENT_RUSTC_VERSION", None, None),
Copy link
Member

Choose a reason for hiding this comment

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

This should get a tracking issue and moved to the feature-group-start: actual feature gates section.

Copy link
Member

Choose a reason for hiding this comment

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

Also if the version you end up landing is not entirely complete, it should be marked as incomplete instead of unstable.


#[repr(simd, scalable(16))] //~ error: Scalable SIMD types are experimental
struct Foo {
_ty: [i8; 0],
Copy link
Member

Choose a reason for hiding this comment

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

This type is a lie and will give issues for Foo { _ty: [] }. Maybe use _ty: [i8] instead? That will also automatically mark it as !Sized.

@@ -422,6 +426,11 @@ impl<'a, Ty> TyAndLayout<'a, Ty> {
}))
}

Abi::ScalableVector { .. } => Ok(HomogeneousAggregate::Homogeneous(Reg {
kind: RegKind::ScalableVector,
size: Size::from_bits(128),
Copy link
Member

Choose a reason for hiding this comment

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

This size is a lie. Would panicking or returning Err instead work? Or is it genuinely possible for a struct that is passed as argument to contain a scalable vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the RFC I did state that these types can't be inside a struct (currently no checking to enforce that though). We might want to relax that in the future though. Currently we could panic or return an error here, I'll make this an error for now.

@@ -370,6 +370,11 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
if attrs.flags.contains(CodegenFnAttrFlags::NAKED) {
return;
}
// FIXME: Don't spill scalable simd, this works for most of them however,
// some intermediate types can't be spilled e.g. `<vscale x 4 x i1>`
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure even with this change, we do end up putting every value that cg_ssa couldn't determine as having a single assignment on the stack anyway.

@@ -124,6 +124,7 @@ impl LlvmType for Reg {
_ => bug!("unsupported float: {:?}", self),
},
RegKind::Vector => cx.type_vector(cx.type_i8(), self.size.bytes()),
RegKind::ScalableVector => cx.type_scalable_vector(cx.type_i8(), 16),
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comment.

@@ -1176,12 +1197,16 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
InvalidMonomorphization::MismatchedLengths { span, name, m_len, v_len }
);
match m_elem_ty.kind() {
ty::Int(_) => {}
ty::Int(_) | ty::Bool => {}
Copy link
Member

Choose a reason for hiding this comment

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

Could use a comment explaining this is for svbool_t.

Copy link

@michaelmaitland michaelmaitland Jun 27, 2024

Choose a reason for hiding this comment

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

Why is this change Arm SVE specific? RISC-V RVV also has scalable bool vector types. I don't think we need to make this comment target specific. We want to support scalable vectors of bool element type.

@@ -831,13 +831,13 @@ fn check_impl_items_against_trait<'tcx>(
}
}

pub fn check_simd(tcx: TyCtxt<'_>, sp: Span, def_id: LocalDefId) {
pub fn check_simd(tcx: TyCtxt<'_>, sp: Span, def_id: LocalDefId, is_scalable: bool) {
Copy link
Member

Choose a reason for hiding this comment

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

This needs to be reworked to check for _ty: [T] which must be an unsized slice.

@@ -122,7 +122,14 @@ pub(super) fn check_fn<'a, 'tcx>(
hir::FnRetTy::DefaultReturn(_) => body.value.span,
hir::FnRetTy::Return(ty) => ty.span,
};
fcx.require_type_is_sized(declared_ret_ty, return_or_body_span, traits::SizedReturnType);

if !declared_ret_ty.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

This should allow scalable simd parameters without requiring params_can_be_unsized.

@@ -265,7 +265,10 @@ fn typeck_with_fallback<'tcx>(

for (ty, span, code) in fcx.deferred_sized_obligations.borrow_mut().drain(..) {
let ty = fcx.normalize(span, ty);
fcx.require_type_is_sized(ty, span, code);
// ScalableSIMD: Justify this.
Copy link
Member

Choose a reason for hiding this comment

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

This check should be moved to the call sites of require_type_is_sized_deferred.

@@ -76,7 +76,9 @@ where
}
}
},
Abi::Vector { .. } | Abi::Uninhabited => return Err(CannotUseFpConv),
Abi::Vector { .. } | Abi::ScalableVector { .. } | Abi::Uninhabited => {
Copy link
Member

Choose a reason for hiding this comment

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

Make this unreachable! or bug!.

@@ -35,6 +35,7 @@ where
RegKind::Integer => false,
RegKind::Float => true,
RegKind::Vector => arg.layout.size.bits() == 128,
RegKind::ScalableVector => true,
Copy link
Member

Choose a reason for hiding this comment

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

Unreachable.

@@ -18,6 +18,7 @@ pub fn compute_abi_info<Ty>(fn_abi: &mut FnAbi<'_, Ty>) {
// FIXME(eddyb) there should be a size cap here
// (probably what clang calls "illegal vectors").
}
Abi::ScalableVector { .. } => {}
Copy link
Member

Choose a reason for hiding this comment

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

Unreachable.

} else if let ty::Slice(e_ty) = f0_ty.kind()
&& def.repr().scalable()
{
(*e_ty, 1, false)
Copy link
Member

Choose a reason for hiding this comment

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

Make e_len an Option.

if def.repr().scalable()
&& variants[FIRST_VARIANT].iter().all(|field| !field.0.is_zst())
{
bug!("Fields for a Scalable vector should be a ZST");
Copy link
Member

Choose a reason for hiding this comment

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

This check is incorrect.

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Some of the earlier comments still haven't been addressed.

Also this could use a test for when a scalable vector is live across an await or generator yield.

// an NLL error, it's a required check to prevent creation
// of unsized rvalues in a call expression.
self.tcx().dcx().emit_err(MoveUnsized { ty, span });
if !ty.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

This is not needed, it's already handled in the callers of ensure_place_sized.

Comment on lines +141 to +143
#[cfg(not(bootstrap))]
#[rustc_nounwind]
pub fn simd_reinterpret<Src: ?Sized, Dst: ?Sized>(src: Src) -> Dst;
Copy link
Member

Choose a reason for hiding this comment

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

This needs a doc comment explaining what this intrinsic is for. In this case you can just say that it is a replacement for transmute that is specifically for use by scalable SIMD types which are !Sized.

@@ -122,7 +122,7 @@ extern "rust-intrinsic" {
/// * Not be infinite
/// * Be representable in the return type, after truncating off its fractional part
#[rustc_nounwind]
pub fn simd_cast<T, U>(x: T) -> U;
pub fn simd_cast<T: ?Sized, U: ?Sized>(x: T) -> U;
Copy link
Member

Choose a reason for hiding this comment

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

Add a line in the docs about restrictions for scalable vector types (On the line that talks about T and U).

@@ -214,6 +214,7 @@ impl<'tcx> Stable<'tcx> for rustc_abi::Abi {
ValueAbi::Vector { element: element.stable(tables), count }
}
rustc_abi::Abi::Aggregate { sized } => ValueAbi::Aggregate { sized },
rustc_abi::Abi::ScalableVector { element: _element, elt: _elt } => todo!(),
Copy link
Member

Choose a reason for hiding this comment

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

We need to figure out how this maps to stable MIR.

@@ -2874,6 +2888,10 @@ impl<'tcx> Ty<'tcx> {
/// This is mostly useful for optimizations, as these are the types
/// on which we can replace cloning with dereferencing.
pub fn is_trivially_pure_clone_copy(self) -> bool {
if self.is_scalable_simd() {
Copy link
Member

Choose a reason for hiding this comment

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

This should be removed.

Copy link
Member

@compiler-errors compiler-errors left a comment

Choose a reason for hiding this comment

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

The feature gating for relaxing Sized checks in gather_locals seems to have the side-effect of allowing non-repr(scalable) unsized types also be valid locals. That should probably be fixed before this lands.

We probably need to find a better way of enforcing that a local may be Sized or repr(scalable), rather than just not registering Sized bounds for certain locals, since we don't know whether a type is Sized or not before we've done type checking. Perhaps we can use a new built-in trait like Local that's impl'd for T: Sized or when the struct is repr(scalable) or something.

@@ -144,7 +144,9 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherLocalsVisitor<'a, 'tcx> {
let var_ty = self.assign(p.span, p.hir_id, None);

if let Some((ty_span, hir_id)) = self.outermost_fn_param_pat {
if !self.fcx.tcx.features().unsized_fn_params {
if !(self.fcx.tcx.features().unsized_fn_params
|| self.fcx.tcx.features().repr_scalable)
Copy link
Member

Choose a reason for hiding this comment

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

Does this not cause us to accept regular unsized fn params as well once repr_scalable is enabled? That isn't correct.

@@ -162,7 +164,9 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherLocalsVisitor<'a, 'tcx> {
);
}
} else {
if !self.fcx.tcx.features().unsized_locals {
if !(self.fcx.tcx.features().unsized_locals
Copy link
Member

Choose a reason for hiding this comment

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

Similarly, this causes us to accept unsized locals if repr_scalable is enabled. This doesn't seem correct.

Copy link

@michaelmaitland michaelmaitland left a comment

Choose a reason for hiding this comment

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

From the LLVM LangRef:

The number of elements is a constant integer value larger than 0; elementtype may be any integer, floating-point or pointer type. Vectors of size zero are not allowed

Should we test this behavior in this PR?

Additionally, I think we are missing tests for the case where you try and create a bad scalable type:

#[repr(simd, scalable(4))]
pub struct BadScalableType1 {
    _ty: [f32],
    _ty2: [f32]
}

I'd like to know more about what is allowed / not allowed here. Does the name inside the struct matter? What if it isn't an array?

// vector. The use of 16 here is chosen as that will generate a valid type with both
// Arm SVE and RISC-V RVV. In the future with other architectures this might not be
// valid and might have to be configured by the target.
RegKind::ScalableVector => cx.type_scalable_vector(cx.type_i8(), 16),

Choose a reason for hiding this comment

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

Why do we use i8 and 16 instead of basing it off of the original type information?

@@ -1052,6 +1052,7 @@ fn build_struct_type_di_node<'ll, 'tcx>(
Cow::Borrowed(f.name.as_str())
};
let field_layout = struct_type_and_layout.field(cx, i);

Choose a reason for hiding this comment

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

nit: unrelated newline?

@@ -1176,12 +1197,16 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
InvalidMonomorphization::MismatchedLengths { span, name, m_len, v_len }
);
match m_elem_ty.kind() {
ty::Int(_) => {}
ty::Int(_) | ty::Bool => {}
Copy link

@michaelmaitland michaelmaitland Jun 27, 2024

Choose a reason for hiding this comment

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

Why is this change Arm SVE specific? RISC-V RVV also has scalable bool vector types. I don't think we need to make this comment target specific. We want to support scalable vectors of bool element type.

@@ -2271,6 +2296,7 @@ fn generic_simd_intrinsic<'ll, 'tcx>(
out_elem
});
}

Choose a reason for hiding this comment

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

nit: unrelated newline?

@@ -69,6 +69,10 @@ impl<'ll> CodegenCx<'ll, '_> {
unsafe { llvm::LLVMVectorType(ty, len as c_uint) }
}

pub(crate) fn type_scalable_vector(&self, ty: &'ll Type, len: u64) -> &'ll Type {

Choose a reason for hiding this comment

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

nit: len does not make sense here. maybe elt_cnt is better?

@@ -1266,6 +1274,10 @@ pub enum Abi {
Uninhabited,
Scalar(Scalar),
ScalarPair(Scalar, Scalar),
ScalableVector {
element: Scalar,
elt: u64,

Choose a reason for hiding this comment

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

nit: elt -> elt_cnt?

&self,
repr_scalable,
attr.span,
"Scalable SIMD types are experimental and possibly buggy"
Copy link

@michaelmaitland michaelmaitland Jun 27, 2024

Choose a reason for hiding this comment

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

Why do we call these SIMD? Is it because they are part of the simd crate? If so, that's unfortunate since SVE and RVV is not SIMD. The these extensions allow the vector length to vary at runtime whereas with SIMD the vector length is fixed at compile time. The best term is scalable vector types.

I'm not suggesting or requesting a change here, just making an observation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the term scalable vector types can also cause confusion. I have spoken with a few people that when you say scalable vectors, they think it's something to with vectors in the sense of a Vec not these types, although there would be context here.

Saying they allow the vector length to change at runtime is not correct here, that would be considered undefined behavior with SVE in LLVM and Rust. With SVE the vector length isn't changed, rather lanes are masked off. Changing vector length at runtime for SVE could lead to an incorrect stack if any SVE register was spilled to the stack. During execution SVE should be a fixed vector size with lanes ignored (for predicated instructions) based on the governing predicate.

Comment on lines 1 to 4
A scalable SIMD type was used in a context where they cannot exist. Scalable
SIMD types exist in a place that is somewhere between `Sized` and `Unsized`
therefore have restrictions on the uses. A scalable SIMD type can't exist in any
of the following:
Copy link

@michaelmaitland michaelmaitland Jun 27, 2024

Choose a reason for hiding this comment

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

Im not sure that telling a user they are somewhere between Sized and Unsized is helpful. Does that mean they are neither Sized or Unsized? Does that mean they are both Sized and Unsized? Is there really a third class of StaticUnknownConstantSized that we have yet to introduce? My opinion is that they should be neither Sized nor Unsized and we should not introduce a new trait, but should allow copying in certain circumstances without having Copy trait.

I suggest the following verbiage:

A scalable SIMD type was used in a context where they cannot exist. The size of a Scalable SIMD type is constant but unknown at compile time. Therefore restrictions are placed on their uses. A scalable SIMD type can't exist in any of the following:

@@ -888,6 +888,7 @@ extern "C" {
// Operations on array, pointer, and vector types (sequence types)
pub fn LLVMPointerTypeInContext(C: &Context, AddressSpace: c_uint) -> &Type;
pub fn LLVMVectorType(ElementType: &Type, ElementCount: c_uint) -> &Type;

Choose a reason for hiding this comment

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

Does LLVMVectorType need to be renamed to LLVMFixedVectorType?

@@ -366,6 +366,24 @@ impl<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>> FunctionCx<'a, 'tcx, Bx> {
return;
}

// LLVM doesn't handle stores on some of the internal SVE types that we are required

Choose a reason for hiding this comment

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

It looks like this is target independent file but we are adding target specific code. I'm not sure this belongs here. I would prefer to generalize this as such:

if target.some_property(operand)
  return

I am also confused why we're only returning in the case that we have a <vscale x i1 x 16> type. Why that type specifically? What about a <vscale x i1 x 32> or a <vscale x i32 x 4>?

On RISC-V we can spill all vector types. It is done using a vector store instruction. I am pretty sure SVE can do this too.

@Amanieu
Copy link
Member

Amanieu commented Jul 2, 2024

@JamieCunliffe Do you have any updates on this? The last commit to this PR is over 2 months ago and the RFC text was last updated 9 months ago.

I've talked with several people in Google, Huawei and Microsoft, all of whom have expressed a rather urgent desire for the ability to use SVE intrinsics in Rust code, especially now that SVE hardware is generally available.

The representation of the element type has been changed to be a slice
rather than a zero length array. Two feature gates are now required in
core_arch unsized fn params and unsized locals.

This still leaves unsized return types being an issue. For this we are
currently bypassing some of the `Sized` trait checking to pass when
the type is scalable simd.

This still leaves the copy issue. For that we have marked scalable
simd types as trivally pure clone copy. We have still had to remove
some trait checks for the copy trait with this though as they are
still performed in certain situations.

The implementation of `transmute` is also an issue for us. For this a
new SIMD intrinsic has been created simd_reinterpret which performs a
transmute on SIMD vectors. A few intrinsics need to be able to produce
an LLVM `undef` this intrinsic will also produce that when given a
zero sized input.
Rather than not spilling any scalable SIMD types for debug info, we
only avoid spilling the ones that are going to cause a problem.
Currently the only ones known to cause a problem are the internal
svbool types for AArch64.
Tests to ensure that scalable SIMD types can't exist in
struct, union, enum variants and compound types.

This also changes the well formed checking of types to improve the
error message when scalable SIMD types are included. The previous
implementation would also allow a scalable SIMD type as the last
element within a struct in some cases which we currently don't want to
allow.
Ensures that an SVE type can have a reference taken to it. This
currently emits a `dereferenceable` attribute for the ptr using the
element size as the number of bytes. While not perfect this is correct
as a vector will always have a least one primitive.
Rather than forcing the user to enable the unsized_fn_params and
unsized_locals features, we condition those features tests with if the
type is a scalable simd type.
Add some comments explaining some not so obvious things.
Mark code as unreachable on architectures that don't currently have
scalable vectors.
Remove some unused (and incorrect) checks that were being performed.
Refactor some code to improve it's readability.
Scalable vectors are unsized types so we can't hold them across an
await point.

Also clean up the tests to remove unsized features from tests as a
user shouldn't have to enable them when using scalable vectors.
@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
#16 2.735 Building wheels for collected packages: reuse
#16 2.736   Building wheel for reuse (pyproject.toml): started
#16 2.977   Building wheel for reuse (pyproject.toml): finished with status 'done'
#16 2.978   Created wheel for reuse: filename=reuse-4.0.3-cp310-cp310-manylinux_2_35_x86_64.whl size=132715 sha256=dfa09868353292d98f811d3efdb0d54d07389e808efc71d68e3b93c514bf8bec
#16 2.979   Stored in directory: /tmp/pip-ephem-wheel-cache-2gsb69cr/wheels/3d/8d/0a/e0fc6aba4494b28a967ab5eaf951c121d9c677958714e34532
#16 2.981 Installing collected packages: boolean-py, binaryornot, tomlkit, reuse, python-debian, markupsafe, license-expression, jinja2, chardet, attrs
#16 3.373 Successfully installed attrs-23.2.0 binaryornot-0.4.4 boolean-py-4.0 chardet-5.2.0 jinja2-3.1.4 license-expression-30.3.0 markupsafe-2.1.5 python-debian-0.1.49 reuse-4.0.3 tomlkit-0.13.0
#16 3.373 WARNING: Running pip as the 'root' user can result in broken permissions and conflicting behaviour with the system package manager. It is recommended to use a virtual environment instead: https://pip.pypa.io/warnings/venv
#16 DONE 3.5s
---
    Checking toml_edit v0.22.12
error[E0063]: missing field `scalable` in initializer of `ReprOptions`
   --> crates/hir-def/src/data/adt.rs:177:10
    |
177 |     Some(ReprOptions { int, align: max_align, pack: min_pack, flags, field_shuffle_seed: 0 })

   Compiling itoa v1.0.11
   Compiling ryu v1.0.18
   Compiling rust-analyzer v0.0.0 (/checkout/src/tools/rust-analyzer/crates/rust-analyzer)

@bors
Copy link
Contributor

bors commented Jul 28, 2024

☔ The latest upstream changes (presumably #128313) made this pull request unmergeable. Please resolve the merge conflicts.

bx.bitcast(val, llret_ty)
}
OperandValue::ZeroSized => bx.const_undef(llret_ty),
OperandValue::Pair(_, _) => todo!(),
Copy link
Member

Choose a reason for hiding this comment

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

Use InvalidMonomorphization instead of todo!

@@ -248,7 +248,7 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(

if memory_locals.contains(local) {
debug!("alloc: {:?} -> place", local);
if layout.is_unsized() {
if layout.is_unsized() && !layout.is_runtime_sized() {
Copy link
Member

Choose a reason for hiding this comment

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

This cannot possibly work, alloca doesn't have a valid size to allocate.

Comment on lines +541 to +543
if !place.layout.is_runtime_sized() {
assert_eq!(place.val.llextra.is_some(), place.layout.is_unsized());
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if !place.layout.is_runtime_sized() {
assert_eq!(place.val.llextra.is_some(), place.layout.is_unsized());
}
assert_eq!(place.val.llextra.is_some(), place.layout.is_unsized() && !place.layout.is_runtime_sized());

@Dylan-DPC
Copy link
Member

@JamieCunliffe any updates on this?

Comment on lines +1705 to +1707
/// Returns true if the size of the type is only known at runtime.
pub fn is_runtime_sized(&self) -> bool {
matches!(self.abi, Abi::ScalableVector { .. })
Copy link
Member

Choose a reason for hiding this comment

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

This coexisting with is_unsized feels potentially misleading, or at the very least ambiguous.

Also, the possibility of knowing the size at all feels like an edge case out of all possible "value-only"/"memory-unrepresentable" types discussed in e.g. these older RFC comments:

As far as Abi knowing about these types, that's an inevitability (since they do participate in ABI), and sadly I don't know of a good way to encode open-ended target-specific "handle" types (other than e.g. an interned [Either<Symbol, u64>] style sequence, comparable to specifying register names using strings in asm! etc.).

@eddyb
Copy link
Member

eddyb commented Nov 11, 2024

Based on #46571 (comment) (by @kennytm, responding to @Amanieu, on the matter of e.g. size_of_val on SVE types), I can probably expand on my comment above (#118917 (comment)):

The impression I'm getting is that the intent here is to make something similar to !Sized (and more specifically, some DynSized proposals) but "by-value, not in memory"... except even "by-value DynSized" is still in-memory, just allowing variable-level "move" operations (growing the stack dynamically and copying the contents, or even taking advantage of pass-by-ref).

But with a type that is meant to be register-only, wouldn't it make more sense to have it outside of the DynSized/Sized spectrum? How would you even place such a value in memory without the dependent typing to be able to ensure it's never accessed with different sizes? (unless this allows ARM SVE but rules out RISC-V's V extension, which would seem like quite the overspecialization IMO)

Relying on something like !Pointee to 100% rule out such types from memory (including being borrowed etc.) seems much better, and we can keep ?Pointee perma-unstable if only intrinsics might need it (if at all) - and that seems to be what I was saying even back in rust-lang/rfcs#3268 (comment).

@oli-obk
Copy link
Contributor

oli-obk commented Nov 11, 2024

Relying on something like !Pointee to 100% rule out such types from memory (including being borrowed etc.) seems much better,

This also overlaps with wasm value types (I forget what they are called, but they are very useful for opaque references to interpreter state (like js objects)), which have the same same "no ref, no placing in memory" rules

@Amanieu
Copy link
Member

Amanieu commented Nov 11, 2024

@eddyb In C, SVE types (this also applies to RVV) are allowed to be in memory. In both SVE and RVV, the size of a vector type is available at runtime and is guaranteed to be constant for the duration of the program. LLVM is able to dynamically allocate enough stack space for local variables and spilling, and functions can pass SVE vector by ref or by mut.

This is different from WASM reference types which truly cannot exist in memory and for which the concept of a size doesn't make sense.

@eddyb
Copy link
Member

eddyb commented Nov 11, 2024

RVV vl+vtype vs vlenb distinction ended up further discussed on the RFC thread: rust-lang/rfcs#3268 (comment)

This is different from WASM reference types which truly cannot exist in memory and for which the concept of a size doesn't make sense.

Yes, I was hoping infrastructure for that could be shared with (what I've described elsewhere as) "SSA-only vl+vtype-dependent vector values" (in the RVV case, at least), but the "worst-case memory fallback" choice removes that option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.