LLVM upgrade #34743
LLVM upgrade #34743
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Ah, forgot to mention that I am using a LLVM checkout of |
| @@ -267,7 +267,7 @@ LLVMRustAddLibraryInfo(LLVMPassManagerRef PMB, | |||
| // similar code in clang's BackendUtil.cpp file. | |||
| extern "C" void | |||
| LLVMRustRunFunctionPassManager(LLVMPassManagerRef PM, LLVMModuleRef M) { | |||
| FunctionPassManager *P = unwrap<FunctionPassManager>(PM); | |||
| llvm::legacy::FunctionPassManager *P = unwrap<llvm::legacy::FunctionPassManager>(PM); | |||
There was a problem hiding this comment.
llvm::legacy::FunctionPassManager
Tangential comment to this pull request: Does anyone know what this got replaced with since it's presumably legacy now?
There was a problem hiding this comment.
LLVM pass manager infrastructure is currently getting rewritten to be more flexible. See https://github.com/llvm-mirror/llvm/blob/d1aa4ea020d932ffc6a3c3e4d1bbab659391c725/tools/opt/NewPMDriver.cpp#L50 for a usage example for the new API. That said, the rewrite isn't complete yet.
There was a problem hiding this comment.
Looks like clang is also still using this, so we probably don't need to update just yet until they do at least.
| @@ -188,7 +188,7 @@ LLVMRustCreateTargetMachine(const char *triple, | |||
| } | |||
|
|
|||
| TargetOptions Options; | |||
| Options.PositionIndependentExecutable = PositionIndependentExecutable; | |||
| //Options.PositionIndependentExecutable = PositionIndependentExecutable; | |||
There was a problem hiding this comment.
I'm pretty sure this is a hack that only makes sense in my fork (there's no PIE for microcontrollers :-)).
Even if we wanted to remove this (doubtful), it should be removed and not commented-out. I believe that LLVM applies this flag at a more granular level now, so looking downward for another flag to apply it to seems reasonable.
There was a problem hiding this comment.
Right, this is one of the things mentioned above. I left it in, so I remember to fix that.
There was a problem hiding this comment.
This was deleted here which appears to be replaced by this which is a new setPIELevel function on the LLVM module itself.
The LLVM module isn't available at this time so we'll just have to arrange for it to be set later. Perhaps something like this though:
- Leave this block, but gated by
#if LLVM_VERSION_MINOR <= 8for compatibility with older versions. - Add
LLVMRustSetModulePIELevelas a function which is a noop on pre-LLVM-3.9 and afterwards calls thesetPIELevelfunction on the LLVM module.
There was a problem hiding this comment.
Did you mean we have a LLVMRustSetModulePIELevel with the #ifdef for calling the setPIELevel method?
If so, at what point do we call it?
There was a problem hiding this comment.
If so, at what point do we call it?
I'd believe you want to call it every time an LLVM module is created.
|
I'm no core contributor, but I don't see the value in commit X introducing less-than-great variable names and then commit X+1 improving them. I'd just squash those two together. |
|
@shepmaster yes, I should have squashed them beforehand. I can still do this. |
| let work_dir = &cx.sess().working_dir; | ||
| let compile_unit_name = match cx.sess().local_crate_source_file { | ||
| None => fallback_path(cx), | ||
| pub fn compile_unit_metadata(scc: &::context::SharedCrateContext, debug_context: &CrateDebugContext, sess: &::session::Session) -> DIDescriptor { |
There was a problem hiding this comment.
Right now our line limit is 100 characters so I think this will trigger make tidy, could you also add imports instead of using absolute paths here?
|
A failure on issue-28950 looks like it may be specific to your system or something like that, I doubt that the LLVM changes would cause it to fail. Are you sure that it succeeds before this PR on your local computer? |
|
@alexcrichton Indeed, with a fresh checkout of rust-lang/rust I see the same error on issue-28950 |
|
This is supposed to fix #34119 ? |
|
@alexbool If it is fixed by something in LLVM upstream, then yes |
|
cc @kripken Here's Rust's LLVM upgrade. |
|
@brson: cool, emscripten's merges of that same LLVM commit are in https://github.com/kripken/emscripten-fastcomp/tree/next-merge and https://github.com/kripken/emscripten-fastcomp-clang/tree/next-merge . The tracking issue is emscripten-core/emscripten#4430. Looks like only some SIMD issues block it, which probably wouldn't be a problem for rust, so it could be usable for testing already. |
|
I'd strongly encourage having links to the original LLVM issues / commits that are forcing these changes. These links should be in the commit message that modifies the relevant aspect of the code. That should make it much easier for people in the future to track down the why. |
I say this having been one of those people to track down such information in the past. |
|
@shepmaster to clarify, you mean the changes to LLVM bindings and why they're changing? (e.g. this) |
Yeah; sorry to be unclear. Having the commit message be something like
would make that commit 💯 in my book. Otherwise the commit is like "moving some functions around!" Again, I stress that I hold no power to enforce these suggestions; I just would have found commits like this to be useful when I was digging into the existing code to make these kind of changes. 🌴 |
|
Sounds like a good plan to me! |
|
I will rebase the commits and change accordingly (force-push incoming!) @shepmaster Thanks for mentioning this, it's indeed a good idea, especially in the long run |
|
@alexcrichton @eddyb Can this PR also be placed under the "Launch MIR into Orbit" milestone, since it's blocking #34119? |
|
@solson sure! |
|
I just force-pushed with the new commits
It seems something is not compiled correctly with |
|
@tmiasko That's interesting. Do you have a backtrace? We couldn't get anything, reproduction seemed to need the concurrent builds, or a specific builder, to trigger. |
|
@tmiasko did some digging and discovered that https://reviews.llvm.org/D22858 may be relevant (stack trace gathered). I've updated our LLVM submodule with this commit (rebased on the llvm/release_39 branch) and now it's rust-lang/llvm@d1cc489. @badboy wanna try updating to that and give it a go? Builds in dev are running, but we can do in parallel hopefully :) |
@tmiasko did some digging and discovered that https://reviews.llvm.org/D22858 may be relevant.
|
Done! Upgraded to d1cc489 |
|
@bors r+ |
|
📌 Commit 5d1d247 has been approved by |
LLVM upgrade As discussed in https://internals.rust-lang.org/t/need-help-with-emscripten-port/3154/46 I'm trying to update the used LLVM checkout in Rust. I basically took @shepmaster's code and applied it on top (though I did the commits manually, the [original commits have better descriptions](master...avr-rust:avr-support). With these changes I was able to build rustc. `make check` throws one last error on `run-pass/issue-28950.rs`. Output: https://gist.github.com/badboy/bcdd3bbde260860b6159aa49070a9052 I took the metadata changes as is and they seem to work, though it now uses the module in another step. I'm not sure if this is the best and correct way. Things to do: * [x] ~~Make `run-pass/issue-28950.rs` pass~~ unrelated * [x] Find out how the `PositionIndependentExecutable` setting is now used * [x] Is the `llvm::legacy` still the right way to do these things? cc @brson @alexcrichton
|
Wuhu! 🎉 |
|
Outstanding work everyone! |
|
Epic! |
|
Hurrah! Now it's time to rebase the AVR and Emscripten forks and really give it a workout ;-) |
|
@shepmaster: That's the next step and I'll try to work on it this week |
|
Heroes, every one of you! On with MIR! |
|
Looks like this missed the LLVM version check in configure, not sure why it didn't break bots: |
|
I have a PR out for that - #35594. No idea about the bots. |
The minimum got bumped in the LLVM upgrade of rust-lang#34743.
…omatsakis Update minimum CMake version in README The minimum got bumped in the LLVM upgrade of rust-lang#34743.
Soon the LLVM upgrade (rust-lang#34743) will require an updated CMake installation, and the easiest way to do this was to upgrade the Ubuntu version of the bots to 16.04. This in turn brings in a new MIPS compiler on the linux-cross builder, which is now from the "official" ubuntu repositories. Unfortunately these new compilers don't support compiling with the `-msoft-float` flag like we're currently passing, causing compiles to fail. This commit removes these flags as it's not clear *why* they're being passed, as the mipsel targets also don't have it. At least if it's not supported by a debian default compiler, perhaps it's not too relevant to support?
As discussed in https://internals.rust-lang.org/t/need-help-with-emscripten-port/3154/46 I'm trying to update the used LLVM checkout in Rust.
I basically took @shepmaster's code and applied it on top (though I did the commits manually, the original commits have better descriptions.
With these changes I was able to build rustc.
make checkthrows one last error onrun-pass/issue-28950.rs. Output: https://gist.github.com/badboy/bcdd3bbde260860b6159aa49070a9052I took the metadata changes as is and they seem to work, though it now uses the module in another step. I'm not sure if this is the best and correct way.
Things to do:
Makeunrelatedrun-pass/issue-28950.rspassPositionIndependentExecutablesetting is now usedllvm::legacystill the right way to do these things?cc @brson @alexcrichton