Always evaluate constant operands in const-prop by b-naber · Pull Request #98426 · rust-lang/rust

b-naber · 2022-06-23T15:02:34Z

r? @lcnr

lcnr

one nit, then r=me

lcnr · 2022-06-24T11:28:12Z

@bors try @rust-timer queue

this might actually impact perf, if unlikely

rust-timer · 2022-06-24T11:28:13Z

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

bors · 2022-06-24T11:28:22Z

⌛ Trying commit 18e0d916581dc54ddd13e1643d25c81408ad1987 with merge 67ad21cde5ced3782151744e37f78e23af0e8b24...

bors · 2022-06-24T13:06:12Z

☀️ Try build successful - checks-actions
Build commit: 67ad21cde5ced3782151744e37f78e23af0e8b24 (67ad21cde5ced3782151744e37f78e23af0e8b24)

rust-timer · 2022-06-24T13:06:14Z

Queued 67ad21cde5ced3782151744e37f78e23af0e8b24 with parent d017d59, future comparison URL.

rust-timer · 2022-06-24T14:36:33Z

Finished benchmarking commit (67ad21cde5ced3782151744e37f78e23af0e8b24): comparison url.

Instruction count

Primary benchmarks: 😿 relevant regressions found
Secondary benchmarks: 🎉 relevant improvements found

	mean¹	max	count²
Regressions 😿 (primary)	0.3%	0.4%	6
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-0.4%	-0.4%	2
All 😿🎉 (primary)	0.3%	0.4%	6

Max RSS (memory usage)

Results

Primary benchmarks: 😿 relevant regression found
Secondary benchmarks: 😿 relevant regression found

	mean¹	max	count²
Regressions 😿 (primary)	1.3%	1.3%	1
Regressions 😿 (secondary)	9.6%	9.6%	1
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	N/A	N/A	0
All 😿🎉 (primary)	1.3%	1.3%	1

Cycles

Results

Primary benchmarks: no relevant changes found
Secondary benchmarks: 🎉 relevant improvement found

	mean¹	max	count²
Regressions 😿 (primary)	N/A	N/A	0
Regressions 😿 (secondary)	N/A	N/A	0
Improvements 🎉 (primary)	N/A	N/A	0
Improvements 🎉 (secondary)	-3.3%	-3.3%	1
All 😿🎉 (primary)	N/A	N/A	0

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

the arithmetic mean of the percent change ↩ ↩² ↩³
number of relevant changes ↩ ↩² ↩³

lcnr · 2022-06-24T15:01:16Z

the way that mostly incr-unchanged is impacted and all perf impacts are really small I think this isn't too relevant.

I consider this a bug fix.

@bors r+

bors · 2022-06-24T15:01:18Z

📌 Commit 18e0d916581dc54ddd13e1643d25c81408ad1987 has been approved by lcnr

RalfJung · 2022-06-28T13:01:19Z

