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

Add a special case for align_offset /w stride != 1 #98866

Merged
merged 1 commit into from
Jul 17, 2022
Merged

Add a special case for align_offset /w stride != 1 #98866

merged 1 commit into from
Jul 17, 2022

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jul 3, 2022

This generalizes the previous stride == 1 special case to apply to any
situation where the requested alignment is divisible by the stride. This
in turn allows the test case from #98809 produce ideal assembly, along
the lines of:

leaq 15(%rdi), %rax
andq $-16, %rax

This also produces pretty high quality code for situations where the
alignment of the input pointer isn’t known:

pub unsafe fn ptr_u32(slice: *const u32) -> *const u32 {
    slice.offset(slice.align_offset(16) as isize)
}

// =>

movl %edi, %eax
andl $3, %eax
leaq 15(%rdi), %rcx
andq $-16, %rcx
subq %rdi, %rcx
shrq $2, %rcx
negq %rax
sbbq %rax, %rax
orq  %rcx, %rax
leaq (%rdi,%rax,4), %rax

Here LLVM is smart enough to replace the usize::MAX special case with
a branch-less bitwise-OR approach, where the mask is constructed using
the neg and sbb instructions. This appears to work across various
architectures I’ve tried.

This change ends up introducing more branches and code in situations
where there is less knowledge of the arguments. For example when the
requested alignment is entirely unknown. This use-case was never really
a focus of this function, so I’m not particularly worried, especially
since llvm-mca is saying that the new code is still appreciably faster,
despite all the new branching.

Fixes #98809.
Sadly, this does not help with #72356.

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jul 3, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jul 3, 2022

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @Mark-Simulacrum

