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

Migrate Rust to use released LLVM versions #42389

Closed
eholk opened this issue Jun 2, 2017 · 6 comments
Closed

Migrate Rust to use released LLVM versions #42389

eholk opened this issue Jun 2, 2017 · 6 comments
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC

Comments

@eholk
Copy link
Contributor

eholk commented Jun 2, 2017

Right now Rust maintains its own "temporary" fork of LLVM: https://github.com/rust-lang/llvm

The fork seems to basically be LLVM 4.0 with a few changes. A list of the current changes on top of LLVM upstream can be seen here. Plus, Rust's LLVM fork includes the fastcomp backend, meaning Rust and fastcomp need to upgrade in lock-step.

Rust and LLVM seem to be mature enough now that Rust should be able to track released version numbers of LLVM. The current state of affairs seems to impose a heavy maintenance burden, making it hard to integrate upstream improvements in LLVM to Rust.

@petrochenkov
Copy link
Contributor

cc @TimNN

@hanna-kruppe
Copy link
Contributor

I'm not necessarily opposed to this, but I'd like more clarity about the impact of this. I'm concerned about three aspects of our current setup:

  1. There's quite a few LLVM bugs (especially on non-x86 platforms), which have fixes upstream that can get backported into our fork. How are these bugs going to be addressed if we're committing to using only released upstream versions? LLVM releases every six months, which is quite a long time to wait by Rust standards (especially for serious regressions where patches may get backported to beta), and historically every release has had its own new set of serious bugs.
  2. What's the story for emscripten/wasm? If we stick to upstream LLVM, there's an in-tree wasm backend but I've heard it's nowhere near ready yet. Is regressing wasm support okay? Or would we wait with this change until there's a released LLVM version with a sufficiently advanced wasm backend?
  3. What about patches that upstream doesn't want? Among the current ones, Add accessors for MCSubtargetInfo CPU and Feature tables llvm#45 and Run GVN again after InstCombine. llvm#49 seem likely to be somewhat controversial.

@eholk
Copy link
Contributor Author

eholk commented Jun 3, 2017

@rkruppe Thanks for your concerns, these are the kinds of things I was hoping to discover by opening the issue.

The more I think about this, the more I think what I actually want is for Rust to track LLVM master more closely with fewer of its own patches. Perhaps this should come with a preference for submitting fixes to LLVM upstream rather than the Rust LLVM fork. Maybe this preference is already present in general.

The main reason I'm interested in this now is in support of @tlively and @vadimcn's efforts to migrate to the LLVM Wasm backend (#38804). The upstream Wasm backend has seen a lot of improvements since Rust last updated LLVM. To take advantage of these in Rust, it seems we either have the option of cherry-picking (or some other git fu) just the WebAssembly changes, thereby causing Rust's fork to drift further from LLVM, or wait for emscripten to upgrade LLVM so Rust can upgrade too. Neither of these options seem ideal to me.

For the time being, it would be nice if both the Emscripten/Fastcomp target and the in-tree Wasm target could coexist in Rust. From what I hear, Fastcomp generates better Wasm code for now, and it also gives support for asm.js.

For the patches that upstream doesn't want, my question would be whether those patches add enough value to Rust to make it worth the cost of maintaining an LLVM fork. Given that they seem to fix real Rust bugs, I would guess the answer is probably so.

@hanna-kruppe
Copy link
Contributor

The more I think about this, the more I think what I actually want is for Rust to track LLVM master more closely with fewer of its own patches.

Then my first concern becomes modified to: LLVM TOT is somewhat less stable/reliable than released versions (simply because releases get more testing and get selected patches merged after branching) and introduces new bugs almost as often as it fixes existing bugs. I'll defer judgement to people who actually track down and fix these bugs (e.g., @arielb1), but picking random trunk revisions seems like it may increase the difficulty of providing a reasonably a codegen-bug-free rustc, especially on less popular targets like (non-Apple-)ARM.

For the time being, it would be nice if both the Emscripten/Fastcomp target and the in-tree Wasm target could coexist in Rust. From what I hear, Fastcomp generates better Wasm code for now, and it also gives support for asm.js.

To clarify, do you mean holding off tracking TOT and experimenting with the wasm backend as of LLVM 4.0, or do you propose tracking TOT more closely while still using fastcomp? The latter seems to require updating fastcomp and merging it into our fork every time we update our fork, which is probably quite a bit of extra work if we start updating more frequently.

@arielb1
Copy link
Contributor

arielb1 commented Jun 3, 2017

I'll defer judgement to people who actually track down and fix these bugs (e.g., @arielb1), but picking random trunk revisions seems like it may increase the difficulty of providing a reasonably a codegen-bug-free rustc, especially on less popular targets like (non-Apple-)ARM.

I'm not sure how buggy are random commits in TOT LLVM, but even for stable LLVM we definitely want to at least pass them through the trains and backport all necessary fixes.

@sanxiyn sanxiyn added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Jun 7, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC label Jul 27, 2017
@alexcrichton
Copy link
Member

This is as effectively as done as it'll ever get right now. We should be compatible with any recent-ish LLVM. I don't think we'll ever use vanilla LLVM by default, but there's not really much impetus to do so as long as we're compatible with released versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC
Projects
None yet
Development

No branches or pull requests

7 participants