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

Avoid using the copy_nonoverlapping wrapper through mem::replace. #87827

Merged
merged 1 commit into from
Aug 8, 2021

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Aug 6, 2021

This is a much simpler way to achieve the pre-#86003 behavior of mem::replace not needing dynamically-sized memcpys (at least before inlining), than re-doing #81238 (which needs #86699 or something similar).

I didn't notice it until recently, but ptr::write already explicitly avoided using the wrapper, while ptr::read just called the wrapper (and was the reason for us observing any behavior change from #86003 in Rust-GPU).


The codegen test I've added fails without the change to core::ptr::read like this (ignore the v0 mangling, I was using a worktree with it turned on by default, for this):

       13: ; core::intrinsics::copy_nonoverlapping::<u8>
       14: ; Function Attrs: inlinehint nonlazybind uwtable
       15: define internal void @_RINvNtCscK5tvALCJol_4core10intrinsics19copy_nonoverlappinghECsaS4X3EinRE8_25mem_replace_direct_memcpy(i8* %src, i8* %dst, i64 %count) unnamed_addr #0 {
       16: start:
       17:  %0 = mul i64 %count, 1
       18:  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 1 %dst, i8* align 1 %src, i64 %0, i1 false)
not:17      !~~~~~~~~~~~~~~~~~~~~~                                                                     error: no match expected
       19:  ret void
       20: }

With the core::ptr::read change, core::intrinsics::copy_nonoverlapping doesn't get instantiated and the test passes.


r? @m-ou-se cc @nagisa (codegen test) @oli-obk / @RalfJung (miri diagnostic changes)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 6, 2021
@nagisa
Copy link
Member

nagisa commented Aug 6, 2021

Looks reasonable enough to me.

@RalfJung
Copy link
Member

RalfJung commented Aug 7, 2021

Yeah this makes sense.

@m-ou-se
Copy link
Member

m-ou-se commented Aug 8, 2021

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Aug 8, 2021

📌 Commit d3dd6cb11db296a59dc0997db65b32403c930cd3 has been approved by m-ou-se

@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 Aug 8, 2021
@m-ou-se m-ou-se added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Aug 8, 2021
@bors
Copy link
Contributor

bors commented Aug 8, 2021

⌛ Testing commit d3dd6cb11db296a59dc0997db65b32403c930cd3 with merge bba5126c336dad13ada2c34b7425aaf94d01c1d9...

@rust-log-analyzer

This comment has been minimized.

@bors

This comment has been minimized.

@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 Aug 8, 2021
@eddyb
Copy link
Member Author

eddyb commented Aug 8, 2021

@bors r=m-ou-se

@bors
Copy link
Contributor

bors commented Aug 8, 2021

📌 Commit a1d014b has been approved by m-ou-se

@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 Aug 8, 2021
@bors
Copy link
Contributor

bors commented Aug 8, 2021

⌛ Testing commit a1d014b with merge 4e886d6...

@bors
Copy link
Contributor

bors commented Aug 8, 2021

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing 4e886d6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2021
@bors bors merged commit 4e886d6 into rust-lang:master Aug 8, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 8, 2021
@eddyb eddyb deleted the wrapperless-mem-replace branch August 8, 2021 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants