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.

Sign up for GitHub

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 repr(simd) on ADTs containing a single array field #63531

Closed
wants to merge 13 commits into from
Closed

Support repr(simd) on ADTs containing a single array field #63531

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
257158f
Support repr(simd) on ADTs containing a single array field
gnzlbg Jul 13, 2019
3773bcf
Document which errors are caught in typeck
gnzlbg Aug 13, 2019
58fe683
Ignore tidy length in simd-array-transmute test
gnzlbg Aug 14, 2019
8963966
Rename simd_type_size to simd_ty_and_len
gnzlbg Aug 14, 2019
bf44809
Remove assert
gnzlbg Aug 14, 2019
a446967
Revert unnecessary param_env change
gnzlbg Aug 14, 2019
518eff2
Improve non-machine type in error message
gnzlbg Aug 14, 2019
fbe64f4
Add more transmute tests
gnzlbg Aug 14, 2019
173aafa
Fix more tests
gnzlbg Aug 15, 2019
9f72fc7
Simplify simd_ty_and_len
gnzlbg Aug 28, 2019
06037a3
Fix typo
gnzlbg Aug 28, 2019
69d0fa9
Skip align attribute in tests
gnzlbg Aug 29, 2019
a21939b
Bump min-llvm-version of transmute test
gnzlbg Aug 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 108 additions & 23 deletions src/librustc/ty/layout.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,34 +687,124 @@ impl<'tcx> LayoutCx<'tcx, TyCtxt<'tcx>> {
}

// SIMD vector types.
ty::Adt(def, ..) if def.repr.simd() => {
let element = self.layout_of(ty.simd_type(tcx))?;
let count = ty.simd_size(tcx) as u64;
assert!(count > 0);
let scalar = match element.abi {
Abi::Scalar(ref scalar) => scalar.clone(),
_ => {
tcx.sess.fatal(&format!("monomorphising SIMD type `{}` with \
a non-machine element type `{}`",
ty, element.ty));
ty::Adt(def, substs) if def.repr.simd() => {
// Supported SIMD vectors are homogeneous ADTs with at least one field:
//
// * #[repr(simd)] struct S(T, T, T, T);
// * #[repr(simd)] struct S { x: T, y: T, z: T, w: T }
// * #[repr(simd)] struct S([T; 4])
//
// where T is a "machine type", e.g., `f32`, `i64`, `*mut _`.

// SIMD vectors with zero fields are not supported.
// (should be caught by typeck)
if def.non_enum_variant().fields.is_empty() {
tcx.sess.fatal(&format!(
"monomorphising SIMD type `{}` of zero length", ty
));
}

// Type of the first ADT field:
let f0_ty = def.non_enum_variant().fields[0].ty(tcx, substs);

// Heterogeneous SIMD vectors are not supported:
// (should be caught by typeck)
for fi in &def.non_enum_variant().fields {
if fi.ty(tcx, substs) != f0_ty {
tcx.sess.fatal(&format!(
"monomorphising heterogeneous SIMD type `{}`", ty
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these new post-monomorphization errors... if so, why that?

Copy link
Contributor Author

@gnzlbg gnzlbg Aug 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(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.

Copy link
Contributor Author

@gnzlbg gnzlbg Aug 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Centril so is this issue resolved?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think I know what needs to be cleared up: the fatal was moved from one place to another, it's not new (AFAIK).

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

));
}
}

// The element type and number of elements of the SIMD vector
// are obtained from:
//
// * the element type and length of the single array field, if
// the first field is of array type, or
//
// * the homogenous field type and the number of fields.
let (e_ty, e_len, is_array) = if let ty::Array(e_ty, _) = f0_ty.sty {
// First ADT field is an array:

// SIMD vectors with multiple array fields are not supported:
// (should be caught by typeck)
if def.non_enum_variant().fields.len() != 1 {
tcx.sess.fatal(&format!(
"monomorphising SIMD type `{}` with more than one array field",
ty
));
}

// Extract the number of elements from the layout of the array field:
let len = if let Ok(TyLayout{
details: LayoutDetails {
fields: FieldPlacement::Array {
count, ..
}, ..
}, ..
}) = self.layout_of(f0_ty) {
count
} else {
unreachable!();
};

(e_ty, *len, true)
} else {
// First ADT field is not an array:
(f0_ty, def.non_enum_variant().fields.len() as _, false)
};
let size = element.size.checked_mul(count, dl)

// SIMD vectors of zero length are not supported.
//
// Can't be caught in typeck if the array length is generic.
if e_len == 0 {
tcx.sess.fatal(&format!(
"monomorphising SIMD type `{}` of zero length", ty
));
}

// Compute the ABI of the element type:
let e_ly = self.layout_of(e_ty)?;
let e_abi = if let Abi::Scalar(ref scalar) = e_ly.abi {
scalar.clone()
} else {
// This error isn;t caught in typeck, e.g., if
gnzlbg marked this conversation as resolved.
Show resolved Hide resolved
// the element type of the vector is generic.
tcx.sess.fatal(&format!(
"monomorphising SIMD type `{}` with a non-machine element type `{}`",
ty, e_ty
))
};


// Compute the size and alignment of the vector:
let size = e_ly.size.checked_mul(e_len, dl)
.ok_or(LayoutError::SizeOverflow(ty))?;
let align = dl.vector_align(size);
let size = size.align_to(align.abi);

// Compute the placement of the vector fields:
let fields = if is_array {
FieldPlacement::Arbitrary {
offsets: vec![Size::ZERO],
memory_index: vec![0],
}
} else {
FieldPlacement::Array {
stride: e_ly.size,
count: e_len,
}
};

tcx.intern_layout(LayoutDetails {
variants: Variants::Single { index: VariantIdx::new(0) },
fields: FieldPlacement::Array {
stride: element.size,
count
},
fields,
abi: Abi::Vector {
element: scalar,
count
element: e_abi,
count: e_len,
},
largest_niche: element.largest_niche.clone(),
largest_niche: e_ly.largest_niche.clone(),
size,
align,
})
Expand Down Expand Up @@ -2170,11 +2260,6 @@ where

ty::Tuple(tys) => tys[i].expect_ty(),

// SIMD vector types.
ty::Adt(def, ..) if def.repr.simd() => {
this.ty.simd_type(tcx)
}

// ADTs.
ty::Adt(def, substs) => {
match this.variants {
Expand Down
16 changes: 0 additions & 16 deletions src/librustc/ty/sty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1823,22 +1823,6 @@ impl<'tcx> TyS<'tcx> {
}
}

pub fn simd_type(&self, tcx: TyCtxt<'tcx>) -> Ty<'tcx> {
match self.sty {
Adt(def, substs) => {
def.non_enum_variant().fields[0].ty(tcx, substs)
}
_ => bug!("simd_type called on invalid type")
}
}

pub fn simd_size(&self, _cx: TyCtxt<'_>) -> usize {
match self.sty {
Adt(def, _) => def.non_enum_variant().fields.len(),
_ => bug!("simd_size called on invalid type")
}
}

#[inline]
pub fn is_region_ptr(&self) -> bool {
match self.sty {
Expand Down
Loading