Const-prop does not run in the body of a const/static, does it?
Those items contain promoteds that might fail to evaluate, so it is crucial that we only evaluate those nested constants that are actually needed to compute the const/static initializer. I tried to fix this but too much code out there has such potentially failing promoteds. :( See #80619.

I am fairly sure I added a test for this so it's probably fine, just double-checking. :)

bors · 2022-06-28T13:27:26Z

⌛ Testing commit 18e0d916581dc54ddd13e1643d25c81408ad1987 with merge efa8ecf1ad8601a2b9aa8549d40052761450cc9d...

bors · 2022-06-28T13:46:38Z

💔 Test failed - checks-actions

Dylan-DPC · 2022-06-28T14:30:16Z

@bors retry

b-naber · 2022-06-28T15:36:10Z

@Dylan-DPC this doesn't look spurious. Can you r- this, please?

RalfJung · 2022-06-28T16:11:49Z

@bors r-

JohnCSimon · 2022-07-24T00:17:06Z

Triage:
@b-naber can you post the status of this PR?

b-naber · 2022-07-28T09:33:34Z

I don't know why these lints are output on that machine (which seems to be a 64-bit machine):

Future incompatibility report: Future breakage diagnostic:
error: any use of this value will cause an error
  --> /checkout/src/test/ui/consts/const-eval/issue-50814.rs:15:21
   |
LL |     const MAX: u8 = A::MAX + B::MAX;
   |     ----------------^^^^^^^^^^^^^^^-
   |                     |
   |                     attempt to compute `u8::MAX + u8::MAX`, which would overflow
   |
   = note: `#[deny(const_err)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

Future breakage diagnostic:
error: any use of this value will cause an error
  --> /checkout/src/test/ui/consts/const-eval/issue-50814.rs:15:21
   |
LL |     const MAX: u8 = A::MAX + B::MAX;
   |     ----------------^^^^^^^^^^^^^^^-
   |                     |
   |                     attempt to compute `u8::MAX + u8::MAX`, which would overflow
   |
   = note: `#[deny(const_err)]` on by default
   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
   = note: for more information, see issue #71800 <https://github.com/rust-lang/rust/issues/71800>

This looks like a miri-related lint, @RalfJung do you have any idea why we get those lints on that machine, but not on the ones in the PR CI?

RalfJung · 2022-07-28T22:07:29Z

Maybe different flags? Debug vs release mode can make a difference.
Also the lints are emitted by const-prop-lint, which uses Miri but then also does a lot of other stuff, and I'm not much involved with that. Maybe @oli-obk knows more.

RalfJung · 2022-07-28T22:08:42Z

Also make sure you rebase, there have been some changes wrt binop overflow checking a few weeks ago.

rustbot · 2022-08-04T09:14:54Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

rust-log-analyzer · 2022-08-04T10:09:20Z

The job x86_64-gnu-llvm-12 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

---- [mir-opt] src/test/mir-opt/remove-never-const.rs stdout ----

error: compilation failed!
status: exit status: 1
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/mir-opt/remove-never-const.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "-O" "-Copt-level=1" "-Zdump-mir=all" "-Zvalidate-mir" "-Zdump-mir-exclude-pass-number" "-Zmir-pretty-relative-line-numbers=yes" "-Zmir-opt-level=4" "-Zdump-mir-dir=/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/remove-never-const" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/remove-never-const" "-Crpath" "-Cdebuginfo=0" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--emit" "mir,link" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/mir-opt/remove-never-const/auxiliary"
stdout: none
--- stderr -------------------------------
  --> /checkout/src/test/mir-opt/remove-never-const.rs:11:8
   |
   |
11 | struct PrintName<T>(T);
   |
   = note: `#[warn(dead_code)]` on by default

warning: function `no_codegen` is never used
warning: function `no_codegen` is never used
  --> /checkout/src/test/mir-opt/remove-never-const.rs:18:4
   |
18 | fn no_codegen<T>() {

warning: associated constant `VOID` is never used
  --> /checkout/src/test/mir-opt/remove-never-const.rs:14:11
   |

b-naber · 2022-08-04T10:14:28Z

The error we get in the failing mir-opt test looks wrong.

oli-obk

I guess the failing mir opt test shows that we fail to add some constants to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Body.html#structfield.required_consts though I don't know why, how or where.

oli-obk · 2022-08-25T10:16:29Z

+          - name: dist-i586-gnu-i586-i686-musl
+            os: ubuntu-20.04-xl
+            env: {}


did this accidentally get added during a rebase?

oli-obk · 2022-08-25T10:16:44Z

-
+
+          - name: dist-i586-gnu-i586-i686-musl
+            <<: *job-linux-xl
+


RalfJung · 2022-08-25T12:22:40Z

I guess the failing mir opt test shows that we fail to add some constants to https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.Body.html#structfield.required_consts though I don't know why, how or where.

Promoteds don't get added there. That is by design since their evaluation cannot fail (in fn), or must only be attempted if they are in live code (in const/static).

Not sure if that's the issue here, though.

bors · 2022-08-29T23:12:44Z

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

b-naber · 2022-08-30T18:49:48Z

I'm going to close this, not familiar enough with const prop to tell what's really going on here.

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 23, 2022

rust-highfive assigned lcnr Jun 23, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2022

lcnr reviewed Jun 24, 2022

View reviewed changes

Comment thread src/test/ui/consts/const-eval/issue-50814.rs Outdated

lcnr added the A-const-generics Area: const generics (parameters and arguments) label Jun 24, 2022

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Jun 24, 2022

bors mentioned this pull request Jun 24, 2022

update Miri #98397

Merged

rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Jun 24, 2022

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 24, 2022

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 Jun 28, 2022

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 28, 2022

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 28, 2022

This comment has been minimized.

Sign in to view

oli-obk reviewed Jul 29, 2022

View reviewed changes

Comment thread compiler/rustc_mir_transform/src/const_prop.rs Outdated

b-naber added 4 commits August 4, 2022 10:10

always evaluate constant operands in const-prop

ddc74d3

fix nit

9704893

ci

d10dccf

same thing in const-prop-lint

5726ed7

b-naber force-pushed the evaluate-const-operand-const-prop branch from 18e0d91 to 5726ed7 Compare August 4, 2022 09:14

rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Aug 4, 2022

This comment has been minimized.

Sign in to view

bless tests

4e43e3a

oli-obk requested changes Aug 25, 2022

View reviewed changes

b-naber closed this Aug 30, 2022

Uh oh!

Conversation

b-naber commented Jun 23, 2022

Uh oh!

lcnr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lcnr commented Jun 24, 2022

Uh oh!

rust-timer commented Jun 24, 2022

Uh oh!

bors commented Jun 24, 2022

Uh oh!

bors commented Jun 24, 2022

Uh oh!

rust-timer commented Jun 24, 2022

Uh oh!

rust-timer commented Jun 24, 2022

Instruction count

Max RSS (memory usage)

Cycles

Footnotes

Uh oh!

lcnr commented Jun 24, 2022

Uh oh!

bors commented Jun 24, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

bors commented Jun 28, 2022

Uh oh!

bors commented Jun 28, 2022

Uh oh!

Dylan-DPC commented Jun 28, 2022

Uh oh!

b-naber commented Jun 28, 2022

Uh oh!

RalfJung commented Jun 28, 2022

Uh oh!

This comment has been minimized.

JohnCSimon commented Jul 24, 2022

Uh oh!

b-naber commented Jul 28, 2022

Uh oh!

RalfJung commented Jul 28, 2022

Uh oh!

RalfJung commented Jul 28, 2022

Uh oh!

Uh oh!

rustbot commented Aug 4, 2022

Uh oh!

This comment has been minimized.

rust-log-analyzer commented Aug 4, 2022

Uh oh!

b-naber commented Aug 4, 2022

Uh oh!

oli-obk left a comment

Choose a reason for hiding this comment

Uh oh!

oli-obk Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

oli-obk Aug 25, 2022

Choose a reason for hiding this comment

Uh oh!

RalfJung commented Aug 25, 2022

Uh oh!

bors commented Aug 29, 2022

Uh oh!

b-naber commented Aug 30, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants