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

Compute target_feature from LLVM #31709

Merged
merged 10 commits into from
Apr 20, 2016
Merged

Conversation

ranma42
Copy link
Contributor

@ranma42 ranma42 commented Feb 16, 2016

This is a work-in-progress fix for #31662.

The logic that computes the target features from the command line has been replaced with queries to the TargetMachine.

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@ranma42
Copy link
Contributor Author

ranma42 commented Feb 16, 2016

I tried this and it has the desired result for the example of #31662, but I think it might need more work.
In particular, I am not confident about several issues:

  • is this an acceptable approach?
  • is a string the desired output type?
  • is it ok to work on a TargetMachine instance?
  • would it be ok to initialise LLVM earlier than in trans? (would it be ok to do it from build_configuration?)

I do not think this is ready for merge, but I would love feedback and directions.

has_avx, "avx";
has_avx2, "avx2";
has_neon, "neon";
has_vfp, "vfp";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the purposes of a macro like this was to ensure that we had control over the names exported. We don't want to blanket export all features that LLVM itself exposes, and we also don't want to tie ourselves to exactly the names LLVM has. In other words this is here to ensure we can provide a stable interface over the possibly unstable interface of LLVM.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filtering and renaming these features to a "rust" convention instead of the LLVM one can be done very easily with a map. The filtering makes sense to me, as rust might want to expose only a subset of the LLVM features through cfg!.
Renaming these features is just as easy, but I would be surprised if it happened only here and not in every place where a feature is named (for example when they are passed through the -C target-feature flag). That would result in an inconsistent naming, as the same feature would have a different name in the command line flags and in the code.
If you confirm that renaming is desired here and not elsewhere, I will implement it, otherwise I believe that it might belong to another PR (under the assumption that if LLVM and rust feature naming was to diverge in future, we would decide how to take care of that and possibly perform a consistent renaming everywhere).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of this macro was that for the function/name pair the cfg directive name would be defined if the function returned true. This is the only place that does that.

Basically, we need to filter what's coming out of LLVM with an explicit whitelist of what cfg directives are defined. We don't need to map names yet (as we just use LLVM names), but we may support that one day.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a whitelist of known LLVM features which is used to filter which ones are exposed through cfg!.

@nikomatsakis
Copy link
Contributor

r? @alexcrichton

@ranma42
Copy link
Contributor Author

ranma42 commented Feb 17, 2016

I moved the initialisation of LLVM to librustc. In order to avoid a circular dependency between crates, I had to move configure_llvm. This might not be correct, as it was a public function. Currently I made it private, but if it is meant to be part of the public interface of librustc_trans it might need to be reexported or moved to a crate which can be used/exported by librustc and librustc_trans.

@@ -720,6 +750,27 @@ pub fn build_configuration(sess: &Session) -> ast::CrateConfig {
}
let mut v = user_cfg.into_iter().collect::<Vec<_>>();
v.extend_from_slice(&default_cfg[..]);

// Before we touch LLVM, make sure that multithreading is enabled.
unsafe {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be extracted into its own function instead of being copy/pasted into an otherwise irrelevant function. This should also probably be called from a more principled location such as the driver itself

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean in librustc_driver? AFAICT it is not possible to invoke functions in librustc_driver from librustc because the crates would have a circular dependency. Should I also try to move build_configuration to another crate? (is it possible/does it make sense?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it'd just be an implicit contract that the driver must initialize LLVM before calling this function.

@ranma42
Copy link
Contributor Author

ranma42 commented Mar 2, 2016

I tried to implement all of the suggested changes. It might be possible that PNaCl needs special care if it has features we want to expose, because it does not look like a normal LLVM target. So far this should not be an issue, as all of the features that are whitelisted are from other targets.

I added the init_llvm function in librustc_trans::back::write and it should be called before any use of LLVM. Is src/librustc/README.md the most appropriate place to warn people that init_llvm should be called?

@alexcrichton
Copy link
Member

Unfortunately I think the makefiles will also need to change to account for the change here, rustbuild is only an alternate build system currently. It's ok to not worry much about pnacl for now though.

And yeah, init_llvm looks good to me. It's probably fine to leave it sprinkled as-is, it's not clear to me anyway about who's supposed to call it anyway.

@ranma42
Copy link
Contributor Author

ranma42 commented Mar 3, 2016

I added the changes to Makefiles (based on the existing build templates from mk/main.mk and mk/llvm.mk) and fixed the tests that were using LLVM to invoke init_llvm as part of the session construction.

@@ -43,6 +43,10 @@ $$(RT_OUTPUT_DIR_$(1))/$$(call CFG_STATIC_LIB_NAME_$(1),rustllvm): \
@$$(call E, link: $$@)
$$(Q)$$(call CFG_CREATE_ARCHIVE_$(1),$$@) $$^

EXTRA_RUSTLLVM_CXXFLAGS_$(1) = $$(shell echo $$(filter $$(LLVM_ALL_COMPONENTS_$(1)),\
$$(LLVM_SUBTARGET_COMPONENTS)) | tr 'a-z' 'A-Z' |\
sed -e 's/^ //;s/\([^ ]*\)/\-DLLVM_SUBTARGET_\1/g')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this could just be:

EXTRA_RUSTLLVM_CXXFLAGS_$(1) = $$(LLVM_ALL_COMPONENTS_$(1):%=-DLLVM_COMPONENT_%)

That way we could just ferry all components down to the C++ code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, you just made me realise that I should not be overwriting EXTRA_RUSTLLVM_CXXFLAGS_$(1), I meant to use the RUSTLLVM_TARGETS_$(1) variable!
Anyway, yes, that would work and looks much simpler, but it will result in mixed-case defines. Is it ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... it does not work, because the - character is not allowed in a define, but it is used in several components. Something like this seems to work:

RUSTLLVM_COMPONENTS_$(1) = $$(shell echo $$(LLVM_ALL_COMPONENTS_$(1)) |\
        tr 'a-z-' 'A-Z_'| sed -e 's/^ //;s/\([^ ]*\)/\-DLLVM_COMPONENT_\1/g')

and still avoids the distinction between target and non-target components.

@ranma42
Copy link
Contributor Author

ranma42 commented Mar 4, 2016

@alexcrichton I tried to implement both your suggestions and the overall change now looks cleaner :)
I kept the new commits separate, but it might be a good idea to squash them before merging so that the changes that have been reverted (those to the tests, for example) are cleaned up.

for component in components {
let mut flag = String::from("-DLLVM_COMPONENT_");
flag.push_str(&component.to_uppercase());
cfg.flag(&flag);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zomg this looks so much nicer than make

@alexcrichton
Copy link
Member

Awesome, looking great to me, thanks @ranma42!

Could you add some tests for this as well? I think the case in #31662 may be a good one to add?

@alexcrichton
Copy link
Member

Also yeah I'd be fine squashing commits before landing as well.

@alexcrichton
Copy link
Member

Also, out of curiosity, did you get a response from asking LLVM about these constants?

@ranma42
Copy link
Contributor Author

ranma42 commented Mar 8, 2016

I tried to adapt the program from #31662, but it is not trivial to make a test which is correct for all possible targets. AFAICT the idea is that the test should check whether the sse feature is available on i686-unknown-linux-gnu. How can I write a test that is only run on a specific triple? I would like to avoid having the test broken on i586-unknown-linux-gnu.

Would a print-cfg test like the following be sufficient?

$(RUSTC) --target i686-unknown-linux-gnu --print cfg | grep sse2

This test would not pass on stable/beta release channels until target_feature is stabilised. Is this a problem?

This morning I sent a message to the llvm-dev mailing list pointing out this approach for computing the features and asking for guidance. The relevant thread starts here.

@alexcrichton
Copy link
Member

Ah yeah a test like that should be fine. We've export the RUST_BOOTSTRAP_KEY during the CI builds of the stable/beta channels so I think that will pass even for those channels?

@alexcrichton
Copy link
Member

Also thanks for starting that thread! It's probably worth waiting a little bit to see if anyone responds.

@bors
Copy link
Contributor

bors commented Mar 27, 2016

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

@bors
Copy link
Contributor

bors commented Apr 20, 2016

💔 Test failed - auto-win-msvc-32-opt

pub fn has_avx2(sess: &Session) -> bool {
features_contain(sess, "+avx2")
}
let x86_whitelist = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be const X86_WHITELIST: &'static [&'static str] = &["..."]; instead of locals (may neaten the code slightly, since it should avoid the need for &...[..] below too).

@alexcrichton
Copy link
Member

@bors: retry

On Tuesday, April 19, 2016, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-win-msvc-32-opt
http://buildbot.rust-lang.org/builders/auto-win-msvc-32-opt/builds/3068


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#31709 (comment)


let tf = InternedString::new("target_feature");
for feat in whitelist {
if unsafe { LLVMRustHasFeature(target_machine, feat.as_ptr() as *const c_char) } {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be nice to have an assertion that the strings end in \0 here, to ensure people don't make a mistake when adding features in future.

@huonw
Copy link
Member

huonw commented Apr 20, 2016

Thanks for doing this! (Sorry for the delayed comments, been away.)

@bors
Copy link
Contributor

bors commented Apr 20, 2016

⌛ Testing commit 1ad8561 with merge 0a0296e...

@bors
Copy link
Contributor

bors commented Apr 20, 2016

💔 Test failed - auto-win-msvc-32-opt

This simplifies the code a bit and makes the types nicer, too.
Assert that the feature strings are NUL terminated, so that they will
be well-formed as C strings.

This is a safety check to ease the maintaninace and update of the
feature lists.
@ranma42
Copy link
Contributor Author

ranma42 commented Apr 20, 2016

Applied the improvements suggested by @huonw.

@alexcrichton
Copy link
Member

@bors: r+ ce99a5e

@bors
Copy link
Contributor

bors commented Apr 20, 2016

⌛ Testing commit ce99a5e with merge 92e3fb3...

bors added a commit that referenced this pull request Apr 20, 2016
Compute `target_feature` from LLVM

This is a work-in-progress fix for #31662.

The logic that computes the target features from the command line has been replaced with queries to the `TargetMachine`.
@bors bors merged commit ce99a5e into rust-lang:master Apr 20, 2016
@ranma42 ranma42 deleted the target_feature-from-llvm branch April 21, 2016 07:36
bors added a commit that referenced this pull request Apr 23, 2016
ranma42 referenced this pull request Aug 1, 2017
The function should accept feature strings that old LLVM might not
support.

Simplify the code using the same approach used by
LLVMRustPrintTargetFeatures.

Dummify the function for non 4.0 LLVM and update the tests accordingly.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants