-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
change offset from u32 to u64 #71696
Conversation
meh me |
r? @oli-obk |
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 |
src/librustc_mir/util/aggregate.rs
Outdated
@@ -53,13 +53,12 @@ pub fn expand_aggregate<'tcx>( | |||
.map(move |(i, (op, ty))| { | |||
let lhs_field = if let AggregateKind::Array(_) = kind { | |||
// FIXME(eddyb) `offset` should be 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.
// FIXME(eddyb) `offset` should be u64. |
@@ -675,7 +675,7 @@ where | |||
let tcx = self.tcx(); | |||
|
|||
if let Some(size) = opt_size { | |||
let size: u32 = size.try_into().unwrap_or_else(|_| { | |||
let size: u64 = size.try_into().unwrap_or_else(|_| { | |||
bug!("move out check isn't implemented for array sizes bigger than u32::MAX"); |
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!("move out check isn't implemented for array sizes bigger than u32::MAX"); | |
bug!("move out check isn't implemented for array sizes bigger than u64::MAX"); |
That's what check_if_subslice_element_is_moved
does, right?
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 entire check can be removed, opt_size
is Option<u64>
so the size
from if let Some(size)
is already u64
.
We should make sure to run this by perf before landing, as it does change the size of some things that are plausibly in the hot path. I suspect that arrays this size are really slow to compile anyway today, at least if you perform some operations on them (such as moving out). |
@@ -645,7 +645,7 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> { | |||
PlaceTy::from_ty(match base_ty.kind { | |||
ty::Array(inner, _) => { | |||
assert!(!from_end, "array subslices should not use from_end"); | |||
tcx.mk_array(inner, (to - from) as u64) | |||
tcx.mk_array(inner, (to - from)) |
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.
Unnecessary parens.
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.
tcx.mk_array(inner, (to - from)) | |
tcx.mk_array(inner, to - from) |
let offset = i as u64; | ||
assert_eq!(offset as usize, i); |
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.
AFAIK this dance isn't needed anymore, since usize
is at most u64
currently, but cc @RalfJung who removed a bunch of as
casts from miri recently.
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'd use u64::try_from(...).unwrap()
instead of a manual assert_eq!
.
I changed Size::from_bytes
so that it does the (checked) cast itself, but here it looks like we need it for ConstantIndex
and I didn't touch that.
@@ -1804,7 +1804,7 @@ pub enum ProjectionElem<V, T> { | |||
/// ``` | |||
ConstantIndex { | |||
/// index or -index (in Python terms), depending on from_end | |||
offset: u32, | |||
offset: 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.
I think you can now also remove a bunch of conversions around here:
rust/src/librustc_mir/interpret/place.rs
Line 541 in bd0bacc
ConstantIndex { offset, min_length, from_end } => { |
068c50e
to
5038a0e
Compare
Marking this as draft till i'm sure I haven't missed anything :D |
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 |
5038a0e
to
9f39396
Compare
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 |
@@ -1861,7 +1861,7 @@ impl<'tcx> Copy for PlaceElem<'tcx> {} | |||
|
|||
// At least on 64 bit systems, `PlaceElem` should not be larger than two pointers. |
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 PR seems to make this comment out of date?
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 changing it, I haven't pushed it yet
☔ The latest upstream changes (presumably #72330) made this pull request unmergeable. Please resolve the merge conflicts. |
@Dylan-DPC this is a triage bump. |
closing in favour of #74848 |
change offset from u32 to u64 References rust-lang#71696 r? @oli-obk (closed the earlier pr because the rebase got messed up)
Changes offset of u32 to u64
Resolves one of eddy's FIXMEs
References #71699
r? @eddyb