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

Jump to bottom

Speed up compilation of large constant arrays #51833

Merged
merged 9 commits into from
Jul 1, 2018
Merged

Speed up compilation of large constant arrays #51833

Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
202aea5
Speed up compilation of large constant arrays
wesleywiser Jun 27, 2018
63ab0cb
Inline `abi::Size::bytes()`
wesleywiser Jun 27, 2018
429bc8d
Inline all methods on `abi::Size`
wesleywiser Jun 27, 2018
1ffa99d
Optimize `copy_undef_mask()` by lifting some loop invariant operations
wesleywiser Jun 30, 2018
8f969ed
Optimize `copy_undef_mask()` to use one pass
wesleywiser Jun 30, 2018
c431f3f
Inline a few `UndefMask` methods.
wesleywiser Jun 30, 2018
84fe0c4
Fix relocations to include repeated values
wesleywiser Jun 30, 2018
faef6a3
Copy undef_masks correctly for repeated bytes
wesleywiser Jun 30, 2018
46512e0
Add two regression tests for const eval
wesleywiser Jun 30, 2018
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
25 changes: 8 additions & 17 deletions src/librustc_mir/interpret/memory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -882,25 +882,16 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> Memory<'a, 'mir, 'tcx, M> {
) -> EvalResult<'tcx> {
// The bits have to be saved locally before writing to dest in case src and dest overlap.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment makes me think that we should not do this commit, otherwise we'll run into trouble in the future (and in miri right now). Can you do an if for whether there is overlap and if there is, just run the old code?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. I thought I preserved the existing behavior by cloning the source allocation's undef_mask before writing to the destination's. Is that sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh right. sorry. I misread the code.

I still think the code isn't doing the right thing. It's only copying once, when it should be copying N-1 times.

You can try this out by creating an array of types with padding, everything starting at the third element will probably not have undef masks for the padding. (you'll need unions to get the bits and then attempt to use them for an array length to actually get a compiler error from that)

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid I'm not quite following. We do call this function with size * length so shouldn't it cover all of the repeated copies? Can you provide a sample program that will fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, you are using the length, but that just means that the entire array is copied from 0..N to 1..=N, not that the 1st element is copied N times.

I'll make a regression test

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly certain that the following test will succeed to compile on your PR: http://play.rust-lang.org/?gist=1d0183fcfb65164d1ca58ccd9614c33c

assert_eq!(size.bytes() as usize as u64, size.bytes());
let mut v = Vec::with_capacity(size.bytes() as usize);

{
let src_allocation = self.get(src.alloc_id)?;
for i in 0..size.bytes() {
let defined = src_allocation.undef_mask.get(src.offset + Size::from_bytes(i));
v.push(defined);
}
}
let undef_mask = self.get(src.alloc_id)?.undef_mask.clone();
let dest_allocation = self.get_mut(dest.alloc_id)?;

{
let dest_allocation = self.get_mut(dest.alloc_id)?;
for (i, defined) in v.into_iter().enumerate() {
dest_allocation.undef_mask.set(
dest.offset +
Size::from_bytes(i as u64),
defined,
);
}
for i in 0..size.bytes() {
let defined = undef_mask.get(src.offset + Size::from_bytes(i));
Copy link
Contributor

@oli-obk oli-obk Jun 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

if you pass a repeat counter to the function, you should be able to just modulo the i here over the size and have the for loop go from 0 ot size.bytes() * repeat

dest_allocation.undef_mask.set(
dest.offset + Size::from_bytes(i),
defined
);
}

Ok(())
Expand Down