Some promotion cleanup #76411

RalfJung · 2020-09-06T14:35:52Z

Based on top of both #75502 and #75585, this does some cleanup of the promotion code. The last 2 commits are new.

Remove the remaining cases where const fn is treated different from fn. This means no longer promoting ptr-to-int casts, raw ptr operations, and union field accesses in const fn -- or anywhere, for that matter. These are all unstable in const-context so this should not break any stable code. Fixes Treat const fn like fn for promotion. #75586.
~~Promote references to statics even outside statics (i.e., in functions) for consistency.~~
Promote &mut [] everywhere, not just in non-const functions, for consistency.
Explain why we do not promote deref's of statics outside statics. ~~(This is the only remaining direct user of const_kind.)~~

This can only land once the other two PRs land; I am mostly putting this up already because I couldn't wait ;) and to get some feedback from @rust-lang/wg-const-eval .

rust-highfive · 2020-09-06T14:35:56Z

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

RalfJung · 2020-09-06T16:04:36Z

compiler/rustc_mir/src/transform/promote_consts.rs

-                        // We can only promote interior borrows of promotable temps (non-temps
-                        // don't get promoted anyway).
+                        // We only actually promote the projection base, so only that is further validated.
+                        // Make sure it does not need dropping!


I think this is correct, based on the fact that we can promote this:

const ALL_THE_NUMS: [u32; 1] = [ 1 ]; #[inline(never)] fn array(i: usize) -> &'static u32 { return &ALL_THE_NUMS[i]; }

But maybe we are even more clever, and promote those projections that we can, stopping at Index?

compiler/rustc_mir/src/transform/promote_consts.rs

ecstatic-morse

I don't think we should be doing non-trivial refactors of this code without crater runs. I think this PR should just be the two crater-tested changes coupled with the removal of the const_kind check for things that are forbidden in a const-context anyways and promotion of &mut []. We just don't have good enough test coverage here, although I appreciate that you're trying to rectify that.

ecstatic-morse · 2020-09-07T16:13:27Z

compiler/rustc_mir/src/transform/promote_consts.rs

-                    // Only allow statics (not consts) to refer to other statics.
-                    // FIXME(eddyb) does this matter at all for promotion?
-                    let is_static = matches!(self.const_kind, Some(hir::ConstContext::Static(_)));
-                    if !is_static {
-                        return Err(Unpromotable);
-                    }
-


Why do we want to promote &STATIC? The resulting reference is already 'static, no? Is this doing something else that I'm not seeing? I would have expected us to go the other way and make statics ineligible for promotion in all contexts for now.

This enables let _val: &'static [&'static u32] = &[&FOO2]; in a fn.

I would have expected us to go the other way and make statics ineligible for promotion in all contexts for now.

I'm all in for restricting promotion but I doubt this one can be taken back -- and then it seems strange to not promote this in fn.

I think this should remain the same for now. Promoting &STATIC will cause us to generate more useless promoted MIR bodies for little benefit.

I doubt this one can be taken back.

Perhaps, but I would like some time to consider before committing to this in all contexts. &STATIC already works everywhere. static OTHER: &[&u32] = &[&STATIC]; should work without promotion due to the "top-level scope" rule, so promotion is only helpful in very narrow circumstances.

Promoting &STATIC will cause us to generate more useless promoted MIR bodies for little benefit.

That's an argument against most promotion though...
My point is, there is absolutely no reason to do this promotion only in statics. That just makes no sense. And also I was hoping to make this a PR that doesn't require its own crater run with associated 2-3 weeks of waiting.

If there is a performance problem here due to too many MIR bodies, shouldn't we also fix it inside static then?

static OTHER: &[&u32] = &[&STATIC]; should work without promotion due to the "top-level scope" rule, so promotion is only helpful in very narrow circumstances.

But TupleStruct(&[&STATIC]) won't work without this. That might be narrow but not more narrow than many other cases here.

But, I guess we can do baby steps for now. I'm just not very patient now that I finally see an endgame for this long-standing problem.^^

I'm happy to discuss this change as part of its own PR. If I were consistent I suppose I would have demanded that the &mut [] change be split out as well, but I'm less concerned about that one since promotion actually does lifetime extension and it fits better into my (pipe) dream of requiring that all promotable expressions be valid patterns.

&mut [] is also widely used, there is no taking it back, so it seems just odd to reject it in const fn/const/static.

ecstatic-morse · 2020-09-07T16:14:46Z

