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

Up the LLVM #36508

Merged
merged 1 commit into from
Sep 17, 2016
Merged

Up the LLVM #36508

merged 1 commit into from
Sep 17, 2016

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Sep 15, 2016

Fixes #36474

The relevant patch to rust-llvm is at rust-lang/llvm#51

r? @alexcrichton

@nagisa
Copy link
Member Author

nagisa commented Sep 15, 2016

Nominating for beta, as it fixes a regression.

@brson brson added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 16, 2016
@alexcrichton
Copy link
Member

Hm so we've got two parallel updates to the LLVM submodule right now. Both this and the wasm backend. I'd feel pretty uncomfortable about backporting the emscripten backend. To be honest our LLVM backend on beta is at rust-lang/llvm@80ad955 which is way off from what's on nightly... I guess all that's to say:

  • To land this on beta, we may want a different backport? To the rust-llvm-2016-03-13 I think.
  • To land on master we may want to either use a rev that doesn't have emscripten or wait for that patch to land first.

@nagisa
Copy link
Member Author

nagisa commented Sep 16, 2016

  • To land this on beta, we may want a different backport? To the rust-llvm-2016-03-13 I think.
  • To land on master we may want to either use a rev that doesn't have emscripten or wait for that patch to land first.

I’d gladly backport the LLVM patch as many times as necessary (its a simple one-liner after all), but I’ll need new branches from 786aad1 for beta and from 16b79d01 for nightly – both of these commits seem to be in 07-09 branch only.

(the branches with the backported patch at relevant base commits are https://github.com/nagisa/llvm/tree/backport-281650-beta for beta and https://github.com/nagisa/llvm/tree/backport-281650-master for nightly)

wait for that patch to land first

This is a fix for beta regression and we do 🚋 roll-over in just a bit over a week. I’m fine with waiting before backporting the fix to the nightlies’ LLVM (in which case you might have to live through 2 beta backports of the same patch), but beta rust has to get that LLVM patch ASAP.

@brson
Copy link
Contributor

brson commented Sep 16, 2016

I rebased the 2016-07-09 branch to make your patch appear ahead of the fastcomp in the history, so you don't need to worry about any fallout from it, but you'll need to update this PR with the correct new commit.

From what I can tell @alexcrichton was mistaken about beta being on a different LLVM. It looks to me like the most recent commit is rust-lang/llvm@eee68ea, which is only one merge behind nightly, this fix to run GVN again rust-lang/llvm@cc2009f. If that looks right to you @nagisa @eddyb we should just be able to take the same commit on both master and beta.

@nagisa
Copy link
Member Author

nagisa commented Sep 16, 2016

Updated the LLVM commit; this is good to go.

@eddyb
Copy link
Member

eddyb commented Sep 17, 2016

@bors r+ p=1

@bors
Copy link
Contributor

bors commented Sep 17, 2016

📌 Commit e5c4d78 has been approved by eddyb

@bors
Copy link
Contributor

bors commented Sep 17, 2016

⌛ Testing commit e5c4d78 with merge 3a35e66...

@bors
Copy link
Contributor

bors commented Sep 17, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@nagisa
Copy link
Member Author

nagisa commented Sep 17, 2016

The bot in question did not rebuild LLVM. What do?

EDIT: it seems like all the rustbuild bots decided that rebuilding LLVM is not necessary. Can we skip/ignore them somehow?

@pmarcelll
Copy link
Contributor

@nagisa IIRC src/rustllvm/llvm-auto-clean-trigger should also be updated.

@eddyb
Copy link
Member

eddyb commented Sep 17, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2016

📌 Commit d104e5b has been approved by eddyb

@bors
Copy link
Contributor

bors commented Sep 17, 2016

⌛ Testing commit d104e5b with merge 32571c0...

bors added a commit that referenced this pull request Sep 17, 2016
Up the LLVM

Fixes #36474

The relevant patch to rust-llvm is at rust-lang/llvm#51

r? @alexcrichton
@bors bors merged commit d104e5b into rust-lang:master Sep 17, 2016
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 19, 2016
@nikomatsakis
Copy link
Contributor

Accepting for beta. Trivial patch (hiding LLVM changes of course...), regression.

@brson
Copy link
Contributor

brson commented Sep 19, 2016

After looking into this further, @alexcrichton was right that beta and master are on two different merge bases. The LLVM patch is going to need to be ported to the beta LLVM specifically.

@brson
Copy link
Contributor

brson commented Sep 19, 2016

Actually, they do look like the have the same merge-base. For some reason when cherry-picking this git says "Failed to merge submodule src/llvm (commits don't follow merge-base)". I don't understand why.

@brson brson mentioned this pull request Sep 19, 2016
@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants