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

Upgrade to LLVM's master branch (LLVM 7) #51966

Merged
merged 1 commit into from
Jul 11, 2018
Merged

Conversation

alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Jul 1, 2018

Current status

Blocked on a performance regression. The performance regression has an upstream LLVM issue and has also been bisected to an LLVM revision.

Ready to merge!


This commit upgrades the main LLVM submodule to LLVM's current master branch.
The LLD submodule is updated in tandem as well as compiler-builtins.

Along the way support was also added for LLVM 7's new features. This primarily
includes the support for custom section concatenation natively in LLD so we now
add wasm custom sections in LLVM IR rather than having custom support in rustc
itself for doing so.

Some other miscellaneous changes are:

  • We now pass --gc-sections to wasm-ld
  • The optimization level is now passed to wasm-ld
  • A --stack-first option is passed to LLD to have stack overflow always cause
    a trap instead of corrupting static data
  • The wasm target for LLVM switched to wasm32-unknown-unknown.
  • The syntax for aligned pointers has changed in LLVM IR and tests are updated
    to reflect this.
  • The thumbv6m-none-eabi target is disabled due to an LLVM bug

Nowadays we've been mostly only upgrading whenever there's a major release of
LLVM but enough changes have been happening on the wasm target that there's been
growing motivation for quite some time now to upgrade out version of LLD. To
upgrade LLD, however, we need to upgrade LLVM to avoid needing to build yet
another version of LLVM on the builders.

The revision of LLVM in use here is arbitrarily chosen. We will likely need to
continue to update it over time if and when we discover bugs. Once LLVM 7 is
fully released we can switch to that channel as well.

cc #50543

@rust-highfive
Copy link
Collaborator

r? @varkor

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive
Copy link
Collaborator

warning Warning warning

  • These commits modify submodules.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 1, 2018
@alexcrichton
Copy link
Member Author

alexcrichton commented Jul 1, 2018

cc @fitzgen for the LLD update
cc @japaric for the proposed removal of thumbv6m-none-eabi

@rust-highfive

This comment has been minimized.

@kennytm

This comment has been minimized.

@jamesmunns

This comment has been minimized.

@alexcrichton

This comment has been minimized.

@alexcrichton

This comment has been minimized.

OptLevel::Less => "-O1",
OptLevel::Default => "-O2",
OptLevel::Aggressive => "-O3",
OptLevel::Size => "-O2",
Copy link
Member

Choose a reason for hiding this comment

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

If this is intentional, it'd be helpful to have a comment here to explain why -O2 is used for the size opt levels (as opposed to -Os and -Oz).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing!

@varkor
Copy link
Member

varkor commented Jul 2, 2018

Everything looks reasonable to me, but it'd be better to have someone more familiar check over this.
r? @michaelwoerister

@gnzlbg

This comment has been minimized.

@alexcrichton alexcrichton force-pushed the llvm7 branch 2 times, most recently from abbd4fc to aa667e3 Compare July 2, 2018 14:16
@alexcrichton alexcrichton force-pushed the llvm7 branch 2 times, most recently from 1e57200 to f29cd71 Compare July 2, 2018 21:43
@alexcrichton
Copy link
Member Author

Now that https://bugs.llvm.org/show_bug.cgi?id=37382 is fixed I've updated to bring back the thumbv6m build

@alexcrichton
Copy link
Member Author

Oh one thing I forgot to mention in the OP as well is that there's a change to src/librustdoc/fold.rs which is a hack around either a bug in LLVM or a bug in our code generation. I was unable to reduce it down after a week or so of attempting various strategies, so I've opted to go ahead and apply the hack and see if we can find a reduced case in the wild.

@alexcrichton
Copy link
Member Author

@bors: try

@bors
Copy link
Contributor

bors commented Jul 2, 2018

⌛ Trying commit f29cd71c42d5099fd693e040f3802dc1c9548070 with merge b77640fe5342825939a1222f847c753f7b3c0968...

@bors

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 2, 2018
@bors
Copy link
Contributor

bors commented Jul 11, 2018

⌛ Testing commit 42eb850 with merge ae5b629...

@bors
Copy link
Contributor

bors commented Jul 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing ae5b629 to master...

@bors bors merged commit 42eb850 into rust-lang:master Jul 11, 2018
bors added a commit that referenced this pull request Jul 11, 2018
Set opt-level = 3 the third time.

This PR reverts #51165 (set -O2 to fix #50867),
which reverted #50329 (set -O3),
which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now),
which reverted #42123 (set -O2 to fix spurious Windows segfaults),
which reverted #41967 (set -O3).

Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
@alexcrichton alexcrichton deleted the llvm7 branch July 11, 2018 14:08
@alexcrichton
Copy link
Member Author

Hm, this was a suspiciously smooth upgrade...

@varkor
Copy link
Member

varkor commented Jul 12, 2018

llvm-rebuild-trigger wasn't modified as part of this PR, which was an oversight. Is it worth updating in a standalone PR, to ease the transition for anyone who isn't yet up to date with master?

@alexcrichton
Copy link
Member Author

Sure!

kennytm added a commit to kennytm/rust that referenced this pull request Jul 12, 2018
…excrichton

Update llvm-rebuild-trigger in light of LLVM 7 upgrade

Not triggering a LLVM rebuild [since the LLVM 7 upgrade](rust-lang#51966 (comment)) causes builds of rustc to fail.

r? @alexcrichton
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request Jul 13, 2018
…excrichton

Update llvm-rebuild-trigger in light of LLVM 7 upgrade

Not triggering a LLVM rebuild [since the LLVM 7 upgrade](rust-lang#51966 (comment)) causes builds of rustc to fail.

r? @alexcrichton
@alexcrichton alexcrichton mentioned this pull request Jul 13, 2018
13 tasks
kennytm added a commit to kennytm/rust that referenced this pull request Jul 13, 2018
…excrichton

Update llvm-rebuild-trigger in light of LLVM 7 upgrade

Not triggering a LLVM rebuild [since the LLVM 7 upgrade](rust-lang#51966 (comment)) causes builds of rustc to fail.

r? @alexcrichton
bors added a commit that referenced this pull request Jul 14, 2018
Set opt-level = 3 the third time.

This PR reverts #51165 (set -O2 for fixing #50867),
which reverted #50329 (set -O3),
which was second attempt of #48204 (set -O3, closed due to Windows segfault that is fixed now),
which reverted #42123 (set -O2 to fix spurious Windows segfaults),
which reverted #41967 (set -O3).

Since we have found the root cause of #50867, this optimization could be tried again.

Last time we've found that setting -O3 regressed the wall time of NLL (#50329 (comment)), so we may need another perf run to confirm. I'd like to check this *after* the LLVM 7 upgrade #51966 has been merged, so marking this as <kbd>S-blocked</kbd> for now.
@bradjc bradjc mentioned this pull request Jul 23, 2018
7 tasks
@@ -57,7 +57,7 @@ struct CheckAttrVisitor<'a, 'tcx: 'a> {
impl<'a, 'tcx> CheckAttrVisitor<'a, 'tcx> {
/// Check any attribute.
fn check_attributes(&self, item: &hir::Item, target: Target) {
if target == Target::Fn {
if target == Target::Fn || target == Target::Const {
Copy link
Member

Choose a reason for hiding this comment

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

@alexcrichton: what motivated this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh... well it's been over a year, so this is way out of cache. I suspect this was done to handle wasm attribute validation, but you are wondering you can remove this then seems fine to remove it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.