compiler/rustc_mir/src/transform/promote_consts.rs

    fn validate_rvalue(&self, rvalue: &Rvalue<'tcx>) -> Result<(), Unpromotable> {
        match *rvalue {
-            Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if self.const_kind.is_none() => {
+            Rvalue::Cast(CastKind::Misc, ref operand, cast_ty) if self.maybe_runtime() => {


Pointer to int casts are not allowed in any const context. We should never promote these.

I can remove it, but in the review comment you said you preferred not changing behavior without a crater run so I am not sure now what you'd prefer me to do.

To me, this change is trivial, while the validate_ref refactor is not.

ecstatic-morse · 2020-09-07T16:20:08Z

compiler/rustc_mir/src/transform/promote_consts.rs

                        return Err(Unpromotable);
                    }
                    _ => {}
                }
            }

-            Rvalue::BinaryOp(op, ref lhs, _) if self.const_kind.is_none() => {
+            Rvalue::BinaryOp(op, ref lhs, _) if self.maybe_runtime() => {


Pointer comparison/offset is not stable in any const context. We should never promote these.

I was also surprised to see no mention of floating point arithmetic, so we currently promote e.g. &(42.0 * 55.1). This is actually okay, because the IEEE spec mandates the precision for simple arithmetic operations, so there shouldn't be a difference between the run-time and compile-time result. I wonder if this was a conscious choice way way back, or just a happy accident.

It probably was an accident.^^

ecstatic-morse · 2020-09-07T16:32:37Z

compiler/rustc_mir/src/transform/promote_consts.rs

@@ -701,32 +674,7 @@ impl<'tcx> Validator<'_, 'tcx> {
                }


place gets mutated here, so your refactor has changed the logic in ways that's difficult to predict.

Place gets rebound here, yeah... I thought that was fine but on the second time reading this I agree I don't understand this code well enough. It removes a top-level * but only if the thing right below it is a reference... but that changes its type?!?

I'll move the validate_ref change to a separate PR once #75585 lands.

bors · 2020-09-08T07:38:23Z

☔ The latest upstream changes (presumably #75585) 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

oli-obk · 2020-09-09T08:54:48Z

r? @ecstatic-morse if you have the time, if not reassign to me

src/test/ui/consts/promotion.rs

src/test/ui/statics/static-promotion.rs

ecstatic-morse · 2020-09-12T20:02:35Z

compiler/rustc_mir/src/transform/promote_consts.rs

-                        if self.const_kind.is_none() {
+                        if self.maybe_runtime() {


I didn't realize this in my first round of comments, but this means that we currently promote union field accesses in constants and statics but not fn, which is somewhat weird. Union field accesses are currently unstable in const fn, so this doesn't change anything for stable users, but if the goal is to normalize promotion across all const contexts, we need to figure out what to do here eventually.

Yes, I mentioned this in rust-lang/const-eval#53.

We cannot accept them in fn as union field access are a fallible operation -- they can cause UB. So either we keep them const/static-only, or we never promote them. In my follow-up PR that needs crater anyway, I intend to reject promoting them to see what the fallout of that would be.

ecstatic-morse · 2020-09-12T20:19:13Z

r=me

I think FCPs are 7 days long, so the one on #75502 has concluded even though the bot didn't leave a comment. Is this correct? This is a better abstraction than the one in #75502, so there's no need to wait for that one to filter through the queue unless we're worried about bisection. @RalfJung, do you wanna wait or squash this and add the title of #75502 to the PR summary?

RalfJung · 2020-09-13T11:36:25Z

I think FCPs are 7 days long

FCPs are 10 days long, so it should finish today or tomorrow. It seems confusing to not let the FCP's PR land (confusing to someone browsing through the history in the future to see what was done why). So I'd just let #75502 land.

bors · 2020-09-19T13:51:17Z

☔ The latest upstream changes (presumably #76912) 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

in particular allow a few more promotions for consistency when they were already allowed in other contexts

RalfJung · 2020-09-19T20:17:05Z

@bors r=ecstatic-morse

bors · 2020-09-19T20:17:07Z

📌 Commit 9216eb8 has been approved by ecstatic-morse

…tatic-morse Some promotion cleanup Based on top of both rust-lang#75502 and rust-lang#75585, this does some cleanup of the promotion code. The last 2 commits are new. * Remove the remaining cases where `const fn` is treated different from `fn`. This means no longer promoting ptr-to-int casts, raw ptr operations, and union field accesses in `const fn` -- or anywhere, for that matter. These are all unstable in const-context so this should not break any stable code. Fixes rust-lang#75586. * ~~Promote references to statics even outside statics (i.e., in functions) for consistency.~~ * Promote `&mut []` everywhere, not just in non-`const` functions, for consistency. * Explain why we do not promote deref's of statics outside statics. ~~(This is the only remaining direct user of `const_kind`.)~~ This can only land once the other two PRs land; I am mostly putting this up already because I couldn't wait ;) and to get some feedback from @rust-lang/wg-const-eval .

bors · 2020-09-20T06:20:21Z

⌛ Testing commit 9216eb8 with merge 10b3595...

bors · 2020-09-20T08:22:44Z

☀️ Test successful - checks-actions, checks-azure
Approved by: ecstatic-morse
Pushing 10b3595 to master...

rust-highfive assigned nikomatsakis Sep 6, 2020

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

RalfJung added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 6, 2020

jyn514 added A-const-eval Area: Constant evaluation (MIR interpretation) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Sep 6, 2020

RalfJung force-pushed the promote-in-const-fn branch from 80d0e22 to 3ef54f1 Compare September 6, 2020 16:03

RalfJung commented Sep 6, 2020

View reviewed changes

RalfJung force-pushed the promote-in-const-fn branch from 3ef54f1 to 2eacc12 Compare September 6, 2020 16:58

oli-obk reviewed Sep 7, 2020

View reviewed changes

compiler/rustc_mir/src/transform/promote_consts.rs Outdated Show resolved Hide resolved

compiler/rustc_mir/src/transform/promote_consts.rs Show resolved Hide resolved

ecstatic-morse reviewed Sep 7, 2020

View reviewed changes

RalfJung force-pushed the promote-in-const-fn branch from eed2710 to d604246 Compare September 9, 2020 08:36

rust-highfive assigned ecstatic-morse and unassigned nikomatsakis Sep 9, 2020

RalfJung force-pushed the promote-in-const-fn branch from d604246 to 782bcb6 Compare September 9, 2020 09:09

RalfJung commented Sep 9, 2020

View reviewed changes

src/test/ui/consts/promotion.rs Show resolved Hide resolved

RalfJung commented Sep 9, 2020

View reviewed changes

src/test/ui/statics/static-promotion.rs Show resolved Hide resolved

ecstatic-morse reviewed Sep 12, 2020

View reviewed changes

RalfJung force-pushed the promote-in-const-fn branch from 140f586 to bc52c8c Compare September 17, 2020 07:05

RalfJung added 4 commits September 19, 2020 22:14

cleanup promotion const_kind checks

4d1ef03

in particular allow a few more promotions for consistency when they were already allowed in other contexts

fix doc comment

7febd5a

never promote non-const operations; revert STATIC promotion change

7b99c8e

fix some comments

9216eb8

RalfJung force-pushed the promote-in-const-fn branch from bc52c8c to 9216eb8 Compare September 19, 2020 20:14

RalfJung removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Sep 19, 2020

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 Sep 19, 2020

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 17 pull requests #76944

Closed

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 18 pull requests #76945

Closed

Dylan-DPC-zz mentioned this pull request Sep 19, 2020

Rollup of 18 pull requests #76946

Closed

Dylan-DPC-zz mentioned this pull request Sep 20, 2020

Rollup of 12 pull requests #76951

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 20, 2020

bors merged commit 10b3595 into rust-lang:master Sep 20, 2020

rustbot added this to the 1.48.0 milestone Sep 20, 2020

RalfJung deleted the promote-in-const-fn branch September 20, 2020 08:33

bors mentioned this pull request Sep 20, 2020

use if let instead of single match arm expressions #76888

Merged

josephlr mentioned this pull request Sep 24, 2020

Regression for #![feature(const_in_array_repeat_expressions)] #77138

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some promotion cleanup #76411

Some promotion cleanup #76411

RalfJung commented Sep 6, 2020 •

edited

Loading

rust-highfive commented Sep 6, 2020

RalfJung Sep 6, 2020

ecstatic-morse left a comment •

edited

Loading

ecstatic-morse Sep 7, 2020

RalfJung Sep 7, 2020

ecstatic-morse Sep 7, 2020

RalfJung Sep 9, 2020 •

edited

Loading

RalfJung Sep 9, 2020

ecstatic-morse Sep 12, 2020

RalfJung Sep 13, 2020

ecstatic-morse Sep 7, 2020

RalfJung Sep 7, 2020

ecstatic-morse Sep 7, 2020

ecstatic-morse Sep 7, 2020

ecstatic-morse Sep 7, 2020 •

edited

Loading

RalfJung Sep 7, 2020 •

edited

Loading

ecstatic-morse Sep 7, 2020

RalfJung Sep 7, 2020 •

edited

Loading

bors commented Sep 8, 2020

oli-obk commented Sep 9, 2020

ecstatic-morse Sep 12, 2020

RalfJung Sep 13, 2020

ecstatic-morse commented Sep 12, 2020 •

edited

Loading

RalfJung commented Sep 13, 2020 •

edited

Loading

bors commented Sep 19, 2020

RalfJung commented Sep 19, 2020

bors commented Sep 19, 2020

bors commented Sep 20, 2020

bors commented Sep 20, 2020

Some promotion cleanup #76411

Some promotion cleanup #76411

Conversation

RalfJung commented Sep 6, 2020 • edited Loading

rust-highfive commented Sep 6, 2020

Choose a reason for hiding this comment

ecstatic-morse left a comment • 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

RalfJung Sep 9, 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

Choose a reason for hiding this comment

ecstatic-morse Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

RalfJung Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

bors commented Sep 8, 2020

oli-obk commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse commented Sep 12, 2020 • edited Loading

RalfJung commented Sep 13, 2020 • edited Loading

bors commented Sep 19, 2020

RalfJung commented Sep 19, 2020

bors commented Sep 19, 2020

bors commented Sep 20, 2020

bors commented Sep 20, 2020

RalfJung commented Sep 6, 2020 •

edited

Loading

ecstatic-morse left a comment •

edited

Loading

RalfJung Sep 9, 2020 •

edited

Loading

ecstatic-morse Sep 7, 2020 •

edited

Loading

RalfJung Sep 7, 2020 •

edited

Loading

RalfJung Sep 7, 2020 •

edited

Loading

ecstatic-morse commented Sep 12, 2020 •

edited

Loading

RalfJung commented Sep 13, 2020 •

edited

Loading