Add help for target CPUs, features, relocation and code models. #34845

bitshifter · 2016-07-16T05:24:36Z

Fix for #30961. Requires PR rust-lang/llvm#45 to be accepted first, and the .gitmodules for llvm to be updated before this can be merged.

rust-highfive · 2016-07-16T05:24:51Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Aatch (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.

bitshifter · 2016-07-16T05:25:46Z

.gitmodules

@@ -1,6 +1,6 @@
 [submodule "src/llvm"]
 	path = src/llvm
-	url = https://github.com/rust-lang/llvm.git
+	url = https://github.com/bitshifter/llvm.git


Needs to be updated once llvm PR is accepted.

bitshifter · 2016-07-16T05:42:29Z

It looks like the travis build didn't get my llvm branch - https://travis-ci.org/rust-lang/rust/builds/145171124#L1179

bitshifter · 2016-07-19T13:29:17Z

I'm a bit stumped at this point why the travis build is failing due to my llvm change not being there. My last commit 87ed467 is pointing the llvm git submodule at rust-lang/llvm@a3c12a7 which looks right. Can anyone else see what's going wrong here?

alexcrichton · 2016-07-19T16:20:34Z

@bitshifter oh travis doesn't actually use the LLVM submodule b/c it takes too long to build, but it brings up an important point that we'd still want builds with external LLVM to continue to work. Perhaps the LLVM support here can be disabled if --llvm-root is specified?

bitshifter · 2016-07-19T23:41:26Z

@alexcrichton Ah I see, I'll look into it.

bitshifter · 2016-07-23T03:06:30Z

@alexcrichton is there any way of determining if --llvm-root is set at C++ compile time. I figure I could just disable this code path if the system llvm was being used. I was looking at the -D flags passed to the compiler and didn't see anything that differentiated between system llvm and Rust's llvm. Perhaps there's a define in the rust-llvm I can look for?

Alternatively I could add a temporary #define to MCSubtargetInfo.h in the rust-lllvm branch that I can check. Even if this patch makes it into LLVM, some kind of preprocessor check for it's existence will be necessary until it makes it into a release and Rust upgrades to that release.

Remove duplication code gen options and updated help to reflect changes.

If using system llvm don't try use modifications made in the fork.

bitshifter · 2016-07-24T14:38:36Z

Got past the compile error, now there is a segfault running the test. I noticed a similar segfault on another recent commit (https://travis-ci.org/rust-lang/rust/builds/146766819 - which I don't think I have) so perhaps it's unrelated to my changes?

edit: running ./configure --llvm-root=/usr/lib/llvm-3.7 && make tidy && make check-notidy -j8 locally all tests passed for me.

Aatch · 2016-07-24T23:45:47Z

I updated the title, since "fixes this other issue" isn't very informative.

alexcrichton · 2016-07-25T04:39:13Z

@bitshifter Right now I don't think there's a define for that but you can check for CFG_LLVM_ROOT in the makefiles and the build script for these files should also have a good entry point for adding a define.

bitshifter · 2016-07-25T07:20:09Z

@alexcrichton thanks, I've added -DLLVM_RUSTLLVM based on an absence of CFG_LLVM_ROOT in ce50bed. I guess I should add something similar when using --enable-rustbuild?

alexcrichton · 2016-07-25T16:16:44Z

@bitshifter yeah you'll want to tweak this build script. The LLVM_CONFIG variable is set here and you can use similar logic as here to set some other environment variable to indicate whether it's a custom or rust llvm

alexcrichton · 2016-07-29T16:08:51Z

@bors: r+ fc210a8

Thanks!

bors · 2016-07-29T16:35:14Z

⌛ Testing commit fc210a8 with merge 58bdec2...

brson · 2016-07-30T03:04:45Z

@bors retry force clean

bors · 2016-07-30T03:04:51Z

⌛ Testing commit fc210a8 with merge 8d2f6ea...

bors · 2016-07-30T03:29:42Z

💔 Test failed - auto-linux-64-cargotest

bors · 2016-08-01T15:58:23Z

☔ The latest upstream changes (presumably #34743) made this pull request unmergeable. Please resolve the merge conflicts.

bitshifter · 2016-08-01T20:34:00Z

I'm on vacation at the moment, I'll take a look later in the week.

bitshifter · 2016-08-06T05:06:50Z

@alexcrichton I've had a little look into why the @bors homu build failed. On the homu build there was no CMake build of LLVM in the logs. So I assume that it is using an old version. I think part of the CMake build copies headers from src/llvm/include to build/x86_64-unknown-linux-gnu/llvm/include - probably a make install step. I think this is not picking up that LLVM has changed and needs to bu rebuilt. I've had this happen a bit on my local machine and have just been cleaning to fix it but I guess a better solution is required. Are you familiar with this part of the build at all? I'm not sure how to force LLVM to pick up that it needs to rebuild (perhaps when the git revision changes).

I'm also addressing the new conflicts and will submit a new PR for rust-lang/llvm shortly.

Edit: It looks like I need to edit src/rustllvm/llvm-auto-clean-trigger to fix the bors problem.

alexcrichton · 2016-08-07T00:16:05Z

src/rustllvm/llvm-auto-clean-trigger

@@ -1,4 +1,4 @@
 # If this file is modified, then llvm will be forcibly cleaned and then rebuilt.
 # The actual contents of this file do not matter, but to trigger a change on the
 # build bots then the contents should be changed so git updates the mtime.
-2016-07-25b
+2016-07-25c


Oh you can actually just change this to today's date, the b was just because it was modified twice in the same day

alexcrichton · 2016-08-07T00:16:34Z

Looks and sounds good to me, thanks for the investigation!

alexcrichton · 2016-08-08T16:34:43Z

src/bootstrap/compile.rs

@@ -198,6 +198,10 @@ pub fn rustc<'a>(build: &'a Build, target: &str, compiler: &Compiler<'a>) {
    if !build.unstable_features {
        cargo.env("CFG_DISABLE_UNSTABLE_FEATURES", "1");
    }
+    // Flag that rust llvm is in use
+    if build.config.target_config.get(target).is_none() {


I think that this needs to check the llvm-config field in the returned table, as this is currently checking if there is any configuration at all for the target.

The strange thing is that there seems to be no target returned at this point if --llvm-root has not been specified. If --llvm-root is specified then there is a target and it has an llvm_config set. I could make this code check for no target or a target with no llvm_config, but it seems like maybe there being no target is unexpected?

How do you usually debug this? For now I've just been printf debugging with println! or panic! calls.

The logic for parsing ./configure's --llvm-root option lives here so it only sets it for one target but it should be there?

Looking a bit closer, there are a bunch of code paths that lazily insert a default target into the target_config HashMap but none are being hit it this particular case, e.g. the code you linked to:

let target = self.target_config.entry(self.build.clone()) .or_insert(Target::default());

At the point I'm performing the check, nothing has done this although it could be created if other config.mk variables that lazily insert a target are in use.

In any case as you pointed out, the current code won't work correctly if target does exist, I'll look into a fix tonight.

alexcrichton · 2016-08-08T16:35:16Z

@bitshifter thanks! Could you also squash the commits?

bitshifter · 2016-08-08T22:46:08Z

@alexcrichton Sure! How do I squash the commits? :)

Make a new branch, merge into it and make a new PR?

alexcrichton · 2016-08-09T03:58:57Z

Oh sure, no problem! You shouldn't have to make a new PR, I think these instructions should suffice?

bitshifter · 2016-08-09T04:07:30Z

Oh, I thought it wasn't a good idea to rebase things that have been pushed already?

From linked article:

Note: do not squash commits that you’ve already shared with others. You’re changing history and it will cause trouble for others.

alexcrichton · 2016-08-09T05:30:27Z

Yeah for us that's fine as we define "sharing with others" as published to the master branch of rust-lang/rust. Your own local branch is only used as the PR here, so it's not necessarily "shared" yet in the sense of people relying on the commit SHAs

@bitshifter

Point llvm @bitshifter branch until PR accepted Use today's date for LLVM auto clean trigger Update LLVM submodule to point at rust-lang fork. Handle case when target is set

bitshifter · 2016-08-10T12:49:55Z

I think I've fixed up the target llvm-config check.

I squashed the last 5 commits, since I last merged with master.

I'm not sure if I should try and squash the other commits, since they're intermingled with merges.

alexcrichton · 2016-08-10T22:39:04Z

@bors: r+ 05045da

Ah it's ok, no worries!

bors · 2016-08-11T04:42:49Z

⌛ Testing commit 05045da with merge 1222f5d...

Add help for target CPUs, features, relocation and code models. Fix for #30961. Requires PR rust-lang/llvm#45 to be accepted first, and the .gitmodules for llvm to be updated before this can be merged.

bors · 2016-08-11T08:58:46Z

bitshifter added 2 commits July 11, 2016 00:22

Add help for target CPUs, features, relocation and code models.

e1efa32

Merge branch 'master' into issue-30961

5c4d621

rust-highfive assigned Aatch Jul 16, 2016

bitshifter reviewed Jul 16, 2016
View reviewed changes

bitshifter changed the title ~~Issue 30961~~ Fix for issue 30961 Jul 16, 2016

bitshifter mentioned this pull request Jul 19, 2016

Add accessors for MCSubtargetInfo CPU and Feature tables rust-lang/llvm#45

Merged

bitshifter added 3 commits July 19, 2016 20:57

Merge branch 'master' into issue-30961

79358aa

Update src/llvm to use the rust-lang version again.

09cc3e2

Hopefully this is the right llvm commit this time!

87ed467

bitshifter added 2 commits July 24, 2016 18:44

Tidy ups for code gen options help

6aee1e2

Remove duplication code gen options and updated help to reflect changes.

Pass -DLLVM_RUSTLLVM to compile against rust llvm fork.

ce50bed

If using system llvm don't try use modifications made in the fork.

Aatch changed the title ~~Fix for issue 30961~~ Add help for target CPUs, features, relocation and code models. Jul 24, 2016

Make rust build pass LLVM_RUSTLLVM to C++ compiler

fc210a8

bitshifter mentioned this pull request Aug 6, 2016

Add accessors for MCSubtargetInfo CPU and Feature tables rust-lang/llvm#46

Merged

Merge branch 'master' into issue-30961

cbb88fa

alexcrichton reviewed Aug 7, 2016
View reviewed changes

alexcrichton reviewed Aug 8, 2016
View reviewed changes

alexcrichton assigned alexcrichton and unassigned Aatch Aug 8, 2016

Improved checking of target's llvm_config

05045da

Point llvm @bitshifter branch until PR accepted Use today's date for LLVM auto clean trigger Update LLVM submodule to point at rust-lang fork. Handle case when target is set

bitshifter force-pushed the issue-30961 branch from 704eb0c to 05045da Compare August 10, 2016 12:48

bors merged commit 05045da into rust-lang:master Aug 11, 2016

bors mentioned this pull request Aug 11, 2016

Take commandline arguments into account for incr. comp. #35340

Merged

bitshifter deleted the issue-30961 branch August 11, 2016 09:08

bluss added the relnotes Marks issues that should be documented in the release notes of the next release. label Sep 21, 2016

Add help for target CPUs, features, relocation and code models. #34845

Add help for target CPUs, features, relocation and code models. #34845

Conversation

bitshifter commented Jul 16, 2016

rust-highfive commented Jul 16, 2016

bitshifter Jul 16, 2016

Choose a reason for hiding this comment

bitshifter commented Jul 16, 2016 • edited Loading

bitshifter commented Jul 19, 2016

alexcrichton commented Jul 19, 2016

bitshifter commented Jul 19, 2016

bitshifter commented Jul 23, 2016 • edited Loading

bitshifter commented Jul 24, 2016 • edited Loading

Aatch commented Jul 24, 2016

alexcrichton commented Jul 25, 2016

bitshifter commented Jul 25, 2016

alexcrichton commented Jul 25, 2016

alexcrichton commented Jul 29, 2016

bors commented Jul 29, 2016

brson commented Jul 30, 2016

bors commented Jul 30, 2016

bors commented Jul 30, 2016

bors commented Aug 1, 2016

bitshifter commented Aug 1, 2016

bitshifter commented Aug 6, 2016 • edited Loading

alexcrichton Aug 7, 2016

Choose a reason for hiding this comment

alexcrichton commented Aug 7, 2016

alexcrichton Aug 8, 2016

Choose a reason for hiding this comment

bitshifter Aug 9, 2016

Choose a reason for hiding this comment

alexcrichton Aug 9, 2016

Choose a reason for hiding this comment

bitshifter Aug 9, 2016 • edited Loading

Choose a reason for hiding this comment

alexcrichton commented Aug 8, 2016

bitshifter commented Aug 8, 2016

alexcrichton commented Aug 9, 2016

bitshifter commented Aug 9, 2016

alexcrichton commented Aug 9, 2016

bitshifter commented Aug 10, 2016 • edited Loading

alexcrichton commented Aug 10, 2016

bors commented Aug 11, 2016

bors commented Aug 11, 2016

bitshifter commented Jul 16, 2016 •

edited

Loading

bitshifter commented Jul 23, 2016 •

edited

Loading

bitshifter commented Jul 24, 2016 •

edited

Loading

bitshifter commented Aug 6, 2016 •

edited

Loading

bitshifter Aug 9, 2016 •

edited

Loading

bitshifter commented Aug 10, 2016 •

edited

Loading