Skip to content

-C suggestions to use llc for details is annoying #30961

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

Closed
brson opened this issue Jan 16, 2016 · 12 comments
Closed

-C suggestions to use llc for details is annoying #30961

brson opened this issue Jan 16, 2016 · 12 comments
Labels
E-help-wanted Call for participation: Help is requested to fix this issue.

Comments

@brson
Copy link
Contributor

brson commented Jan 16, 2016

rustc -C help says

    -C        target-cpu=val -- select target processor (llc -mcpu=help for details)
    -C    target-feature=val -- target specific attributes (llc -mattr=help for details)
    -C  relocation-model=val -- choose the relocation model to use (llc -relocation-model for details)
    -C        code-model=val -- choose the code model to use (llc -code-model for details)

Everytime I need to find the right strings to pass these flags I groan - I have to go find some copy of llc - Rust doesn't provide it - and hope it corresponds reasonably to my rustc. This is somewhere between annoying and useless to end users. rustc should have a way itself to emit this information.

Probably should have --print target-cpus etc arguments.

@brson brson added A-tools E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Jan 16, 2016
@brson
Copy link
Contributor Author

brson commented Jan 16, 2016

Add --print options for each of these that query llvm for the information and print to screen, then change the help strings to indicate the new way to get that info.

@bluss
Copy link
Member

bluss commented Jan 16, 2016

rustc -C target-cpu=help filename.rs does produce a long list. Several times. So it's buggy and not helpful, but it... it deserves mention as a bug? The other flags don't know about help.

@muhang
Copy link

muhang commented Jan 17, 2016

@brson Could I pick this up? I'd really like to start contributing, and this seems like a great crash course in Rust's internals.

@nagisa
Copy link
Member

nagisa commented Jan 19, 2016

rustc -C target-cpu=help filename.rs does produce a long list. Several times. So it's buggy and not helpful, but it... it deserves mention as a bug?

llc -mcpu=help produces the same CPU list trice as well, so it is a bug in LLVM.

Notably, the man page says:

       target-cpu=help
              Selects a target processor.  If the value is 'help', then a list of available CPUs is printed.

so I think the -C *=help should be reused and we should make sure it works without a filename.

@brson
Copy link
Contributor Author

brson commented Jan 20, 2016

so I think the -C *=help should be reused and we should make sure it works without a filename.

Not that I disagree with this (I'm not sure), but I don't think we need to stick to LLVM's interface for this. We do have a facility for printing information. This would create a second way for the compiler to print information.

@brson
Copy link
Contributor Author

brson commented Jan 20, 2016

@muhang Yes, please do! It'll probably take some digging through LLVM to figure out where the information is, and then you'll likely need to C via RustWrapper.cpp.

@muhang
Copy link

muhang commented Jan 20, 2016

Cool, sounds good! I'll reach out to the community via IRC if I run into any serious roadblocks.

@brson brson added E-help-wanted Call for participation: Help is requested to fix this issue. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Jun 27, 2016
@brson
Copy link
Contributor Author

brson commented Jun 27, 2016

There's still disagreement here about the interface. We have two proposals:

-C target-cpu=help
-C target-feature=help
-C relocation-model=help
-C code-model=help
--print target-cpus
--print target-features
--print relocation-models
--print code-models

The former is like llc, but feels weird to me because it's unlike other -C flags and it overloads these flags to do something different than their normal function. If we went with the latter we'd probably want to explicitly turn -C target-cpu=help into an error to avoid passing it to LLVM. f? @rust-lang/compiler

In either case we can still make progress on coding up the feature. The interface is a small detail. If anybody picks this up I'd say start assuming we'll do the former, llc-like interface.

@bitshifter
Copy link
Contributor

I have a fix for this but it requires a modification to LLVM, which complicates things PR wise since the LLVM change would need to be accepted before the Rust change as I'm pointing Rust at my LLVM branch. What would be the best way of doing this? Just create a PR for both?

The reason I had to modify LLVM is the information used to print the target CPU and feature help are private members on MCSubtargetInfo. LLVM currently prints these as a side effect of calling MCSubtargetInfo::getFeatureBits which is I think why they're printed multiple times. I added accessors for these members so I can print them on demand from rustc.

This is my first Rust change, please let me know the best way to proceed.

FYI my changes so far are bitshifter/llvm@31d7a40 and bitshifter@e1efa32.

@brson
Copy link
Contributor Author

brson commented Jul 15, 2016

@bitshifter Oh, awesome!

Yes, you got it right. First submit the LLVM patch to our fork. It looks like the branch you want to submit to is rust-llvm-2016-03-13. You can go ahead and submit the Rust PR that depends on it at the same time, and link to the LLVM PR, mentioning the dependency.

This sounds like the kind of LLVM change that might get pushback to upstream to mainline LLVM, so I'd be prepared for that, but start with the PRs to Rust for the review.

cc @alexcrichton @bitshifter has a fix for this issue that requires an LLVM patch. fyi.

@bitshifter
Copy link
Contributor

@brson thanks! I've created a couple of PRs per your suggestion.

Regarding mainline LLVM, I could create a bug for llc printing duplicate information. I couldn't see anything similar in their bug tracker, potentially a similar fix could be applied upstream (expose details and print explicitly rather than implicitly).

bors added a commit that referenced this issue Aug 11, 2016
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.
@brson
Copy link
Contributor Author

brson commented Sep 20, 2016

Fixed! Thanks @bitshifter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-help-wanted Call for participation: Help is requested to fix this issue.
Projects
None yet
Development

No branches or pull requests

6 participants