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.

Sign up for GitHub

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

Jump to bottom

Unconditionally emit the target-cpu LLVM attribute. #56609

Merged
merged 1 commit into from
Dec 14, 2018
Merged

Unconditionally emit the target-cpu LLVM attribute. #56609

merged 1 commit into from
Dec 14, 2018

Conversation

michaelwoerister
Copy link
Member

This PR makes rustc always emit the target-cpu LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in libstd. So far libstd functions were the only function without a target-cpu attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 7, 2018
@michaelwoerister
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Dec 7, 2018

⌛ Trying commit 86822eb with merge 85f3e04b446e18c8043659f95d0118320dc135af...

@alexcrichton
Copy link
Member

r=me assuming perf looks reasonable

@bors
Copy link
Contributor

bors commented Dec 7, 2018

☀️ Test successful - status-travis
State: approved= try=True

@alexcrichton
Copy link
Member

@rust-timer build 85f3e04b446e18c8043659f95d0118320dc135af

@rust-timer
Copy link
Collaborator

Success: Queued 85f3e04b446e18c8043659f95d0118320dc135af with parent 1c3236a, comparison URL.

@nagisa
Copy link
Member

nagisa commented Dec 7, 2018

So far libstd functions were the only function without a target-cpu attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

That does not appear to be true at all? That is, I don’t recall seeing this attribute in code generated by rustc, ever.

There is one use case of mine that is semi-broken by unconditionally emitting target-cpu attribute. When investigating codegen bugs for trivial code, I like emiting LLVM-IR and running it through llc with varying -mtriple values. Now, with target-cpu applied to every function, llc would emit:

'X' is not a recognized processor for this target (ignoring processor)

a dozen times for each function, making it necessary to remove those attributes somehow to make it workable. Not that I’m gonna block this PR on it, it is just something I felt was not taken into account of being possible :)

@michaelwoerister
Copy link
Member Author

So far libstd functions were the only function without a target-cpu attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

That does not appear to be true at all? That is, I don’t recall seeing this attribute in code generated by rustc, ever.

What I meant was: When compiling all of your code with xLTO then only the libstd LLVM IR pulled in from crate metadata would not have the attribute.

I'm sorry about the regression you mention :(

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 85f3e04b446e18c8043659f95d0118320dc135af

@michaelwoerister
Copy link
Member Author

The performance difference looks like noise.

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Dec 10, 2018

📌 Commit 86822eb has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 10, 2018
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Dec 11, 2018
@pietroalbini
Rollup merge of rust-lang#56609 - michaelwoerister:unconditional-targ…
286c3cc 
…et-cpu-attr, r=alexcrichton

Unconditionally emit the target-cpu LLVM attribute.

This PR makes `rustc` always emit the `target-cpu` LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in `libstd`. So far `libstd` functions were the only function without a `target-cpu` attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton
@pietroalbini pietroalbini mentioned this pull request Dec 11, 2018
Closed
@bors
Copy link
Contributor

bors commented Dec 11, 2018

⌛ Testing commit 86822eb with merge 91e470ccc038fc9010f49872b1869d5bde2ad819...

@bors
Copy link
Contributor

bors commented Dec 11, 2018

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 11, 2018
@alexcrichton
Copy link
Member

@bors: retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Dec 12, 2018
@kennytm
Rollup merge of rust-lang#56609 - michaelwoerister:unconditional-targ…
b4d16ef 
…et-cpu-attr, r=alexcrichton

Unconditionally emit the target-cpu LLVM attribute.

This PR makes `rustc` always emit the `target-cpu` LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in `libstd`. So far `libstd` functions were the only function without a `target-cpu` attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton
@kennytm kennytm mentioned this pull request Dec 12, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Dec 13, 2018
@kennytm
Rollup merge of rust-lang#56609 - michaelwoerister:unconditional-targ…
3df7a92 
…et-cpu-attr, r=alexcrichton

Unconditionally emit the target-cpu LLVM attribute.

This PR makes `rustc` always emit the `target-cpu` LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in `libstd`. So far `libstd` functions were the only function without a `target-cpu` attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton
@kennytm kennytm mentioned this pull request Dec 13, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
@kennytm
Rollup merge of rust-lang#56609 - michaelwoerister:unconditional-targ…
c443aac 
…et-cpu-attr, r=alexcrichton

Unconditionally emit the target-cpu LLVM attribute.

This PR makes `rustc` always emit the `target-cpu` LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in `libstd`. So far `libstd` functions were the only function without a `target-cpu` attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton
@kennytm kennytm mentioned this pull request Dec 14, 2018
Closed
kennytm added a commit to kennytm/rust that referenced this pull request Dec 14, 2018
@kennytm
Rollup merge of rust-lang#56609 - michaelwoerister:unconditional-targ…
4f0f110 
…et-cpu-attr, r=alexcrichton

Unconditionally emit the target-cpu LLVM attribute.

This PR makes `rustc` always emit the `target-cpu` LLVM attribute for functions. The goal is to allow for cross-language inlining of functions defined in `libstd`. So far `libstd` functions were the only function without a `target-cpu` attribute, so in whole-crate-graph cross-lang LTO scenarios they were not eligible for inlining into foreign code.

r? @alexcrichton
This was referenced Dec 14, 2018
Closed
Merged
bors added a commit that referenced this pull request Dec 14, 2018
@bors
Auto merge of #56818 - kennytm:rollup-2, r=kennytm
1897657 
Rollup of 14 pull requests (first batch)

Successful merges:

 - #56562 (Update libc version required by rustc)
 - #56609 (Unconditionally emit the target-cpu LLVM attribute.)
 - #56637 (rustdoc: Fix local reexports of proc macros)
 - #56658 (Add non-panicking `maybe_new_parser_from_file` variant)
 - #56695 (Fix irrefutable matches on integer ranges)
 - #56699 (Use a `newtype_index!` within `Symbol`.)
 - #56702 ([self-profiler] Add column for percent of total time)
 - #56708 (Remove some unnecessary feature gates)
 - #56709 (Remove unneeded extra chars to reduce search-index size)
 - #56744 (specialize: remove Boxes used by Children::insert)
 - #56748 (Update panic message to be clearer about env-vars)
 - #56749 (x86: Add the `adx` target feature to whitelist)
 - #56756 (Disable btree pretty-printers on older gdbs)
 - #56789 (rustc: Add an unstable `simd_select_bitmask` intrinsic)

r? @ghost
@bors bors merged commit 86822eb into rust-lang:master Dec 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@alexcrichton alexcrichton

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michaelwoerister @bors @alexcrichton @rust-timer @nagisa @rust-highfive