Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove the -Zinsert-sideeffect #82884

Merged
merged 1 commit into from
Mar 10, 2021
Merged

Remove the -Zinsert-sideeffect #82884

merged 1 commit into from
Mar 10, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Mar 8, 2021

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
Copy link
Collaborator

r? @estebank

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

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

nagisa commented Mar 8, 2021

cc @nikic @Mark-Simulacrum

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from 6f62d9b to cc930aa Compare March 8, 2021 00:25
@Mark-Simulacrum
Copy link
Member

@bors try @rust-timer queue

I would expect either improved or unchanged performance.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

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

bors commented Mar 8, 2021

⌛ Trying commit cc930aaa931c6607ed17d699afc63108d8b6359a with merge 0ed3adf212fcc9697c642f4043b36a48606909eb...

@nagisa nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from cc930aa to e40911f Compare March 8, 2021 00:57
estebank
estebank reviewed Mar 8, 2021
View reviewed changes
compiler/rustc_codegen_ssa/src/mir/block.rs
} else {
helper.maybe_sideeffect(self.mir, &mut bx, &[target]);
// NB: the `sideeffect` currently checks for the LLVM version used internally.
bx.sideeffect();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nikic
Copy link
Contributor

nikic commented Mar 8, 2021

@bors retry

@bors 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
@nikic
Copy link
Contributor

nikic commented Mar 8, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

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

@bors
Copy link
Contributor

bors commented Mar 8, 2021

⌛ Trying commit e40911fe9a3461247904cfb27095cb71e4c28bf5 with merge f9ee4d76ec0bea8c917cb9e0afd7aa39c4d7ccb1...

@bors
Copy link
Contributor

bors commented Mar 8, 2021

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

@rust-timer
Copy link
Collaborator

Queued f9ee4d76ec0bea8c917cb9e0afd7aa39c4d7ccb1 with parent 76c500e, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@rustbot 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
@nikic
Copy link
Contributor

nikic commented Mar 8, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Mar 8, 2021

📌 Commit e40911fe9a3461247904cfb27095cb71e4c28bf5 has been approved by nikic

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2021
@bors
Copy link
Contributor

bors commented Mar 9, 2021

⌛ Testing commit e40911fe9a3461247904cfb27095cb71e4c28bf5 with merge 9383cb7dc5ec233f61935cb6dd2fea9d363422ea...

@bors
Copy link
Contributor

bors commented Mar 9, 2021

💔 Test failed - checks-actions

@bors 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
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa
Copy link
Member Author

nagisa commented Mar 9, 2021

@bors retry

@bors 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
Copy link
Contributor

bors commented Mar 10, 2021

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

@bors 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
Remove the -Zinsert-sideeffect
0517acd 
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 nagisa force-pushed the nagisa/remove-most-of-sideeffect-inserts branch from e40911f to 0517acd Compare March 10, 2021 10:22
@nagisa
Copy link
Member Author

nagisa commented Mar 10, 2021

@bors r=nikic

@bors
Copy link
Contributor

bors commented Mar 10, 2021

📌 Commit 0517acd has been approved by nikic

@bors 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
Copy link
Contributor

bors commented Mar 10, 2021

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

@bors
Copy link
Contributor

bors commented Mar 10, 2021

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 10, 2021
@bors bors merged commit 5fe790e into rust-lang:master Mar 10, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 10, 2021
@Sobeston Sobeston mentioned this pull request Sep 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@estebank estebank estebank left review comments

Assignees

@estebank estebank

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
9 participants
@nagisa @rust-highfive @rust-log-analyzer @Mark-Simulacrum @rust-timer @bors @nikic @estebank @rustbot