Skip to content

Specialize slice::fill with Copy type and u8/i8/bool #81874

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

Merged
merged 2 commits into from
Feb 27, 2021
Merged

Specialize slice::fill with Copy type and u8/i8/bool #81874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
446798f
fix bootstrap
tesuji Feb 8, 2021
010e194
Specialize slice::fill for Copy type and u8/i8/bool
tesuji Feb 8, 2021
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
9 changes: 2 additions & 7 deletions library/core/src/slice/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ mod iter;
mod raw;
mod rotate;
mod sort;
mod specialize;

#[stable(feature = "rust1", since = "1.0.0")]
pub use iter::{Chunks, ChunksMut, Windows};
Expand Down Expand Up @@ -2831,13 +2832,7 @@ impl<T> [T] {
where
T: Clone,
{
if let Some((last, elems)) = self.split_last_mut() {
for el in elems {
el.clone_from(&value);
}

*last = value
}
specialize::SpecFill::spec_fill(self, value);
}

/// Fills `self` with elements returned by calling a closure repeatedly.
Expand Down
58 changes: 58 additions & 0 deletions library/core/src/slice/specialize.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
use crate::ptr::write_bytes;

pub(super) trait SpecFill<T> {
fn spec_fill(&mut self, value: T);
}

impl<T: Clone> SpecFill<T> for [T] {
default fn spec_fill(&mut self, value: T) {
if let Some((last, elems)) = self.split_last_mut() {
for el in elems {
el.clone_from(&value);
}

*last = value
}
}
}

impl<T: Copy> SpecFill<T> for [T] {
default fn spec_fill(&mut self, value: T) {
for item in self.iter_mut() {
*item = value;
}
}
Copy link
Member

@the8472 the8472 Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Instead of specializing separately on [u8] below you can do a if mem::size_of::<T>() == 1 here and call memset for any type that's 1 byte wide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, that's another option. Will do that! Thanks.

Copy link
Contributor Author

@tesuji tesuji Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

value now has 1-byte size. ptr::write_bytes only accepts u8 as second argument.
Should I transmute value to u8 ? Is it sound ?

Copy link
Contributor Author

@tesuji tesuji Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Even if I want to transmute, the compiler forbids me: https://rust.godbolt.org/z/a3bsYs

error[E0512]: cannot transmute between types of different sizes, or dependently-sized types
  --> <source>:38:33
   |
38 |                 let value: u8 = std::mem::transmute(value);
   |                                 ^^^^^^^^^^^^^^^^^^^
   |
   = note: source type: `T` (this type does not have a fixed size)
   = note: target type: `u8` (8 bits)

Copy link
Member

Choose a reason for hiding this comment

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

transmute_copy should work

Copy link
Contributor Author

@tesuji tesuji Feb 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Neat! That's really work! But I wonder if that those specific specialization for [u8] matters much with #81874 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

The advantage is that it doesn't rely on the optimizer, so it'll work in debug mode too or with less powerful backends.

With the size_of approach it only has to be implemented once instead of one specialization for each T.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have strong opinion on this. I defer that to reviewer to decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @bjorn3. the8472 and I were discussing if other codegen backends could handle straight
for loop to copy item and make it to memcpy when item is 1-byte size.

Do you think cranelift could be benefited from this optimization ^^?

}

impl SpecFill<u8> for [u8] {
Copy link
Contributor Author

@tesuji tesuji Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The only difference when not specialize for [u8] is that compiler emits a check whether the slice is empty
before the call to memcpy.
https://rust.godbolt.org/z/z9fbcT.

Maybe it is not worth to keep those specs for u8/i8/bool slice.

fn spec_fill(&mut self, value: u8) {
// SAFETY: this is slice of u8
unsafe {
let ptr = self.as_mut_ptr();
let len = self.len();
write_bytes(ptr, value, len);
}
}
}

impl SpecFill<i8> for [i8] {
fn spec_fill(&mut self, value: i8) {
// SAFETY: this is slice of i8
unsafe {
let ptr = self.as_mut_ptr();
let len = self.len();
write_bytes(ptr, value as u8, len);
}
}
}

impl SpecFill<bool> for [bool] {
fn spec_fill(&mut self, value: bool) {
// SAFETY: this is slice of bool
unsafe {
let ptr = self.as_mut_ptr();
let len = self.len();
write_bytes(ptr, value as u8, len);
}
}
}
2 changes: 1 addition & 1 deletion src/bootstrap/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1490,7 +1490,7 @@ impl<'a> Builder<'a> {
for el in stack.iter().rev() {
out += &format!("\t{:?}\n", el);
}
panic!(out);
panic!("{}", out);
}
if let Some(out) = self.cache.get(&step) {
self.verbose(&format!("{}c {:?}", " ".repeat(stack.len()), step));
Expand Down