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

For MIRI, cfg out the swap vectorization logic from 94212 #94412

Merged
merged 1 commit into from
Feb 27, 2022
Merged

For MIRI, cfg out the swap vectorization logic from 94212 #94412

merged 1 commit into from
Feb 27, 2022

Conversation

scottmcm
Copy link
Member

Because of #69488 the swap logic from #94212 doesn't currently work in MIRI.

Copying in smaller pieces is probably much worse for its performance anyway, so it'd probably rather just use the simple path regardless.

Part of #94371, though another PR will be needed for the CTFE aspect.

r? @oli-obk
cc @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 27, 2022
RalfJung
RalfJung reviewed Feb 27, 2022
View reviewed changes
library/core/src/mem/mod.rs
let a = ptr::read(x);
let b = ptr::read(y);
ptr::write(x, b);
ptr::write(y, a);
Copy link
Member

Choose a reason for hiding this comment

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

However, in Miri this will mean we are copying 4 times rather than 3 times. No idea if that's a problem for performance or not...

@RalfJung
Copy link
Member

I agree this makes sense for Miri. Not sure if there is a nice way to also use this for CTFE without code duplication.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 27, 2022

I think we should just fix the FIXME mentioned right there:

the block optimization also prevents MIR optimizations from
// understanding mem::replace, Option::take, etc. - a better overall
// solution might be to make ptr::swap_nonoverlapping into an intrinsic, which
// a backend can choose to implement using the block optimization, or not.

This will take care of miri, CTFE, spirv in one go

@oli-obk
Copy link
Contributor

oli-obk commented Feb 27, 2022

The "right" solution for that is probably to not use an intrinsic, but a lang item. So the default logic is swap_simple, but backends can intercept the lang item and generate an entirely different body for it

@oli-obk
Copy link
Contributor

oli-obk commented Feb 27, 2022

Ideally we'd have

working by now, but... that needs a bit more work.

@RalfJung
Copy link
Member

The "right" solution for that is probably to not use an intrinsic, but a lang item. So the default logic is swap_simple, but backends can intercept the lang item and generate an entirely different body for it

Yeah, I was about to say that the logic that is currently implemented here would probably be quite annoying to replicate in a backend.

But also, Miri is currently broken by 3 independent issues, and we need to resolve this one to make progress on that. So given that, I think it would make sense to land this or something like #94411 rather soon. (In CTFE it only affects unstable features so hopefully it is a bit less pressing.)

@oli-obk
Copy link
Contributor

oli-obk commented Feb 27, 2022

@bors r+

Agreed, we can fix it cleaner later, the FIXME stays in place after all, this PR doesn't make the situation any worse

@bors
Copy link
Contributor

bors commented Feb 27, 2022

📌 Commit b582bd3 has been approved by oli-obk

@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 Feb 27, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 27, 2022
@Dylan-DPC
Rollup merge of rust-lang#94412 - scottmcm:cfg-out-miri-from-swap, r=…
92ad84d 
…oli-obk

For MIRI, cfg out the swap vectorization logic from 94212

Because of rust-lang#69488 the swap logic from rust-lang#94212 doesn't currently work in MIRI.

Copying in smaller pieces is probably much worse for its performance anyway, so it'd probably rather just use the simple path regardless.

Part of rust-lang#94371, though another PR will be needed for the CTFE aspect.

r? `@oli-obk`
cc `@RalfJung`
@Dylan-DPC Dylan-DPC mentioned this pull request Feb 27, 2022
Closed
@RalfJung
Copy link
Member

RalfJung commented Feb 27, 2022 •
edited
Loading

@bors r=oli-obk p=1

@bors
Copy link
Contributor

bors commented Feb 27, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 27, 2022

📌 Commit b582bd3 has been approved by 1

@RalfJung
Copy link
Member

oops
@bors r=oli-obk p=1

@bors
Copy link
Contributor

bors commented Feb 27, 2022

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Feb 27, 2022

📌 Commit b582bd3 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Feb 27, 2022

⌛ Testing commit b582bd3 with merge 6a70556...

@bors
Copy link
Contributor

bors commented Feb 27, 2022

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 6a70556 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 27, 2022
@bors bors merged commit 6a70556 into rust-lang:master Feb 27, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 27, 2022
@RalfJung RalfJung mentioned this pull request Feb 27, 2022
Merged
@bors bors mentioned this pull request Feb 27, 2022
Merged
@scottmcm scottmcm deleted the cfg-out-miri-from-swap branch February 27, 2022 21:25
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6a70556): comparison url.

Summary: This benchmark run shows 4 relevant regressions 😿 to instruction counts.

  • Arithmetic mean of relevant regressions: 0.8%
  • Largest regression in instruction counts: 0.9% on incr-unchanged builds of externs debug

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

@rustbot rustbot added the perf-regression Performance regression. label Feb 27, 2022
@Mark-Simulacrum
Copy link
Member

Dropping the regression label -- externs is currently a little noisy (and we haven't absorbed that noisiness into our statistical estimation yet). Seems to be due to #93839, which makes me suspect this is related to PGO or inlining decisions of some kind. #94373 might help.

@Mark-Simulacrum Mark-Simulacrum removed the perf-regression Performance regression. label Mar 1, 2022
@scottmcm scottmcm mentioned this pull request Mar 1, 2022
Closed
@RalfJung RalfJung mentioned this pull request Apr 5, 2022
Closed
@compiler-errors compiler-errors mentioned this pull request Nov 4, 2022
Closed
This was referenced Oct 1, 2023
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

Assignees

@oli-obk oli-obk

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
8 participants
@scottmcm @RalfJung @oli-obk @bors @rust-timer @Mark-Simulacrum @rust-highfive @rustbot