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

Adjust -Ctarget-cpu=native handling in cg_llvm #83084

Merged
merged 1 commit into from
Mar 17, 2021
Merged

Adjust -Ctarget-cpu=native handling in cg_llvm #83084

merged 1 commit into from
Mar 17, 2021

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Mar 13, 2021 •
edited
Loading

When cg_llvm encounters the -Ctarget-cpu=native it computes an
explciit set of features that applies to the target in order to
correctly compile code for the host CPU (because e.g. skylake alone is
not sufficient to tell if some of the instructions are available or
not).

However there were a couple of issues with how we did this. Firstly, the
order in which features were overriden wasn't quite right – conceptually
you'd expect -Ctarget-cpu=native option to override the features that
are implicitly set by the target definition. However due to how other
-Ctarget-cpu values are handled we must adopt the following order
of priority:

  • Features from -Ctarget-cpu=*; are overriden by
  • Features implied by --target; are overriden by
  • Features from -Ctarget-feature; are overriden by
  • function specific features.

Another problem was in that the function level target-features
attribute would overwrite the entire set of the globally enabled
features, rather than just the features the
#[target_feature(enable/disable)] specified. With something like
-Ctarget-cpu=native we'd end up in a situation wherein a function
without #[target_feature(enable)] annotation would have a broader
set of features compared to a function with one such attribute. This
turned out to be a cause of heavy run-time regressions in some code
using these function-level attributes in conjunction with
-Ctarget-cpu=native, for example.

With this PR rustc is more careful about specifying the entire set of
features for functions that use #[target_feature(enable/disable)] or
#[instruction_set] attributes.

Sadly testing the original reproducer for this behaviour is quite
impossible – we cannot rely on -Ctarget-cpu=native to be anything in
particular on developer or CI machines.

cc #83027 @BurntSushi

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@nagisa nagisa force-pushed the nagisa/features-native branch 2 times, most recently from be2beea to 1e8f7db Compare March 13, 2021 17:56
@BurntSushi
Copy link
Member

Thank you @nagisa!!!

@lqd
Copy link
Member

lqd commented Mar 14, 2021

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2021
@bors
Copy link
Contributor

bors commented Mar 14, 2021

⌛ Trying commit 1e8f7dbfcd15e5154f84e00c1e4c9603f14752e3 with merge dda2654d0b2e9f8d11f6c86075e2a08bd80867d9...

@bors
Copy link
Contributor

bors commented Mar 14, 2021

☀️ Try build successful - checks-actions
Build commit: dda2654d0b2e9f8d11f6c86075e2a08bd80867d9 (dda2654d0b2e9f8d11f6c86075e2a08bd80867d9)

@rust-timer
Copy link
Collaborator

Queued dda2654d0b2e9f8d11f6c86075e2a08bd80867d9 with parent 2caeeb0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (dda2654d0b2e9f8d11f6c86075e2a08bd80867d9): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 14, 2021
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 14, 2021
@nagisa nagisa added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 15, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor

PR description needs to be updated.
r=me after that.

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 15, 2021
@nagisa
Copy link
Member Author

nagisa commented Mar 16, 2021

@bors r=petrochenkov

(was missing a --target in one of the tests)

@bors
Copy link
Contributor

bors commented Mar 16, 2021

📌 Commit 72fb437 has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 16, 2021
@bors
Copy link
Contributor

bors commented Mar 17, 2021

⌛ Testing commit 72fb437 with merge 0c34122...

@bors
Copy link
Contributor

bors commented Mar 17, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 0c34122 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 17, 2021
@bors bors merged commit 0c34122 into rust-lang:master Mar 17, 2021
@rustbot rustbot added this to the 1.52.0 milestone Mar 17, 2021
@camelid
Copy link
Member

camelid commented Mar 17, 2021

Does this fix #83027?

@camelid
Copy link
Member

camelid commented Mar 17, 2021

If so, this PR should probably be backported to beta so the bug isn't in 1.51.

@nagisa
Copy link
Member Author

nagisa commented Mar 17, 2021

I didn't test the original reproducer, remains for the others to verify.

@pnkfelix pnkfelix added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 18, 2021
@pnkfelix
Copy link
Member

did a quick beta nomination and attempted to discuss at T-compiler meeting today, but the meeting was over time at that point so we didn't get much discussion.

collectively I'd say we are hesitant at best to beta-backport this change. As nagisa put it in the meeting:

this problem only affects a small number of people using a very specific flag combination which isn't a default and has been known to break in other ways in the past (miscompile)

I'm leaving the beta-nomination label on it at the moment while we wait for @BurntSushi to respond about whether PR #83084 fixes #83027. But even if it does fix issue #83027, it seems likely that we'll still decline to backport; its a big patch and somewhat risky, which is a tough tradeoff given context established by nagisa above.

@rustbot rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 18, 2021
@apiraino apiraino added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 18, 2021
@BurntSushi BurntSushi mentioned this pull request Mar 18, 2021
Closed
@BurntSushi
Copy link
Member

@pnkfelix I agree with not back-porting. If this didn't require target-cpu=native to be triggered, that'd be a different story. But target-cpu=native definitely makes this lower impact!

@pnkfelix
Copy link
Member

(hmm can someone tell me why rustbot removed the beta-nomination up above? Was that because of #83027 getting closed somehow...?)

@pnkfelix pnkfelix removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 18, 2021
@apiraino
Copy link
Contributor

apiraino commented Mar 18, 2021 •
edited
Loading

(hmm can someone tell me why rustbot removed the beta-nomination up above? Was that because of #83027 getting closed somehow...?)

@pnkfelix heh ... github web UI sometimes fails to update comments. I had wrote a comment to @rustbot label -beta-nominated 7 minutes after you wrote yours but my page didn't refresh so I wasnt aware of it. I have then removed my comment and manually reinstated the beta-nominated label. Which you then removed again 😄

@as-com as-com mentioned this pull request Mar 24, 2021
Closed
@hkratz hkratz mentioned this pull request May 3, 2021
Closed
@petrochenkov petrochenkov mentioned this pull request May 6, 2021
Merged
@SNCPlay42 SNCPlay42 mentioned this pull request May 27, 2021
Open
@GuentherVIII GuentherVIII mentioned this pull request Oct 9, 2023
Closed
@RalfJung RalfJung mentioned this pull request Sep 29, 2024
Merged
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 29, 2024
@GuillaumeGomez
Rollup merge of rust-lang#131010 - RalfJung:target-cpu-native, r=jiey…
e9c9307 
…ouxu

extend comment in global_llvm_features regarding target-cpu=native

Based on the description in rust-lang#83084 by `@nagisa` -- seems better to have this in the code, where it is easier to find.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 29, 2024
@rust-timer
Unrolled build for rust-lang#131010
ec2bc42 
Rollup merge of rust-lang#131010 - RalfJung:target-cpu-native, r=jieyouxu

extend comment in global_llvm_features regarding target-cpu=native

Based on the description in rust-lang#83084 by `@nagisa` -- seems better to have this in the code, where it is easier to find.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@petrochenkov petrochenkov petrochenkov left review comments

@lqd lqd lqd left review comments

Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.