Run RemoveZsts pass at mir-opt-level=1 #83417

erikdesjardins · 2021-03-23T18:20:47Z

This pass removes assignments to ZST places.

Perf (from #83177 (comment)): https://perf.rust-lang.org/compare.html?start=41b315a470d583f6446599984ff9ad3bd61012b2&end=bd5d1b96f0c64c9938feea831789e1b5bb2cd4a2

r? @oli-obk

camelid · 2021-03-23T19:05:59Z

compiler/rustc_mir/src/transform/remove_zsts.rs

@@ -8,9 +8,6 @@ pub struct RemoveZsts;

 impl<'tcx> MirPass<'tcx> for RemoveZsts {
    fn run_pass(&self, tcx: TyCtxt<'tcx>, body: &mut Body<'tcx>) {
-        if tcx.sess.mir_opt_level() < 3 {


Should this be kept and use < 1 instead? I noticed that some const errors have changed, so I'm wondering if it's being triggered on mir-opt-level=0.

It doesn't run at mir-opt-level=0.

See

rust/compiler/rustc_mir/src/transform/mod.rs

Line 498 in 79e5814

&remove_zsts::RemoveZsts,

and

rust/compiler/rustc_mir/src/transform/mod.rs

Line 558 in 79e5814

if mir_opt_level > 0 { optimizations } else { no_optimizations },

The error changes because it removes an assignment, so we hit a different error path due to a different use of the constant. The specific error messages changed in this PR are fine (see #83177 (comment)).

But this area seems undertested, I think I've found some cases where it's wrong. I'll open another PR to add tests, that we should merge before this one.

The error changes because it removes an assignment, so we hit a different error path due to a different use of the constant. The specific error messages changed in this PR are fine (see #83177 (comment)).

I see, does the mir-opt-level apply to const-eval as well?

~~I believe so, because I believe the same MIR is used for const eval and codegen.~~

But I don't think that's relevant here--the error that changes is in main, not in const eval'd code.

There is a separate const eval error due to the panic (reported as a future compat warning):

rust/src/test/ui/consts/const-eval/panic-never-type.rs

Lines 8 to 10 in 79e5814

const VOID: ! = panic!();

//~^ WARN any use of this value will cause an error

//~| WARN this was previously accepted by the compiler but is being phased out

Since #78407, CTFE uses unoptimized MIR to avoid situations where an undefined behaviour is hidden because of MIR optimizations.

But I don't think that's relevant here--the error that changes is in main, not in const eval'd code.

Ah, I didn't notice that. Thanks for clearing up my confusion :)

The reason for the change in diagnostics is that const propagation now doesn't see the constant anymore, but codegen will still evaluate it because it was used in the initial MIR body (see the https://doc.rust-lang.org/nightly/nightly-rustc/rustc_mir/transform/required_consts/index.html field)

Another way to resolve that these tests get changed would be to eagerly evaluate all required_consts in const propagation

To stop depending on optimizations reporting these entirely, we could also evaluate the constant in

rust/compiler/rustc_mir/src/transform/required_consts.rs

Line 18 in 372afcf

if let ConstKind::Unevaluated(_) = ct.val {

and only put it into required_consts if the evaluation did not succeed

erikdesjardins · 2021-03-23T21:37:26Z

Blocked on #83423

@rustbot labels: -S-waiting-on-review +S-blocked

Edit: ~~are you not allowed to have any other text when mentioning rustbot?~~ ah label not labels 😠

camelid · 2021-03-23T23:54:23Z

Edit: ~~are you not allowed to have any other text when mentioning rustbot?~~ ah label not labels 😠

Note that in the future you can just edit your message to replace labels with label and triagebot should pick it up.

oli-obk · 2021-03-30T12:39:18Z

Blocked on #83423

I don't understand the reason for this PR. If we are breaking those tests, they are already in our test suite, so what is that PR adding?

I think we should avoid changing ui tests in this PR by doing what I mentioned in #83417 (comment) (that you can do in a separate PR though if you want :) )

erikdesjardins · 2021-03-31T21:53:46Z

The existing tests aren't the same as #83423. For the existing tests, RemoveZsts just changes the error message. But for the tests in #83423, RemoveZsts removes the error entirely, allowing erroneous code to compile.

(I thought that was strange so I did some more testing...this seems to only happen with --emit=mir, I guess because it stops before codegen. Maybe that's fine? Is it okay if errors disappear when using --emit=mir?)

oli-obk · 2021-04-01T09:03:40Z

Right, these are only evaluated once we go into codegen, and for generic constants, this will always be true, we need to monomorphize in order to evaluate them. For the already monomorphic constants we can optimize a little by doing the second part in #83417 (comment) (meaning: only put constants into required_consts if their evaluation failed due to TooGeneric or Linted).

If we can keep errors in check builds instead of full builds, we should do so, even if it is imperfect.

bors · 2021-05-31T15:34:42Z

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

Effectively reverts commit 6960bc9.

erikdesjardins · 2021-08-07T16:51:10Z

Rebased.

I made RequiredConstsVisitor attempt to evaluate consts (not pushed to this branch), but it made some errors disappear. You can see those changes here: erikdesjardins@c10bacf

@rustbot label: -S-waiting-on-author +S-waiting-on-review

oli-obk · 2021-08-09T14:41:06Z

Let's merge as is then. Pointing to the use site of a broken constant isn't really necessary, and for associated constants of generics we still behave as previously, so that is good.

@bors r+

bors · 2021-08-09T14:41:08Z

📌 Commit 585e4ae has been approved by oli-obk

bors · 2021-08-09T16:46:09Z

⌛ Testing commit 585e4ae with merge 5b243cdd288c0c43b89060c71cfc549ee02c04fb...

bors · 2021-08-09T16:50:29Z

💔 Test failed - checks-actions

rust-log-analyzer · 2021-08-09T19:14:07Z

A job failed! Check out the build log: (web) (plain)

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

Prepare workflow directory
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v2'
##[warning]Failed to download action 'https://api.github.com/repos/actions/checkout/zipball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
##[warning]Back off 10.339 seconds before retry.
##[warning]Failed to download action 'https://api.github.com/repos/actions/checkout/zipball/5a4ac9002d0be2fb38bd78e4b4dbde5606d7042f'. Error: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.
##[warning]Back off 17.939 seconds before retry.
##[error]A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.

oli-obk · 2021-08-09T21:29:25Z

@bors retry

erikdesjardins · 2021-08-11T02:38:17Z

Maybe this should be rollup=never? (because perf implications)

bors · 2021-08-12T03:57:12Z

⌛ Testing commit 585e4ae with merge 92d26e1859495c185896762086e495f4061de4fd...

rust-log-analyzer · 2021-08-12T04:29:44Z

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

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

---- [ui] ui/where-clauses/where-clause-early-bound-lifetimes.rs stdout ----

error: test run failed!
status: exit status: 101
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "run" "0" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/where-clauses/where-clause-early-bound-lifetimes/a"
------------------------------------------
uploaded "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/where-clauses/where-clause-early-bound-lifetimes/a", waiting for result

------------------------------------------
------------------------------------------
stderr:
------------------------------------------
thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9

------------------------------------------


---

Some tests failed in compiletest suite=ui mode=ui host=x86_64-unknown-linux-gnu target=arm-unknown-linux-gnueabihf


command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/arm-unknown-linux-gnueabihf/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-arm-unknown-linux-gnueabihf" "--suite" "ui" "--mode" "ui" "--target" "arm-unknown-linux-gnueabihf" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/checkout/obj/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--linker" "arm-linux-gnueabihf-gcc" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0  -Lnative=/checkout/obj/build/arm-unknown-linux-gnueabihf/native/rust-test-helpers" "--docck-python" "/usr/bin/python3" "--lldb-python" "/usr/bin/python3" "--llvm-version" "12.0.1-rust-1.56.0-nightly" "--llvm-components" "aarch64 aarch64asmparser aarch64codegen aarch64desc aarch64disassembler aarch64info aarch64utils aggressiveinstcombine all all-targets analysis arm armasmparser armcodegen armdesc armdisassembler arminfo armutils asmparser asmprinter avr avrasmparser avrcodegen avrdesc avrdisassembler avrinfo binaryformat bitreader bitstreamreader bitwriter bpf bpfasmparser bpfcodegen bpfdesc bpfdisassembler bpfinfo cfguard codegen core coroutines coverage debuginfocodeview debuginfodwarf debuginfogsym debuginfomsf debuginfopdb demangle dlltooldriver dwarflinker engine executionengine extensions filecheck frontendopenacc frontendopenmp fuzzmutate globalisel hellonew hexagon hexagonasmparser hexagoncodegen hexagondesc hexagondisassembler hexagoninfo instcombine instrumentation interfacestub interpreter ipo irreader jitlink libdriver lineeditor linker lto mc mca mcdisassembler mcjit mcparser mips mipsasmparser mipscodegen mipsdesc mipsdisassembler mipsinfo mirparser msp430 msp430asmparser msp430codegen msp430desc msp430disassembler msp430info native nativecodegen nvptx nvptxcodegen nvptxdesc nvptxinfo objcarcopts object objectyaml option orcjit orcshared orctargetprocess passes powerpc powerpcasmparser powerpccodegen powerpcdesc powerpcdisassembler powerpcinfo profiledata remarks riscv riscvasmparser riscvcodegen riscvdesc riscvdisassembler riscvinfo runtimedyld scalaropts selectiondag sparc sparcasmparser sparccodegen sparcdesc sparcdisassembler sparcinfo support symbolize systemz systemzasmparser systemzcodegen systemzdesc systemzdisassembler systemzinfo tablegen target textapi transformutils vectorize webassembly webassemblyasmparser webassemblycodegen webassemblydesc webassemblydisassembler webassemblyinfo windowsmanifest x86 x86asmparser x86codegen x86desc x86disassembler x86info xray" "--cc" "" "--cxx" "" "--cflags" "" "--remote-test-client" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/remote-test-client" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--channel" "nightly" "--color" "always"


Build completed unsuccessfully in 0:29:44

bors · 2021-08-12T04:30:36Z

💔 Test failed - checks-actions

erikdesjardins · 2021-08-13T16:29:07Z

^ looks like more network issues:

thread 'main' panicked at 'client.read_exact(&mut header) failed with Connection reset by peer (os error 104)', src/tools/remote-test-client/src/main.rs:310:9

Aaron1011 · 2021-08-14T19:05:33Z

@bors retry

bors · 2021-08-14T19:59:12Z

⌛ Testing commit 585e4ae with merge 8007b50...

bors · 2021-08-14T22:29:21Z

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 8007b50 to master...

…ts, r=oli-obk" This reverts commit 8007b50, reversing changes made to e55c13e.

Revert "Auto merge of rust-lang#83417 - erikdesjardins:enableremovezsts, r=oli-obk" This reverts commit 8007b50, reversing changes made to e55c13e. Fixes rust-lang#88043 r? `@oli-obk`

…removezsts, r=oli-obk"" This reverts commit 8e11199.

Reenable RemoveZsts Now that the underlying issue has been fixed by rust-lang#88124, we can reland rust-lang#83417. r? `@oli-obk`

rust-highfive assigned oli-obk Mar 23, 2021

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

camelid added the A-mir-opt Area: MIR optimizations label Mar 23, 2021

camelid reviewed Mar 23, 2021

View reviewed changes

erikdesjardins marked this pull request as draft March 23, 2021 20:43

erikdesjardins mentioned this pull request Mar 23, 2021

Add tests related to erroneous consts in dead code with --emit=mir #83423

Closed

This comment has been minimized.

Sign in to view

rustbot added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 23, 2021

oli-obk added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. labels Mar 30, 2021

Run RemoveZsts at mir-opt-level=1

585e4ae

Effectively reverts commit 6960bc9.

erikdesjardins force-pushed the enableremovezsts branch from 429a536 to 585e4ae Compare August 7, 2021 16:50

erikdesjardins marked this pull request as ready for review August 7, 2021 17:43

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

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 Aug 9, 2021

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 Aug 12, 2021

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 Aug 14, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 14, 2021

bors merged commit 8007b50 into rust-lang:master Aug 14, 2021

rustbot added this to the 1.56.0 milestone Aug 14, 2021

bors mentioned this pull request Aug 14, 2021

Fix debugger stepping behavior with match expressions #87832

Merged

erikdesjardins deleted the enableremovezsts branch August 14, 2021 23:01

dtolnay mentioned this pull request Aug 15, 2021

LLVM crash: Basic Block does not have terminator! #88043

Closed

erikdesjardins added a commit to erikdesjardins/rust that referenced this pull request Aug 16, 2021

Revert "Auto merge of rust-lang#83417 - erikdesjardins:enableremovezs…

8e11199

…ts, r=oli-obk" This reverts commit 8007b50, reversing changes made to e55c13e.

erikdesjardins added a commit to erikdesjardins/rust that referenced this pull request Aug 20, 2021

Revert "Revert "Auto merge of rust-lang#83417 - erikdesjardins:enable…

6ea536d

…removezsts, r=oli-obk"" This reverts commit 8e11199.

erikdesjardins mentioned this pull request Aug 20, 2021

Reenable RemoveZsts #88176

Merged

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2021

Auto merge of rust-lang#88176 - erikdesjardins:rezst, r=oli-obk

914a1e2

Reenable RemoveZsts Now that the underlying issue has been fixed by rust-lang#88124, we can reland rust-lang#83417. r? `@oli-obk`

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run RemoveZsts pass at mir-opt-level=1 #83417

Run RemoveZsts pass at mir-opt-level=1 #83417

erikdesjardins commented Mar 23, 2021 •

edited

Loading

camelid Mar 23, 2021

erikdesjardins Mar 23, 2021 •

edited

Loading

camelid Mar 23, 2021

erikdesjardins Mar 23, 2021 •

edited

Loading

tmiasko Mar 23, 2021

camelid Mar 23, 2021

oli-obk Mar 25, 2021

oli-obk Mar 25, 2021

erikdesjardins commented Mar 23, 2021 •

edited

Loading

This comment has been minimized.

camelid commented Mar 23, 2021

oli-obk commented Mar 30, 2021

erikdesjardins commented Mar 31, 2021

oli-obk commented Apr 1, 2021

bors commented May 31, 2021

erikdesjardins commented Aug 7, 2021 •

edited

Loading

oli-obk commented Aug 9, 2021

bors commented Aug 9, 2021

bors commented Aug 9, 2021

bors commented Aug 9, 2021

rust-log-analyzer commented Aug 9, 2021

oli-obk commented Aug 9, 2021

erikdesjardins commented Aug 11, 2021

bors commented Aug 12, 2021

rust-log-analyzer commented Aug 12, 2021

bors commented Aug 12, 2021

erikdesjardins commented Aug 13, 2021

Aaron1011 commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

	const VOID: ! = panic!();
	//~^ WARN any use of this value will cause an error
	//~\| WARN this was previously accepted by the compiler but is being phased out

Run RemoveZsts pass at mir-opt-level=1 #83417

Run RemoveZsts pass at mir-opt-level=1 #83417

Conversation

erikdesjardins commented Mar 23, 2021 • edited Loading

camelid Mar 23, 2021

Choose a reason for hiding this comment

erikdesjardins Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

camelid Mar 23, 2021

Choose a reason for hiding this comment

erikdesjardins Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

tmiasko Mar 23, 2021

Choose a reason for hiding this comment

camelid Mar 23, 2021

Choose a reason for hiding this comment

oli-obk Mar 25, 2021

Choose a reason for hiding this comment

oli-obk Mar 25, 2021

Choose a reason for hiding this comment

erikdesjardins commented Mar 23, 2021 • edited Loading

This comment has been minimized.

camelid commented Mar 23, 2021

oli-obk commented Mar 30, 2021

erikdesjardins commented Mar 31, 2021

oli-obk commented Apr 1, 2021

bors commented May 31, 2021

erikdesjardins commented Aug 7, 2021 • edited Loading

oli-obk commented Aug 9, 2021

bors commented Aug 9, 2021

bors commented Aug 9, 2021

bors commented Aug 9, 2021

rust-log-analyzer commented Aug 9, 2021

oli-obk commented Aug 9, 2021

erikdesjardins commented Aug 11, 2021

bors commented Aug 12, 2021

rust-log-analyzer commented Aug 12, 2021

bors commented Aug 12, 2021

erikdesjardins commented Aug 13, 2021

Aaron1011 commented Aug 14, 2021

bors commented Aug 14, 2021

bors commented Aug 14, 2021

erikdesjardins commented Mar 23, 2021 •

edited

Loading

erikdesjardins Mar 23, 2021 •

edited

Loading

erikdesjardins Mar 23, 2021 •

edited

Loading

erikdesjardins commented Mar 23, 2021 •

edited

Loading

erikdesjardins commented Aug 7, 2021 •

edited

Loading