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 copy/copy_nonoverlapping fn's again #86003

Merged
merged 8 commits into from
Jun 9, 2021

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jun 4, 2021

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.

This is preparation for reverting 81238 for short-term resolution of issue 84297.
@rust-highfive
Copy link
Collaborator

Some changes occurred in src/tools/clippy.

cc @rust-lang/clippy

@rust-highfive
Copy link
Collaborator

r? @yaahc

(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 Jun 4, 2021
@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2021

oh wait I need a regression test too. :)

@rust-log-analyzer

This comment has been minimized.

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2021

r? @Mark-Simulacrum

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. beta-nominated Nominated for backporting to the compiler in the beta channel. labels Jun 4, 2021
@rust-log-analyzer

This comment has been minimized.

@pnkfelix
Copy link
Member Author

pnkfelix commented Jun 4, 2021

The job mingw-check failed! Check out the build log: (web) (plain)
Click to see the possible cause of the failure (guessed by this bot)

configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
    Checking rand v0.7.3
    Checking alloc v0.0.0 (/checkout/library/alloc)
    Checking std v0.0.0 (/checkout/library/std)
    Checking core v0.0.0 (/checkout/library/core)
error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
   |
   |
60 |             ptr::write(&mut res as *mut _, 42);


error[E0015]: calls in constant functions are limited to constant functions, tuple structs and tuple variants
   |
   |
84 |             (&mut res as *mut i32).write(42);

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0015`.
For more information about this error, try `rustc --explain E0015`.
error: could not compile `core`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--all-targets" "-p" "test" "-p" "panic_abort" "-p" "alloc" "-p" "core" "-p" "panic_unwind" "-p" "unwind" "-p" "term" "-p" "std" "-p" "proc_macro" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:31

weird, my local build+tests passed. Looking now.

Update: Oh, my local build driver script only runs x.py test on stuff under src/test/. Heh, whoops.

…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.)
@Mark-Simulacrum
Copy link
Member

@bors r+ p=1

Mostly just reverting some stuff, including unstable const-fn-ing, but seems good.

@bors
Copy link
Contributor

bors commented Jun 8, 2021

📌 Commit f08f933 has been approved by Mark-Simulacrum

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

bors commented Jun 8, 2021

⌛ Testing commit f08f933 with merge 957453fc62205d24f9c5e331c4b68fe802e83797...

@bors
Copy link
Contributor

bors commented Jun 8, 2021

💔 Test failed - checks-actions

@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 10, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 11, 2021
…r=dtolnay

Beta targetted Make copy/copy_nonoverlapping fn's again

beta backport of PR rust-lang#86003

to address issue rust-lang#84297
@Mark-Simulacrum Mark-Simulacrum modified the milestones: 1.54.0, 1.53.0 Jun 11, 2021
@Mark-Simulacrum Mark-Simulacrum removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 11, 2021
@Mark-Simulacrum
Copy link
Member

beta backported in #86203

@rylev
Copy link
Member

rylev commented Jun 16, 2021

@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?

@Mark-Simulacrum
Copy link
Member

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.

flip1995 pushed a commit to flip1995/rust that referenced this pull request Jun 17, 2021
…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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 27, 2021
…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
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 8, 2021
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants