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.

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

Disable LLVM verification by default #51230

Merged
merged 3 commits into from
Jul 11, 2018
Merged

Conversation

nikic
Copy link
Contributor

@nikic nikic commented May 30, 2018

Currently -Z no-verify only controls IR verification prior to LLVM codegen, while verification is performed unconditionally both before and after linking with (Thin)LTO.

Also wondering what the sentiment is on disabling verification by default (and e.g. only enabling it on ALT builds with assertions). This does not seem terribly useful outside of rustc development and it does seem to show up in profiles (at something like 3%).

EDIT: A table showing the various configurations and what is enabled when.

Configuration Dynamic verification performed LLVM static assertions compiled in
alt builds yes
nightly builds no
stable builds no
CI builds
dev builds in a checkout

@rust-highfive
Copy link
Collaborator

r? @pnkfelix

(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 May 30, 2018
@pietroalbini
Copy link
Member

Ping from triage @pnkfelix! This PR needs your review.

@TimNN
Copy link
Contributor

TimNN commented Jun 12, 2018

Ping from triage, @pnkfelix / @rust-lang/compiler, this PR needs your review.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2018

Also wondering what the sentiment is on disabling verification by default (and e.g. only enabling it on ALT builds with assertions). This does not seem terribly useful outside of rustc development and it does seem to show up in profiles (at something like 3%).

Sidenote: I do dislike the !config.no_verify condition.

If you change this to a verify (and also the flag), then it would be off by default. 3% gains seem nice, let's do it and do a bors perf run

Currently -Z no-verify only controls IR verification prior to
LLVM codegen, while verification is performed unconditionally
both before and after linking with (Thin)LTO.
This disables IR verification by default.
@nikic
Copy link
Contributor Author

nikic commented Jun 12, 2018

I've pushed a variant that renames -Z no-verify to -Z verify-llvm-ir.

Should I add a config.toml flag for this as well?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 12, 2018

Should I add a config.toml flag for this as well?

seems sensible, people will want to turn this on for debugging llvm crashes. Can we turn this on on the dev-channel automatically?

@nikic
Copy link
Contributor Author

nikic commented Jun 12, 2018

I've added an (untested) implementation of the config.toml flag. What does enabling it on the dev-channel mean?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 13, 2018

@bors try

let's do a perf run

@bors
Copy link
Contributor

bors commented Jun 13, 2018

⌛ Trying commit 3f18a41 with merge 6ba43c9...

bors added a commit that referenced this pull request Jun 13, 2018
Respect -Z no-verify during LTO

Currently -Z no-verify only controls IR verification prior to LLVM codegen, while verification is performed unconditionally both before and after linking with (Thin)LTO.

Also wondering what the sentiment is on disabling verification by default (and e.g. only enabling it on ALT builds with assertions). This does not seem terribly useful outside of rustc development and it does seem to show up in profiles (at something like 3%).
@bors
Copy link
Contributor

bors commented Jun 13, 2018

☀️ Test successful - status-travis
State: approved= try=True

@pietroalbini
Copy link
Member

cc @Mark-Simulacrum

@pietroalbini pietroalbini added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 13, 2018
@Mark-Simulacrum
Copy link
Member

Perf queued.

@nikic
Copy link
Contributor Author

nikic commented Jun 14, 2018

Perf results: http://perf.rust-lang.org/compare.html?start=cd1105437cd433c12ae5132c9632e01d387b2384&end=6ba43c91a8c395f7247a7605c23dfcb028c0bfc9&stat=instructions%3Au

So 1-3% improvement across the board (apart from check builds, which don't touch LLVM).

@kennytm kennytm 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 Jun 14, 2018
@pietroalbini
Copy link
Member

Ping from triage @pnkfelix! This PR needs your review.

@pnkfelix
Copy link
Member

I'm a little wary of turning off verification unconditionally across the board.

I understand the sentiment that it is only useful for compiler development, but in practice I think it has caught bugs for us.

Could we perhaps leave verification on by default in the nightly channel, and turn it off by default in the stable (and beta?) channels?

Nominating for discussion at rustc team meeting.

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. I-nominated I-needs-decision Issue: In need of a decision. labels Jun 18, 2018
@rfcbot rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 8, 2018
@pnkfelix
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jul 10, 2018

📌 Commit 3f18a41 has been approved by pnkfelix

@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 Jul 10, 2018
@pnkfelix pnkfelix removed the I-needs-decision Issue: In need of a decision. label Jul 10, 2018
@bors
Copy link
Contributor

bors commented Jul 11, 2018

⌛ Testing commit 3f18a41 with merge 63d9e0003894440cad59003a0938d4c30a4d6f53...

@bors
Copy link
Contributor

bors commented Jul 11, 2018

💔 Test failed - status-travis

@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 Jul 11, 2018
@rust-highfive
Copy link
Collaborator

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:31:42] thread '<unnamed>' panicked at 'specified instant was later than self', libstd/sys/unix/time.rs:292:17
[01:31:42] test time::tests::system_time_math ... ok
[01:31:47] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:31:48] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:32:18] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

