Stop generating code in mem::forget #79989

scottmcm · 2020-12-12T23:52:30Z

Yes, the optimizer can remove it, but there's really no reason to bother emitting it in the first place. Not to mention that it all gets run in debug mode...

Preemptively cc @RalfJung, who probably wouldn't be happy about me doing this.

Tangentially related to #79914, though that's more about used ones where optimizing it away is harder than it is here.

rust-highfive · 2020-12-12T23:52:34Z

r? @cramertj

(rust-highfive has picked a reviewer for you, use r? to override)

Mark-Simulacrum · 2020-12-12T23:54:50Z

@bors try @rust-timer queue - though I doubt this will have any significant effect, seems good to know

rust-timer · 2020-12-12T23:54:52Z

Awaiting bors try build completion

bors · 2020-12-12T23:55:01Z

⌛ Trying commit bb78e10a9662fcaefffdd8b40ebcea4977232c39 with merge 196ca161be8363503d2d2445bab9840f0ce2bba4...

Yes, the optimizer can remove it, but there's really no reason to bother emitting it in the first place. Not to mention that it all gets run in debug mode...

scottmcm · 2020-12-12T23:58:21Z

Thanks @Mark-Simulacrum -- you beat me to it 😆

(Edit: I was going to wait for CI because I'm way too good at forgetting something.)

Mark-Simulacrum · 2020-12-13T00:02:38Z

(I think the post-try push may prevent the bors notification for try completion, so a manual rust-timer build invocation may become necessary).

scottmcm · 2020-12-13T02:30:25Z

@rust-timer build 196ca16

rust-timer · 2020-12-13T02:30:26Z

Queued 196ca161be8363503d2d2445bab9840f0ce2bba4 with parent 7efc097, future comparison URL.

rust-timer · 2020-12-13T04:45:33Z

Finished benchmarking try commit (196ca161be8363503d2d2445bab9840f0ce2bba4): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

RalfJung · 2020-12-13T17:56:58Z

library/core/src/intrinsics.rs

+    /// compiles fine just using `ManuallyDrop` instead.
+    ///
+    /// As this does literally nothing, it's trivially const-safe.
+    #[rustc_const_stable(feature = "const_forget_intrinsic", since = "1.50")]


Cc @rust-lang/wg-const-eval for the const stabilization here

@scottmcm could you add a test making sure that this actually works at const-time? From what I can see it actually should not work since this intrinsic is not implemented... you'll probably have to copy the Miri implementation.

I added a test, but it seemed to run fine? Please let me know if that's not the right way to test such a thing.

@oli-obk any idea how this test can possibly pass without forget being implemented in this file? I am very confused right now.

The intrinsic does not even seem to be called:

├─7ms INFO rustc_mir::interpret::step // executing bb1 ├─7ms INFO rustc_mir::interpret::step _0 = forget_arg_and_return_4(move _1) -> bb2 ├┐rustc_mir::interpret::eval_context::frame FOUR_THE_HARD_WAY │└┐rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 │┌┘rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 ├┘rustc_mir::interpret::eval_context::frame FOUR_THE_HARD_WAY ├┐rustc_mir::interpret::eval_context::frame FOUR_THE_HARD_WAY │└┐rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 │ ├─0ms INFO rustc_mir::interpret::step // executing bb0 │ ├─0ms INFO rustc_mir::interpret::step StorageLive(_2) │ ├─0ms INFO rustc_mir::interpret::step StorageLive(_3) │ ├─0ms INFO rustc_mir::interpret::step _3 = move _1 │ ├─0ms INFO rustc_mir::interpret::step _2 = std::mem::forget::<Vec<i32>>(move _3) -> [return: bb1, unwind: bb2] │ ├┐rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 │ │└┐rustc_mir::interpret::eval_context::frame std::mem::forget::<Vec<i32>> │ │ ├─0ms INFO rustc_mir::interpret::step // executing bb0 │ │ ├─0ms INFO rustc_mir::interpret::step _0 = const () │ │ ├─0ms INFO rustc_mir::interpret::step return │ │ ├─0ms INFO rustc_mir::interpret::eval_context popping stack frame (returning from function) │ │┌┘rustc_mir::interpret::eval_context::frame std::mem::forget::<Vec<i32>> │ ├┘rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 │ ├─0ms INFO rustc_mir::interpret::step // executing bb1 │ ├─0ms INFO rustc_mir::interpret::step StorageDead(_3) │ ├─0ms INFO rustc_mir::interpret::step StorageDead(_2) │ ├─0ms INFO rustc_mir::interpret::step _0 = const 4_i32 │ ├─0ms INFO rustc_mir::interpret::step return │ ├─0ms INFO rustc_mir::interpret::eval_context popping stack frame (returning from function) │┌┘rustc_mir::interpret::eval_context::frame forget_arg_and_return_4 ├┘rustc_mir::interpret::eval_context::frame FOUR_THE_HARD_WAY ├─7ms INFO rustc_mir::interpret::step // executing bb2 ├─7ms INFO rustc_mir::interpret::step StorageDead(_1) ├─7ms INFO rustc_mir::interpret::step return ├─7ms INFO rustc_mir::interpret::eval_context popping stack frame (returning from function) rustc_mir::interpret::eval_context::frame FOUR_THE_HARD_WAY

The body of std::mem::forget seems to consist only of _0 = const ().

Also very strange: the trace indicates two calls to forget_arg_and_return_4?

Hm, the intrinsic didn't have a body before either, so I guess there is no UB change. The intrinsic simply does not need an implementation any more.

@scottmcm you are good then, aside from the cfg(bootstrap) at the test which I think is unnecessary. :)

