Remove the -Zinsert-sideeffect by nagisa · Pull Request #82884 · rust-lang/rust

nagisa · 2021-03-08T00:04:09Z

This removes all of the code we had in place to work-around LLVM's
handling of forward progress. From this removal excluded is a workaround
where we'd insert a sideeffect into clearly infinite loops such as
loop {}. This code remains conditionally effective when the LLVM
version is earlier than 12.0, which fixed the forward progress related
miscompilations at their root.

rust-highfive · 2021-03-08T00:04:12Z

r? @estebank

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

nagisa · 2021-03-08T00:04:39Z

cc @nikic @Mark-Simulacrum

compiler/rustc_codegen_llvm/src/intrinsic.rs

Mark-Simulacrum · 2021-03-08T00:42:48Z

@bors try @rust-timer queue

I would expect either improved or unchanged performance.

rust-timer · 2021-03-08T00:42:49Z

Awaiting bors try build completion.

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

bors · 2021-03-08T00:42:56Z

⌛ Trying commit cc930aaa931c6607ed17d699afc63108d8b6359a with merge 0ed3adf212fcc9697c642f4043b36a48606909eb...

estebank · 2021-03-08T01:01:26Z

compiler/rustc_codegen_ssa/src/mir/block.rs

So this is the only call to sideeffect left in the codebase? Won't this cause a miscompilation for the removed cases when run under LLVM <12?

All the other calls did not generate a sideeffect unless an unstable flag was specified.

nikic · 2021-03-08T09:58:46Z

@bors retry

nikic · 2021-03-08T10:15:24Z

@bors try @rust-timer queue

rust-timer · 2021-03-08T10:15:25Z

Awaiting bors try build completion.

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

bors · 2021-03-08T10:15:33Z

⌛ Trying commit e40911fe9a3461247904cfb27095cb71e4c28bf5 with merge f9ee4d76ec0bea8c917cb9e0afd7aa39c4d7ccb1...

bors · 2021-03-08T11:05:51Z

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

rust-timer · 2021-03-08T11:05:53Z

Queued f9ee4d76ec0bea8c917cb9e0afd7aa39c4d7ccb1 with parent 76c500e, future comparison URL.

rust-timer · 2021-03-08T12:40:09Z

Finished benchmarking try commit (f9ee4d76ec0bea8c917cb9e0afd7aa39c4d7ccb1): 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 label: +S-waiting-on-review -S-waiting-on-perf

nikic · 2021-03-08T13:32:50Z

@bors r+

bors · 2021-03-08T13:32:51Z

📌 Commit e40911fe9a3461247904cfb27095cb71e4c28bf5 has been approved by nikic

bors · 2021-03-09T12:34:59Z

⌛ Testing commit e40911fe9a3461247904cfb27095cb71e4c28bf5 with merge 9383cb7dc5ec233f61935cb6dd2fea9d363422ea...

bors · 2021-03-09T16:10:37Z

💔 Test failed - checks-actions

nagisa · 2021-03-09T17:00:52Z

@bors retry

bors · 2021-03-10T04:38:50Z

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

This removes all of the code we had in place to work-around LLVM's handling of forward progress. From this removal excluded is a workaround where we'd insert a `sideeffect` into clearly infinite loops such as `loop {}`. This code remains conditionally effective when the LLVM version is earlier than 12.0, which fixed the forward progress related miscompilations at their root.

nagisa · 2021-03-10T10:30:41Z

@bors r=nikic

bors · 2021-03-10T10:30:43Z

📌 Commit 0517acd has been approved by nikic

bors · 2021-03-10T14:03:06Z

⌛ Testing commit 0517acd with merge 5fe790e...

bors · 2021-03-10T16:43:57Z

☀️ Test successful - checks-actions
Approved by: nikic
Pushing 5fe790e to master...

rust-highfive assigned estebank Mar 8, 2021

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

This comment has been minimized.

Sign in to view

nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from 6f62d9b to cc930aa Compare March 8, 2021 00:25

Mark-Simulacrum reviewed Mar 8, 2021

View reviewed changes

compiler/rustc_codegen_llvm/src/intrinsic.rs Outdated Show resolved Hide resolved

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 8, 2021

nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from cc930aa to e40911f Compare March 8, 2021 00:57

estebank reviewed Mar 8, 2021

View reviewed changes

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 Mar 8, 2021

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Mar 8, 2021

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 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 Mar 9, 2021

This comment has been minimized.

Sign in to view

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

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 Mar 10, 2021

nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from e40911f to 0517acd Compare March 10, 2021 10:22

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 10, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 10, 2021

bors merged commit 5fe790e into rust-lang:master Mar 10, 2021

rustbot added this to the 1.52.0 milestone Mar 10, 2021

Sobeston mentioned this pull request Sep 22, 2021

infinite loop optimized away unless function is exported ziglang/zig#1658

Closed

Uh oh!

Conversation

nagisa commented Mar 8, 2021

Uh oh!

rust-highfive commented Mar 8, 2021

Uh oh!

nagisa commented Mar 8, 2021

Uh oh!

This comment has been minimized.

Uh oh!

Mark-Simulacrum commented Mar 8, 2021

Uh oh!

rust-timer commented Mar 8, 2021

Uh oh!

bors commented Mar 8, 2021

Uh oh!

estebank Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

nagisa Mar 8, 2021

Choose a reason for hiding this comment

Uh oh!

nikic commented Mar 8, 2021

Uh oh!

nikic commented Mar 8, 2021

Uh oh!

rust-timer commented Mar 8, 2021

Uh oh!

bors commented Mar 8, 2021

Uh oh!

bors commented Mar 8, 2021

Uh oh!

rust-timer commented Mar 8, 2021

Uh oh!

rust-timer commented Mar 8, 2021

Uh oh!

nikic commented Mar 8, 2021

Uh oh!

bors commented Mar 8, 2021

Uh oh!

bors commented Mar 9, 2021

Uh oh!

bors commented Mar 9, 2021

Uh oh!

This comment has been minimized.

nagisa commented Mar 9, 2021

Uh oh!

bors commented Mar 10, 2021

Uh oh!

nagisa commented Mar 10, 2021

Uh oh!

bors commented Mar 10, 2021

Uh oh!

bors commented Mar 10, 2021

Uh oh!

bors commented Mar 10, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants