-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Use ty::layout for ABI computation instead of LLVM types. #40658
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
cc @rust-lang/compiler |
f55fb66
to
c91c10a
Compare
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 nits gathered over the day.
src/librustc/ty/layout.rs
Outdated
@@ -1087,25 +1089,33 @@ impl<'a, 'gcx, 'tcx> Layout { | |||
// Arrays and slices. | |||
ty::TyArray(element, count) => { | |||
let element = element.layout(infcx)?; | |||
let element_size = element.size(dl); | |||
let count = count as u64; |
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 cast is not necessary and is possibly dangerous (e.g. after a type in the declaration changes)
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.
It's currently usize
(even if it shouldn't be).
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.
if we care, maybe make fn usize_to_u64(x: usize)
and then do usize_to_u64(count)
?
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.
or (count: usize) as u64
.
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.
LOL. I'll add a whole variable dealing with this fact and a comment on it.
src/librustc/ty/layout.rs
Outdated
i: usize, | ||
variant_index: Option<usize>) | ||
-> Size { | ||
match *self { |
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.
debug_assert i==0, perhaps?
|
||
ty::TyTuple(tys, _) => tys[i], | ||
|
||
// SIMD vector types. |
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.
Assumes that all fields of a some vector must be the same (true for LLVM but not true for some architectures/proposals such as for RISC-V)
Not a big issue (also we discussed this on IRC already)
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.
Oh, just to be super clear: ty::layout
already assumes uniform vectors.
We'd have to overhaul it a bit when that assumption breaks down anyway.
src/librustc_trans/cabi_x86_64.rs
Outdated
} | ||
} | ||
// Currently supported vector size (AVX). | ||
const LARGEST_VECTOR_SIZE: usize = 256; |
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.
AVX-512 has 512 bits. (Also discussed on IRC)
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.
Right, I meant AVX
. AVX512
is different from AVX
like SSE3
is different from SSE
.
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.
Also cc @BurntSushi on what stance we should have in terms of ABI compat.
Scalar { .. } | | ||
CEnum { .. } | | ||
UntaggedUnion { .. } | | ||
RawNullablePointer { .. } => { |
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.
debug_assert i==0, perhaps?
You mean here? I might just take this out, except for UntaggedUnion
. It was meant for something I abandoned.
☔ The latest upstream changes (presumably #39628) made this pull request unmergeable. Please resolve the merge conflicts. |
b3001b2
to
6ac00de
Compare
src/librustc/ty/layout.rs
Outdated
@@ -1087,25 +1089,33 @@ impl<'a, 'gcx, 'tcx> Layout { | |||
// Arrays and slices. | |||
ty::TyArray(element, count) => { | |||
let element = element.layout(infcx)?; | |||
let element_size = element.size(dl); | |||
let count = count as u64; |
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.
or (count: usize) as u64
.
src/librustc_trans/type_of.rs
Outdated
pub fn size_of(&self, ty: Ty<'tcx>) -> machine::llsize { | ||
let layout = self.layout_of(ty); | ||
layout.size(&self.tcx().data_layout).bytes() as machine::llsize | ||
} | ||
} | ||
|
||
fn llvm_type_name<'a, 'tcx>(cx: &CrateContext<'a, 'tcx>, ty: Ty<'tcx>) -> String { |
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.
Nice catch!
@@ -202,6 +202,16 @@ impl TargetDataLayout { | |||
} | |||
} | |||
|
|||
pub trait HasDataLayout: Copy { |
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.
Generic bloat? Not sure this is worth worrying about.
} | ||
} | ||
|
||
pub trait LayoutTyper<'tcx>: HasTyCtxt<'tcx> { |
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 looks like a rather heavy abstraction - it is only used for .field
, which is only called from trans::abi
, which already defines LayoutExt
. I'll move Layout::field
to LayoutExt
and remove this trait.
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 abstraction exists so we can pull out the ABI computation into a separate crate, so the heaviness is intentional.
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'm fine with this if this is temporary code.
src/librustc/ty/layout.rs
Outdated
pub fn of(infcx: &InferCtxt<'a, 'gcx, 'tcx>, ty: Ty<'gcx>) | ||
-> Result<Self, LayoutError<'gcx>> { | ||
let ty = normalize_associated_type(infcx, ty); | ||
pub trait HasTyCtxt<'tcx>: HasDataLayout { |
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.
If you are defining this trait, maybe move it to ty::fold
and make TypeFolder
a subtrait? Can you add a blanket impl<T: HasTyCtxt> HasDataLayout for T
? Is there any place that needs data layout but does not have a tcx (aka do we need HasDataLayout
)?
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 tried that and it required weird inexpressible region bounds to work, sadly.
src/librustc_trans/abi.rs
Outdated
@@ -133,32 +134,279 @@ impl ArgAttributes { | |||
} | |||
} | |||
|
|||
pub trait CastTarget { |
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.
Any reason this is a trait rather than an enum (Reg | RegPair | Uniform)
? That would make moving away from LLVM easier.
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 wanted to make sure it works first, and the trait allows replacing with an enum later, but maybe I should do it now.
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.
ok
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.
Looks excellent. I should look at this last commit more but I have to go.
src/librustc/ty/layout.rs
Outdated
|
||
// ADTs. | ||
ty::TyAdt(def, _) => { | ||
let v = if def.is_enum() { |
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'll say "what about univariant enums" but you appear to have fixed this in the next commit.
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'll move it back (rebasing was painful when fixing that but now it should be fine.
src/librustc/ty/layout.rs
Outdated
ty::TyNever | | ||
ty::TyFnDef(..) | | ||
ty::TyDynamic(..) => { | ||
ty::TyFnPtr(_) => { | ||
bug!("TyLayout::field_type({:?}): not applicable", self) |
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.
bug
and comment are wrong - function is called field_count
, and TyDynamic
is not a ZST. OTOH, non-ADT/arrays have 0 fields, so presumably you should always return 0 for them (+TyStr
).
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.
It mostly has to match the Layout
variant (maybe I should match on that instead).
src/librustc_trans/abi.rs
Outdated
} | ||
} | ||
RegKind::Vector => { | ||
// Try to pick an integer at least twice as small. |
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.
What's the motivation for this? AFAICT this seems to be used only for SSE/AVX on amd64 SysV, so the if
statement can never be hit. If LLVM really does not care about the type of the vector elements as long as it has the right length, why not use [u8; N]
everywhere? Or [u64; N]
because all SIMD is a multiple of 64 bits.
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'll switch to <n x i8>
, it's the cleanest simple solution.
src/librustc_trans/cabi_x86_64.rs
Outdated
use rustc::ty::layout::{self, Layout, TyLayout, Size}; | ||
|
||
#[derive(Clone, Copy, PartialEq, Debug)] | ||
enum Class { |
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.
Nice to see we can reduce this.
src/librustc_trans/cabi_x86_64.rs
Outdated
SSEDv, | ||
SSEInt(/* bitwidth */ u64), | ||
LoneF32, | ||
Sse, |
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.
Might be worth commenting that this also includes AVX (yes I know the spec calls this Sse).
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.
Also, LoneF32
is also an Sse
in the spec - the only difference is that we want to tell LLVM that only the lowest 32 bits matter. I'll make it a field of the enum.
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.
There's a more elegant way to handle this (last Sse
with only 4 bytes of data left, only way to get LoneF32
anyway, unless you have weird padding). I'll try to implement it.
Also, clang does somewhat complex type recovery - I don't think we care, as this is for FFI.
src/librustc/ty/layout.rs
Outdated
@@ -1513,6 +1531,59 @@ impl<'a, 'gcx, 'tcx> Layout { | |||
} | |||
} | |||
} | |||
|
|||
pub fn field_offset(&self, |
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.
Seems like some comments, or better choice of names, would be good here. e.g. what is i
-- the index of the field, I guess? variant_index
is for structs/enums? (I'd also find the opposite order of those parameters more intuitive, but I can't say why -- I guess because the i
is defined relative to variant_index
)
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.
We'll hopefully eventually remove variant_index
by adding variant types, so I'm not sure how much I care about it.
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.
Most changes should be fixed in a separate PR, but I don't like the "pirate" pair of floats feature in this PR.
} | ||
} | ||
|
||
pub trait LayoutTyper<'tcx>: HasTyCtxt<'tcx> { |
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'm fine with this if this is temporary code.
src/librustc/ty/layout.rs
Outdated
self.layout.field_offset(cx, i, self.variant_index) | ||
} | ||
|
||
pub fn field_count<C: HasTyCtxt<'tcx>>(&self, cx: C) -> usize { |
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.
Can you match on the layout instead?
src/librustc_trans/abi.rs
Outdated
@@ -133,32 +134,279 @@ impl ArgAttributes { | |||
} | |||
} | |||
|
|||
pub trait CastTarget { |
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.
ok
return; | ||
} | ||
|
||
// Pairs of floats. |
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 is not present in the original code. Random micro-optimization?
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.
Oh I meant to find that issue and mention it in the description. This is a long-wanted feature and now it's easy to implement so I just did it when I rewrote that code.
let fields = variant.field_index_by_increasing_offset().map(|i| fields[i as usize]); | ||
if sizing { | ||
fields.filter(|ty| !dst || cx.shared().type_is_sized(*ty)) | ||
.map(|ty| type_of::sizing_type_of(cx, ty)).collect() | ||
bug!() |
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.
nit: remove the sizing
and dst
parameters? They don't seem to be used anywhere.
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.
Have to keep to allow #39999 to rebase.
let size = ret.layout.size(ccx); | ||
let bits = size.bits(); | ||
if bits <= 128 { | ||
let unit = if bits <= 8 { |
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.
can't this logic be lifted to abi
? It appears to be used in several places, including the Rust ABI.
src/librustc_trans/cabi_x86_64.rs
Outdated
SSEDv, | ||
SSEInt(/* bitwidth */ u64), | ||
LoneF32, | ||
Sse, |
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.
Also, LoneF32
is also an Sse
in the spec - the only difference is that we want to tell LLVM that only the lowest 32 bits matter. I'll make it a field of the enum.
src/librustc_trans/cabi_x86_64.rs
Outdated
|
||
#[derive(Clone, Copy, PartialEq, Debug)] | ||
enum Class { | ||
None, |
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 is called NoClass
in the spec. Why not stick to it?
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.
Because Class::NoClass
is unidiomatic.
@@ -121,13 +121,13 @@ pub fn unsafe_slice(_: &[UnsafeInner]) { | |||
fn str(_: &[u8]) { | |||
} | |||
|
|||
// CHECK: @trait_borrow(i8* nonnull, void (i8*)** noalias nonnull readonly) | |||
// CHECK: @trait_borrow({}* nonnull, {}* noalias nonnull readonly) |
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.
Are LLVM's problems with unit pointers gone?
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.
Those problems were only around C FFI were LLVM was looking for a specific signature, IIRC.
i.e. Not unit pointers were the problem but any pointer thst wasn't i8*
.
@bors r+ |
📌 Commit 6ac00de has been approved by |
⌛ Testing commit 6ac00de with merge 0b20dbd... |
💔 Test failed - status-travis |
⌛ Testing commit 6ac00de with merge 5f08d8d... |
💔 Test failed - status-travis |
@bors r=arielb1 |
📌 Commit 1759358 has been approved by |
⌛ Testing commit 1759358 with merge fbe44d3... |
@bors r- retry |
@bors r=arielb1 |
📌 Commit a8dd65a has been approved by |
⌛ Testing commit a8dd65a with merge c5404c5... |
💔 Test failed - status-travis |
@bors retry
|
This is the log of the error (there are multiple like these:
This is not related to the out-of-disk-space errors. There’s still plenty left. cc @alexcrichton this one is new. |
@bors r- Aaaaah there's an LLVM error in there, thanks! |
This appears to be an emscripten bug, which I've filed as emscripten-core/emscripten-fastcomp#178. |
@bors r=arielb1 |
📌 Commit f0636b6 has been approved by |
⌛ Testing commit f0636b6 with merge 2c48ae6... |
Use ty::layout for ABI computation instead of LLVM types. This is the first step in creating a backend-agnostic library for computing call ABI details from signatures. I wanted to open the PR *before* attempting to move `cabi_*` from trans to avoid rebase churn in #39999. **EDIT**: As I suspected, #39999 needs this PR to fully work (see #39999 (comment)). The first 3 commits add more APIs to `ty::layout` and replace non-ABI uses of `sizing_type_of`. These APIs are probably usable by other backends, and miri too (cc @stoklund @solson). The last commit rewrites `rustc_trans::cabi_*` to use `ty::layout` and new `rustc_trans::abi` APIs. Also, during the process, a couple trivial bugs were identified and fixed: * `msp430`, `nvptx`, `nvptx64`: type sizes *in bytes* were compared with `32` and `64` * `x86` (`fastcall`): `f64` was incorrectly not treated the same way as `f32` Although not urgent, this PR also uses the more general "homogenous aggregate" logic to fix #32045.
☀️ Test successful - status-appveyor, status-travis |
Fix pairs of doubles using an illegal <8 x i8> vector. Accidentally introduced in rust-lang#40658 and discovered in some Objective-C bindings (returning `NSPoint`). Turns out LLVM will widen element types of illegal vectors instead of increasing element count, i.e. it will zero-extend `<8 x i8>` to `<8 x i16>`, interleaving the bytes, instead of using the first 8 of `<16 x i8>`.
Fix pairs of doubles using an illegal <8 x i8> vector. Accidentally introduced in rust-lang#40658 and discovered in some Objective-C bindings (returning `NSPoint`). Turns out LLVM will widen element types of illegal vectors instead of increasing element count, i.e. it will zero-extend `<8 x i8>` to `<8 x i16>`, interleaving the bytes, instead of using the first 8 of `<16 x i8>`.
This is the first step in creating a backend-agnostic library for computing call ABI details from signatures.
I wanted to open the PR before attempting to move
cabi_*
from trans to avoid rebase churn in #39999.EDIT: As I suspected, #39999 needs this PR to fully work (see #39999 (comment)).
The first 3 commits add more APIs to
ty::layout
and replace non-ABI uses ofsizing_type_of
.These APIs are probably usable by other backends, and miri too (cc @stoklund @solson).
The last commit rewrites
rustc_trans::cabi_*
to usety::layout
and newrustc_trans::abi
APIs.Also, during the process, a couple trivial bugs were identified and fixed:
msp430
,nvptx
,nvptx64
: type sizes in bytes were compared with32
and64
x86
(fastcall
):f64
was incorrectly not treated the same way asf32
Although not urgent, this PR also uses the more general "homogenous aggregate" logic to fix #32045.