(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 3, 2022
@nagisa nagisa force-pushed the nagisa/align-offset-wroom branch from f495c37 to 3d98adb Compare July 3, 2022 22:18
nagisa
nagisa commented Jul 6, 2022
View reviewed changes
library/core/src/ptr/mod.rs
let byte_offset = wrapping_sub(aligned_address, addr);
// SAFETY: `stride` is non-zero. This is guaranteed to divide exactly as well, because
// addr has been verified to be aligned to the original type’s alignment requirements.
unsafe { exact_div(byte_offset, stride) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about it again, there may be a better way to compute the element offset in the first place rather than dividing the byte offset. I believe this won’t regress stride == 1 case anyway and this division won’t actually appear in most of the code that immediately passes the result from this function to an offset, either so I wouldn’t consider it a blocker for the time being.

@Mark-Simulacrum
Copy link
Member

r=me modulo perf:

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jul 14, 2022
@bors
Copy link
Contributor

bors commented Jul 14, 2022

⌛ Trying commit 3d98adb558215e0f5078c33d60760bad1295481b with merge da5de824feffc6dd3116d84c8dbc8083dcd61358...

@Mark-Simulacrum
Copy link
Member

It might also make sense to add an asm or LLVM test verifying the optimization here is working, so that we don't unintentionally regress in the future.

@Mark-Simulacrum Mark-Simulacrum removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 14, 2022
@bors
Copy link
Contributor

bors commented Jul 14, 2022

☀️ Try build successful - checks-actions
Build commit: da5de824feffc6dd3116d84c8dbc8083dcd61358 (da5de824feffc6dd3116d84c8dbc8083dcd61358)

@rust-timer
Copy link
Collaborator

Queued da5de824feffc6dd3116d84c8dbc8083dcd61358 with parent 87588a2, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (da5de824feffc6dd3116d84c8dbc8083dcd61358): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 1.1% 1.1% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -2.7% -3.5% 3
All 😿🎉 (primary) N/A N/A 0

Cycles

Results
  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.5% 2.5% 1
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 -4.1% -4.1% 1
All 😿🎉 (primary) N/A N/A 0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2

  2. number of relevant changes ↩ ↩2

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jul 14, 2022
@Mark-Simulacrum
Copy link
Member

I'll hold off on approving in case you want to add the codegen test:

It might also make sense to add an asm or LLVM test verifying the optimization here is working, so that we don't unintentionally regress in the future.

but otherwise r=me, perf looks neutral (but not unexpected, this kind of thing is likely to have marginal impact at best for something as large as rustc).

@nagisa nagisa force-pushed the nagisa/align-offset-wroom branch from 3d98adb to 36a96bb Compare July 16, 2022 12:56
@nagisa
Copy link
Member Author

nagisa commented Jul 16, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit 36a96bb6f67d2c48a36988d742cba02003eaab98 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 16, 2022
@bors
Copy link
Contributor

bors commented Jul 16, 2022

⌛ Testing commit 36a96bb6f67d2c48a36988d742cba02003eaab98 with merge fc0ddaf7ea2f0db718d1bdb7693eabf84d197611...

@bors
Copy link
Contributor

bors commented Jul 16, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jul 16, 2022
@nagisa nagisa force-pushed the nagisa/align-offset-wroom branch from 36a96bb to fe2a05e Compare July 16, 2022 20:37
@nagisa
Copy link
Member Author

nagisa commented Jul 16, 2022

@bors r=Mark-Simulacrum

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit fe2a05e53ef813151f6314a8bfa57c790fafec6e has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@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 16, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa
Add a special case for align_offset /w stride != 1
62a182c 
This generalizes the previous `stride == 1` special case to apply to any
situation where the requested alignment is divisible by the stride. This
in turn allows the test case from rust-lang#98809 produce ideal assembly, along
the lines of:

    leaq 15(%rdi), %rax
    andq $-16, %rax

This also produces pretty high quality code for situations where the
alignment of the input pointer isn’t known:

    pub unsafe fn ptr_u32(slice: *const u32) -> *const u32 {
        slice.offset(slice.align_offset(16) as isize)
    }

    // =>

    movl %edi, %eax
    andl $3, %eax
    leaq 15(%rdi), %rcx
    andq $-16, %rcx
    subq %rdi, %rcx
    shrq $2, %rcx
    negq %rax
    sbbq %rax, %rax
    orq  %rcx, %rax
    leaq (%rdi,%rax,4), %rax

Here LLVM is smart enough to replace the `usize::MAX` special case with
a branch-less bitwise-OR approach, where the mask is constructed using
the neg and sbb instructions. This appears to work across various
architectures I’ve tried.

This change ends up introducing more branches and code in situations
where there is less knowledge of the arguments. For example when the
requested alignment is entirely unknown. This use-case was never really
a focus of this function, so I’m not particularly worried, especially
since llvm-mca is saying that the new code is still appreciably faster,
despite all the new branching.

Fixes rust-lang#98809.
Sadly, this does not help with rust-lang#72356.
@nagisa nagisa force-pushed the nagisa/align-offset-wroom branch from fe2a05e to 62a182c Compare July 16, 2022 22:27
@nagisa
Copy link
Member Author

nagisa commented Jul 16, 2022

@bors r=Mark-Simulacrum

The test is definitely turning out to be as finicky as I feared it would be.

@bors
Copy link
Contributor

bors commented Jul 16, 2022

📌 Commit 62a182c has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 16, 2022

⌛ Testing commit 62a182c with merge db41351...

@bors
Copy link
Contributor

bors commented Jul 17, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing db41351 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 17, 2022
@bors bors merged commit db41351 into rust-lang:master Jul 17, 2022
@rustbot rustbot added this to the 1.64.0 milestone Jul 17, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (db41351): comparison url.

Instruction count

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: 😿 relevant regressions found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 2.3% 3.0% 6
Improvements 🎉
(primary)		 N/A N/A 0
Improvements 🎉
(secondary)		 N/A N/A 0
All 😿🎉 (primary) N/A N/A 0

Max RSS (memory usage)

Results
  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: 🎉 relevant improvements found
mean1 max count2
Regressions 😿
(primary)		 N/A N/A 0
Regressions 😿
(secondary)		 N/A N/A 0
Improvements 🎉
(primary)		 -3.3% -3.3% 1
Improvements 🎉
(secondary)		 -4.5% -9.4% 6
All 😿🎉 (primary) -3.3% -3.3% 1

Cycles

Results
  • Primary benchmarks: mixed results
  • Secondary benchmarks: mixed results
mean1 max count2
Regressions 😿
(primary)		 2.4% 2.4% 1
Regressions 😿
(secondary)		 3.0% 3.8% 3
Improvements 🎉
(primary)		 -2.5% -2.5% 1
Improvements 🎉
(secondary)		 -2.5% -2.5% 1
All 😿🎉 (primary) -0.1% -2.5% 2

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

Footnotes

  1. the arithmetic mean of the percent change ↩ ↩2 ↩3

  2. number of relevant changes ↩ ↩2 ↩3

@rustbot rustbot added the perf-regression Performance regression. label Jul 17, 2022
@rylev
Copy link
Member

rylev commented Jul 19, 2022 •
edited
Loading

Given this is a change in library code, and it only impacts one secondary benchmark (deeply-nested-multi) , I'm going to mark this as triaged.

@rustbot label: +perf-regression-triaged

Edit: it looks like it's just noise, corrected from the previous run.

@rustbot rustbot added the perf-regression-triaged The performance regression has been triaged. label Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
merged-by-bors This PR was explicitly merged by bors. perf-regression Performance regression. perf-regression-triaged The performance regression has been triaged. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.64.0
Development

Successfully merging this pull request may close these issues.

align_offset seems to generate worse code than is desirable (unless size_of::<T>() == 1)
8 participants
@nagisa @rustbot @rust-highfive @Mark-Simulacrum @rust-timer @bors @rust-log-analyzer @rylev