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.

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

make memrchr use align_offset #52744

Merged
merged 2 commits into from
Jul 28, 2018
Merged

make memrchr use align_offset #52744

merged 2 commits into from
Jul 28, 2018

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jul 26, 2018

I hope I did not screw that up...

Cc @oli-obk who authored the original #44537

Fixes #50567 (thanks @bjorn3)

@rust-highfive
Copy link
Collaborator

r? @Kimundi

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 26, 2018
// a version of align_offset that says how much must be *subtracted*
// from a pointer to be aligned.
#[inline(always)]
fn align_offset_down(ptr: *const u8, align: usize) -> usize {
Copy link
Member

@nagisa nagisa Jul 26, 2018

Choose a reason for hiding this comment

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

Consider slice::align_to::<usize>()? I believe that does exactly what a large part of this code does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, plausible. Should I then also do that for memchr?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually it's not so easy, we want to do the main loop on a u8 slice, not usize (or actually pairs of usize, which is the steps that the loop is taking).

0
} else {
// E.g. if align=8 and we have to add 1, then we can also subtract 7.
align - align_offset
Copy link
Member

Choose a reason for hiding this comment

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

You probably want (align - align_offset) & (align - 1) (or % align if you're sure it will optimize) instead of the last if/else. Not sure about the first one though, seems kinda fine as-is.

@RalfJung
Copy link
Member Author

Now we just have to hope that align_to gets inline, but that seems to be the case.

@Kimundi
Copy link
Member

Kimundi commented Jul 27, 2018

@bors: r+

@bors
Copy link
Contributor

bors commented Jul 27, 2018

📌 Commit 3bc59b5 has been approved by Kimundi

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2018
@bjorn3
Copy link
Member

bjorn3 commented Jul 27, 2018

closes #50567

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 27, 2018
make memrchr use align_offset

I hope I did not screw that up...

Cc @oli-obk who authored the original rust-lang#44537
@bors
Copy link
Contributor

bors commented Jul 28, 2018

⌛ Testing commit 3bc59b5 with merge d754582...

bors added a commit that referenced this pull request Jul 28, 2018
make memrchr use align_offset

I hope I did not screw that up...

Cc @oli-obk who authored the original #44537

Fixes #50567 (thanks @bjorn3)
@bors
Copy link
Contributor

bors commented Jul 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: Kimundi
Pushing d754582 to master...

@bors bors merged commit 3bc59b5 into rust-lang:master Jul 28, 2018
@RalfJung RalfJung mentioned this pull request Jul 30, 2018
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Aug 1, 2018
fix memrchr in miri

The previous PR rust-lang#52744 was not enough because it assumed that the split between the `mid` and `end` parts returned by `align_to` was aligned. But really the only guarantee we have is that the `mid` part is aligned, so make use of that.
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Aug 1, 2018
fix memrchr in miri

The previous PR rust-lang#52744 was not enough because it assumed that the split between the `mid` and `end` parts returned by `align_to` was aligned. But really the only guarantee we have is that the `mid` part is aligned, so make use of that.
@RalfJung RalfJung deleted the align_offset branch August 16, 2018 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants