-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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 copy/copy_nonoverlapping fn's again #86003
Conversation
This is preparation for reverting 81238 for short-term resolution of issue 84297.
Some changes occurred in src/tools/clippy. cc @rust-lang/clippy |
r? @yaahc (rust-highfive has picked a reviewer for you, use r? to override) |
oh wait I need a regression test too. :) |
This comment has been minimized.
This comment has been minimized.
d9ff911
to
1111edc
Compare
This comment has been minimized.
This comment has been minimized.
weird, my local build+tests passed. Looking now. Update: Oh, my local build driver script only runs |
…ntrinsics). This is to address issue 84297.
…pping`. Test was added in PR rust-lang#84404. The intent here is: The `copy`/`copy_overlapping` intrinsics are going through some flip-flopping now of "are they intrinsics or not". We can achieve the same effect that the test intended by using `likely`/`unlikely`.
(for the short term, that is; see issue 84297.)
85cb915
to
f08f933
Compare
@bors r+ p=1 Mostly just reverting some stuff, including unstable const-fn-ing, but seems good. |
📌 Commit f08f933 has been approved by |
⌛ Testing commit f08f933 with merge 957453fc62205d24f9c5e331c4b68fe802e83797... |
💔 Test failed - checks-actions |
…r=dtolnay Beta targetted Make copy/copy_nonoverlapping fn's again beta backport of PR rust-lang#86003 to address issue rust-lang#84297
beta backported in #86203 |
@pnkfelix @Mark-Simulacrum This change led to some moderate performance regressions. I imagine performance regressions are somewhat expected going from intrinsics to function calls as it seems LLVM might have more to process now. A quick look at where the regressions are happening shows it to be mostly in codegen. Any thoughts? |
This was a stopgap measure to fix regressions; it should be fixable in the long run. I don't think it makes sense to prioritize the work too much though. |
…Mark-Simulacrum Make copy/copy_nonoverlapping fn's again Make copy/copy_nonoverlapping fn's again, rather than intrinsics. This a short-term change to address issue rust-lang#84297. It effectively reverts PRs rust-lang#81167 rust-lang#81238 (and part of rust-lang#82967), rust-lang#83091, and parts of rust-lang#79684.
…r=RalfJung Revert revert of constness in rust-lang#86003 Re-constify `mem::swap`, `mem::replace`, `ptr::write` which were marked as not `const` in rust-lang#86003 Once the checks pass, this should solve rust-lang#86236
Avoid using the `copy_nonoverlapping` wrapper through `mem::replace`. This is a much simpler way to achieve the pre-rust-lang#86003 behavior of `mem::replace` not needing dynamically-sized `memcpy`s (at least before inlining), than re-doing rust-lang#81238 (which needs rust-lang#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 rust-lang#86003 in Rust-GPU). <hr/> 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): ```llvm 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. <hr/> r? `@m-ou-se` cc `@nagisa` (codegen test) `@oli-obk` / `@RalfJung` (miri diagnostic changes)
Make copy/copy_nonoverlapping fn's again, rather than intrinsics.
This a short-term change to address issue #84297.
It effectively reverts PRs #81167 #81238 (and part of #82967), #83091, and parts of #79684.