1 similar comment
@rust-highfive
Copy link
Collaborator

The job arm-android of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[01:31:42] thread '<unnamed>' panicked at 'specified instant was later than self', libstd/sys/unix/time.rs:292:17
[01:31:42] test time::tests::system_time_math ... ok
[01:31:47] test sync::mpsc::tests::stress_recv_timeout_two_threads ... ok
[01:31:48] test collections::hash::map::test_map::test_lots_of_insertions ... ok
[01:32:18] test process::tests::test_process_output_fail_to_start ... test process::tests::test_process_output_fail_to_start has been running for over 60 seconds
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@kennytm
Copy link
Member

kennytm commented Jul 11, 2018

@bors retry #43283

@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 Jul 11, 2018
@bors
Copy link
Contributor

bors commented Jul 11, 2018

⌛ Testing commit 3f18a41 with merge 11432ba...

bors added a commit that referenced this pull request Jul 11, 2018
Disable LLVM verification by default

Currently -Z no-verify only controls IR verification prior to LLVM codegen, while verification is performed unconditionally both before and after linking with (Thin)LTO.

Also wondering what the sentiment is on disabling verification by default (and e.g. only enabling it on ALT builds with assertions). This does not seem terribly useful outside of rustc development and it does seem to show up in profiles (at something like 3%).

**EDIT:** A table showing the various configurations and what is enabled when.

| Configuration | Dynamic verification performed | LLVM static assertions compiled in |
| --- | --- | --- |
| alt builds | | yes |
| nightly builds | | no |
| stable builds | | no |
| CI builds | | |
| dev builds in a checkout | | |
@bors
Copy link
Contributor

bors commented Jul 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: pnkfelix
Pushing 11432ba to master...

@bors bors merged commit 3f18a41 into rust-lang:master Jul 11, 2018
@bors bors mentioned this pull request Jul 11, 2018
@nnethercote
Copy link
Contributor

perf.rust-lang.org results show 1--3% speedups on lots of opt and debug benchmark builds:
https://perf.rust-lang.org/compare.html?start=989fa053895a27fd40896335224b619843b7e58a&end=66787e05242d86e0bcfa227265559330c45cdc42&stat=instructions:u

Nice work!

@gnzlbg
Copy link
Contributor

gnzlbg commented Jul 12, 2018

Is there a nightly build with all verification, assertions (debug_asserts in rustc, in std, ...), LLVM assertions, etc. turned on that one can use during development? Like every now and then LLVM crashes and the first step to debug the issue is "recompile the world with LLVM with assertions enabled", which basically adds over one hour of cost upfront to start debugging an issue that might just end up being an user error. I would find something like rustup default nightly-dev very helpful even if we only would keep around the artifacts for the last nightly.

@Mark-Simulacrum
Copy link
Member

I don't believe nightly builds exist but you can use https://crates.io/crates/rustup-toolchain-install-master with the --alt flag to get what you want I think, though not sure if all of what you want is enabled in those (I think debug asserts in rustc might not be).

kennytm added a commit to kennytm/rust that referenced this pull request Oct 18, 2018
…crum

Improve verify_llvm_ir config option

LLVM IR verification has been disabled by default in rust-lang#51230. However, the implementation doesn't quite match what was discussed in the discussion. This patch implements two changes:

* Make `verify_llvm_ir` influence the behavior of the compiled rustc binary, rather than just the rustc build system. That is, if `verify_llvm_ir=true`, even manual invocations of the built rustc will verify LLVM IR.
* Enable verification of LLVM IR in CI, for non-deploy and deploy-alt builds. This is similar to how LLVM assertions are handled.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.