Turns out you actually do need to add the intrinsic implementation, as @tmiasko pointed out. It is still needed when libstd is built with -Zmir-opt-level=0.

It should be possible to test this by calling core::intrinsics::forget directly, instead of core::mem::forget wrapper, with -Zmir-opt-level=0,

I'll add the test. Looks like with #80040 it won't need the intrinsic implementation any more, though, right?

Looks like with #80040 it won't need the intrinsic implementation any more, though, right?

Yes.

RalfJung · 2020-12-13T17:57:47Z

library/core/src/mem/mod.rs

+    // which there's no reason to do.  Please switch it back if things have changed and
+    // the forget-is-nop codegen test confirms the intrinsic is no longer needed here.
+
+    // SAFETY: Forgetting is safe; it's just the intrinsic that isn't.


FWIW, intrinsics can also be marked as safe. (But no need to do that in this PR.)

RalfJung · 2020-12-13T18:00:47Z

Preemptively cc @RalfJung, who probably wouldn't be happy about me doing this.

Thanks for the heads-up! This is certainly not the nicest reason I have seen to introduce/expand an intrinsic, but I've also seen worse, so I am rather neutral regarding this change. ;)

RalfJung · 2020-12-14T09:17:43Z

library/core/tests/mem.rs

@@ -137,3 +137,16 @@ fn assume_init_good() {

    assert!(TRUE);
 }
+
+#[test]
+#[cfg(not(bootstrap))]


The test should pass with and without bootstrap, shouldn't it? mem::forget is const-stable already...

I would have thought so too, but https://github.com/rust-lang/rust/runs/1548179565

Ah, that's probably because of the missing intrinsic implementation in the bootstrap compiler. Good point.

the8472 · 2020-12-14T16:18:15Z

library/core/src/mem/mod.rs

+    // ```
+    // but as of 2020-12-12 that actually codegens the construction of the type,
+    // which there's no reason to do.  Please switch it back if things have changed and
+    // the forget-is-nop codegen test confirms the intrinsic is no longer needed here.


Should this be a FIXME pointing to the ManuallyDrop issue?

bors · 2020-12-16T09:44:46Z

☔ The latest upstream changes (presumably #79607) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

Dylan-DPC-zz · 2021-02-19T14:45:58Z

@scottmcm any update?

Make ptr::write const ~~The code in this PR as of right now is not much more than an experiment.~~ ~~This should, if I am not mistaken, in theory compile and pass the tests once the bootstraping compiler is updated. Thus the PR is blocked on that which should happen some time after the February the 9th. Also we might want to wait for rust-lang#79989 to avoid regressing performance due to using `mem::forget` over `intrinsics::forget`~~.

rust-highfive assigned cramertj Dec 12, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2020

Stop generating code in mem::forget

d23d87c

Yes, the optimizer can remove it, but there's really no reason to bother emitting it in the first place. Not to mention that it all gets run in debug mode...

scottmcm force-pushed the forgetful branch from bb78e10 to d23d87c Compare December 12, 2020 23:57

jyn514 added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Dec 13, 2020

rust-lang deleted a comment from rust-timer Dec 13, 2020

RalfJung reviewed Dec 13, 2020

View reviewed changes

Add a test using mem::forget in a const fn

edb4542

scottmcm force-pushed the forgetful branch from 479dd4c to edb4542 Compare December 14, 2020 05:54

RalfJung reviewed Dec 14, 2020

View reviewed changes

RalfJung mentioned this pull request Dec 14, 2020

Lower intrinsics calls: forget, size_of, unreachable, wrapping_* #79049

Merged

the8472 reviewed Dec 14, 2020

View reviewed changes

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2021

RalfJung mentioned this pull request Jan 18, 2021

Tracking Issue for const_ptr_read #80377

Closed

3 tasks

usbalbin mentioned this pull request Jan 18, 2021

Make ptr::write const #81167

Merged

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 29, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 14, 2021

JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 7, 2021

scottmcm closed this Mar 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop generating code in mem::forget #79989

Stop generating code in mem::forget #79989

scottmcm commented Dec 12, 2020

rust-highfive commented Dec 12, 2020

Mark-Simulacrum commented Dec 12, 2020

rust-timer commented Dec 12, 2020

bors commented Dec 12, 2020

scottmcm commented Dec 12, 2020 •

edited

Loading

Mark-Simulacrum commented Dec 13, 2020

scottmcm commented Dec 13, 2020

rust-timer commented Dec 13, 2020

rust-timer commented Dec 13, 2020

RalfJung Dec 13, 2020

RalfJung Dec 13, 2020

scottmcm Dec 14, 2020 •

edited

Loading

RalfJung Dec 14, 2020

RalfJung Dec 14, 2020 •

edited

Loading

RalfJung Dec 14, 2020

RalfJung Dec 14, 2020

tmiasko Dec 14, 2020

scottmcm Dec 15, 2020

tmiasko Dec 15, 2020

RalfJung Dec 13, 2020

RalfJung commented Dec 13, 2020 •

edited

Loading

RalfJung Dec 14, 2020

scottmcm Dec 14, 2020

RalfJung Dec 14, 2020

the8472 Dec 14, 2020

bors commented Dec 16, 2020

Dylan-DPC-zz commented Feb 19, 2021

Stop generating code in mem::forget #79989

Stop generating code in mem::forget #79989

Conversation

scottmcm commented Dec 12, 2020

rust-highfive commented Dec 12, 2020

Mark-Simulacrum commented Dec 12, 2020

rust-timer commented Dec 12, 2020

bors commented Dec 12, 2020

scottmcm commented Dec 12, 2020 • edited Loading

Mark-Simulacrum commented Dec 13, 2020

scottmcm commented Dec 13, 2020

rust-timer commented Dec 13, 2020

rust-timer commented Dec 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottmcm Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Dec 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Dec 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Dec 16, 2020

Dylan-DPC-zz commented Feb 19, 2021

scottmcm commented Dec 12, 2020 •

edited

Loading

scottmcm Dec 14, 2020 •

edited

Loading

RalfJung Dec 14, 2020 •

edited

Loading

RalfJung commented Dec 13, 2020 •

edited

Loading