search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.28k stars 12.71k forks source link

Upgrade to LLVM 4.0 #37609

Closed shepmaster closed 7 years ago

shepmaster commented 8 years ago

The AVR-Rust post continues to track AVR-LLVM, which is tracking upstream LLVM. That means we are running into changes from the future LLVM 4.0. I figured we could open up this issue now to track the changes required to stay compatible.

This is the current list of upgrade hazards. Note that some of the text may be nonsensical as I don't truly grok all the internals of LLVM so some changes are wild stabs in the dark. It's also possible that some failed fix for earlier problems actually caused later problems:

A WIP PR that performs the upgrade! #40123

Some issues whose resolution is believed to be blocked on LLVM 4.0:

brson commented 8 years ago

Next time we do this upgrade we'll need emscripten to do it to. It would be good if we could pull in a working llvm wasm backend in the same upgrade, but that may not be ready before the end of the year.

hanna-kruppe commented 7 years ago

For the record, I am also working with a fork of LLVM that is tracking upstream (not AVR-LLVM but similar in spirit) and am starting to work on this now. I'm looking through the AVR-Rust commits and I'd love to coordinate --- feel free to ping me on IRC (rkruppe) or via mail!

TimNN commented 7 years ago

Note that there is a regression reported against llvm 4.0 regarding the powi intrinsic on armhf targets:

https://llvm.org/bugs/show_bug.cgi?id=30543

I'm explicitly mentioning this here, because there were previously problems with it, see #37559 and related issues. Also cc rust-lang-nursery/compiler-builtins#116, an issue about the same problem in the compiler-rt rust port.

alexcrichton commented 7 years ago

To clarify what I think the @TimNN is referring to (correct me if I'm wrong)

So it seems that if the LLVM change is not reverted (expected powisf2 to be aapcs) then we need to update our definition.

This is going to be a huge PITA for supporting multiple LLVM versions as well. We need to select the calling convention depending on the LLVM version as well...

shepmaster commented 7 years ago

and I'd love to coordinate

@rkruppe Any objections to doing that coordination here on GitHub, or on one of our forks?

hanna-kruppe commented 7 years ago

I don't have strong preferences for the communication channel.

hanna-kruppe commented 7 years ago

37831 should address what is listed as "AttrBuilder API changes" above (the core of the change is in how attributes are represented).

hanna-kruppe commented 7 years ago

@shepmaster I would prefer to upstream LLVM 4.0 compatibility patches sooner rather than later. That includes my own work obviously, but I'm also looking at the commits I cherry-picked from AVR-Rust. In particular, https://github.com/avr-rust/rust/commit/3e1e4a49bb89526db16151a6cdf6950a140c6415 looks like a reasonable simplification even independent of LLVM 4.0 compatibility, and https://github.com/avr-rust/rust/commit/c32f9c15d4ce3b380e58fa088c914b505e8b966e looks like a nice little self-contained fix. Could you create PRs for those? (If you don't have the time but are otherwise fine with upstreaming, I'd be happy to manage that, giving proper credit of course.)

shepmaster commented 7 years ago

@rkruppe Absolutely! I wasn't really sure how much we could expect LLVM 4 to change as it goes towards stable / how much churn the Rust maintainers would want to put up with. I'll get those pulled out and submitted soon. I think I'm also starting to understand some of the silly error handling that the new LLVM does.

hanna-kruppe commented 7 years ago

@shepmaster Awesome, thanks in advance!

hanna-kruppe commented 7 years ago

BTW the PR reference for "llvm::AttrBuilder changed API" is wrong, it points at this issue instead of #37831

Kixunil commented 7 years ago

Does anyone have ETA? I'd like to know when I can use AVR. :)

shepmaster commented 7 years ago

I'd like to know when I can use AVR. :)

@Kixunil You should follow along at avr-rust/rust#22 for the AVR-specific issues. Upgrading to LLVM 4 will not magically yield AVR support.

Kixunil commented 7 years ago

@shepmaster Ok, thank you!

dylanmckay commented 7 years ago

I've created #38294 for the createBasicType API change.

dylanmckay commented 7 years ago

I've also created #38295 for another debug info DIBuilder API change.

dylanmckay commented 7 years ago

After putting all of those patches on top of master, rustllvm was successfully built. I got several undefined references, most likely due to my local LLVM setup.

dylanmckay commented 7 years ago

I think the next step after all those patches land is to allow LLVM 4.0 in the configure/rustbuild scripts and then actually update the submodule.

dylanmckay commented 7 years ago

We'll also need to fix that AAPCS bug first, I just noticed it

shepmaster commented 7 years ago

and then actually update the submodule.

Don't forget brson's comment:

Next time we do this upgrade we'll need emscripten to do it to.

I think that means that the Rust fork of LLVM is following the Emscripten fork of LLVM, thus Emscripten needs to upgrade to LLVM 4.0 / master before Rust can.

shepmaster commented 7 years ago

I've created #38294 for the createBasicType API change.

And I'm hoping to supersede it with #38317 😇

hanna-kruppe commented 7 years ago

I just ran into https://github.com/llvm-mirror/llvm/commit/58be60c4830ea33097d9c1d65e08029e4d5c6aad which changed the alignment from u64 to u32. Will think about a proper fix later, unless someone beats me to it (but #38295 should merge first to avoid further conflicts).

shepmaster commented 7 years ago

which changed the alignment from u64 to u32.

And it says

In futher patches we shall have alignment field added to DIVariable family

So, that will be "exciting".

DemiMarie commented 7 years ago

Is there much left blocking an upgrade?

japaric commented 7 years ago

@DemiMarie

This:

Next time we do this upgrade we'll need emscripten to do it to.

Has to be done too but I don't know if it's currently being worked on. @brson Any info on that ^?

brson commented 7 years ago

@japaric No nobody is actively working on an emscripten upgrade.

Since even when that is done it will probably take a lot of work to land an LLVM upgrade, there's probably more we can do to prepare. Once we decide we want to do it, somebody can prepare a patch to throw at the bots. It seems likely stuff will not work and it will be a slog to fix it all, which will give us time.

We don't need emcc to generate wasm stds, just a fairly small patch on our LLVM branch that will hopefully port without much difficulty.

So if somebody feels motivated, it's not too soon to start going after it.

As to the emscripten upgrade, we should at least let @kripken know that we're starting to think seriously about doing the upgrade, and need emscripten to upgrade LLVM too.

It seems like that could be a big effort. Another alternative is to use the LLVM wasm backend, but I still have not heard that it is generally usable yet.

cc @alexcrichton

kripken commented 7 years ago

I can look into the effort needed to update emscripten's fastcomp LLVM version, but I need to know a commit to aim for. Sounds like the goal here is 4.0, which hasn't released yet, so I guess I have to wait on that?

dylanmckay commented 7 years ago

So far as AVR is concerned, any commit within the last month will do. There will likely be a few more months until 4.0 is actually tagged (every ~6 months a new release is made). It is quite common for projects to track LLVM master, which I'm personally a fan of.

cuviper commented 7 years ago

But is it common to release snapshots from LLVM master? When LLVM itself only releases every 6 months, it seems incongruous for Rust to pick arbitrary snapshots in its 6-week release cycle.

hanna-kruppe commented 7 years ago

LLVM 4.0 will branch in a week and the final release will happen in late feburary (schedule says 21st for final tag, but this is flexible IIRC).

japaric commented 7 years ago

From the llvm-dev mailing list:

"This is just a quick reminder that branching for the upcoming release is scheduled for one week from now, 12 January 2017."

aidanhs commented 7 years ago

http://lists.llvm.org/pipermail/llvm-dev/2017-January/108949.html branch has happened

TimNN commented 7 years ago

An update on the "powidf2 / powisf2 calling convention changed" situation:

The regression has been fixed on llvm master (https://llvm.org/bugs/show_bug.cgi?id=30543#c13) however from what I can tell the fixes are not yet in the 4.0 branch, although the fix may be backported (https://llvm.org/bugs/show_bug.cgi?id=30543#c15).

dylanmckay commented 7 years ago

I've started working on updating emscripten

I've also created a new bug requesting that the powidf2 regression fix be merged into 4.0 here.

brson commented 7 years ago

@kripken says the emcc LLVM upgrade is ready to go. @shepmaster or @rkruppe are we prepared to begin the Rust LLVM upgrade?

I think what needs to happen is to submit the Rust PR and start throwing it against the bots. Once it looks like that is close to landing then we'll have to coordinate both project's upgrades to minimize the breakage window where rustc and emcc have different backends. I'm not sure how best to do that - we might just try to upgrade as simultaneously as possible and accept up to a few days of breakage, or we might ask emscripten to stage their build such that they can swap their incoming branch right after we land to master.

@kripken needs to know the exact LLVM commit we're moving to as well, and that will become clear once the PR starts working through the queue.

The stars are aligning. Let's make this happen.

Thanks @dylanmckay for doing the fastcomp upgrade.

hanna-kruppe commented 7 years ago

We are not fully compatible with LLVM 4.0. I have heard from at least one person (on IRC) that building against stock LLVM 4.0 (RC) does not work. I haven't personally inspected all failures, but there's at least two things still missing:

dylanmckay commented 7 years ago

The powidf2 fix has been merged into the 4.0 release (details).

I'm going to try and build Rust against stock LLVM 4.0 and figure out what needs modifying.

dylanmckay commented 7 years ago

On top of what @rkruppe mentioned:

dylanmckay commented 7 years ago

I've got a half-baked patch to use the new signature for createCompileUnit

patch.diff.txt

I'm not familiar enough with Rust internals to finish it off. I ran into a problem in fn compile_unit_metadata as it is given a SharedCrateContext but in order to use fn file_metadata we need a CrateContext. We need to call file_metadata in order to build an instance of DIFile*.

dylanmckay commented 7 years ago

I've opened #39528 for the createGlobalVariable change that @rkruppe mentioned.

I've also opened #39529 for the 32-bit alignment change.

I believe that createCompileUnit is the only thing we need to update aside from these two PRs in order to get the compiler to successfully be built with LLVM 4.0.

mattico commented 7 years ago

So to summarize where we are with LLVM 4 (correct me if I'm wrong):

  1. LLVM 4.0 will be released in about 1 week (2/21).

  2. There are no known issues with supporting LLVM 4.0, but there are probably issues that will be discovered once we try to use it in nightlies.

  3. Travis runs on in-tree LLVM + LLVM 3.7 (so PRs haven't actually been tested on LLVM 4.0 yet).

  4. We support LLVM back to ~3.6(?)~ 3.7 and will continue to support it until either: a. It's so old that it can be removed without bothering package maintainers b. We need at least version X in order to fix an important bug c. The API is so different from version Y that it's a huge pain to support

  5. Someone will open a PR to update the submodule shortly after release but it will take a month to land due to: a. Waiting for emscripten (incoming -> master) b. Bugs in upstream c. Missed LLVM API changes d. Making bors happy e. All of the above

alexcrichton commented 7 years ago

Thanks for the summary @mattico! The only correction I'd make is that in your point (4) you can s/3.6/3.7/, but otherwise it's accurate :)

If you'd like to start preparing a PR to update to LLVM 4.0 I don't think we need to block waiting for LLVM itself to release 4.0. Currently only one Travis builder runs on each PR but we could temporarily allow more builders running on an LLVM 4.0 PR to start weeding out bugs before it hits @bors.

mattico commented 7 years ago

I gave it a go but I don't think my git-fu (or patience) is strong enough to handle a big merge like this.

What I can do is try to run rustc with upstream LLVM and file/fix issues as I find them. There is at least one issue remaining as of #39747 (details to come).

japaric commented 7 years ago

Whoever ends up doing the LLVM upgrade, please also upgrade the compiler-rt submodule so that the sanitizer related LLVM passes don't diverge from what the sanitizer runtimes expect.

bstrie commented 7 years ago

If you'd like to start preparing a PR to update to LLVM 4.0 I don't think we need to block waiting for LLVM itself to release 4.0.

@alexcrichton In fact, if you upgrade before the release, they usually mention your project in the release notes. :)

dsvensson commented 7 years ago

@shepmaster @dylanmckay So whats the next issue(s) to subscribe to in the race-to-AVR? :)

dylanmckay commented 7 years ago

As @shepmaster says on reddit, we can integrate the latest Rust master into the avr-rust compiler. After that, we can spend some time getting libcore compiling for AVR.

Once that is working, we could then start looking into upstreaming the AVR target to Rust.

shepmaster commented 7 years ago

So whats the next issue(s) to subscribe to in the race-to-AVR?

AVR-specific issues can be raised / tracked in https://github.com/avr-rust/rust

For example, there are two that I'm aware of right now:

BatmanAoD commented 7 years ago

39782 has been closed, so that can be checked off in the top post.