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

Direct users towards using Rust target feature names in CLI #87402

Merged

Conversation

nagisa
Copy link
Member

@nagisa nagisa commented Jul 23, 2021

This PR consists of a couple of changes on how we handle target features.

In particular there is a bug-fix wherein we avoid passing through features that aren't prefixed by + or - to LLVM. These appear to be causing LLVM to assert, which is pretty poor a behaviour (and also makes it pretty clear we expect feature names to be prefixed).

The other commit, I anticipate to be somewhat more controversial is outputting a warning when users specify a LLVM-specific, or otherwise unknown, feature name on the CLI. In those situations we request users to either replace it with a known Rust feature name (e.g. bmi -> bmi1) or file a feature request. I've a couple motivations for this: first of all, if users are specifying these features on the command line, I'm pretty confident there is also a need for these features to be usable via #[cfg(target_feature)] machinery. And second, we're growing a fair number of backends recently and having ability to provide some sort of unified-ish interface in this place seems pretty useful to me.

Sponsored by: standard.ai

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Jul 23, 2021
@bors
Copy link
Contributor

bors commented Aug 1, 2021

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

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@nagisa nagisa force-pushed the nagisa/request-feature-requests-for-features branch from e15e415 to 1761c60 Compare September 24, 2021 13:41
@nagisa
Copy link
Member Author

nagisa commented Sep 24, 2021

Hm, one remaining problem is that we call llvm_global_features for each function so there will be warnings repeated many times for a typical compilation. Will think about how to best address that.

@nagisa nagisa force-pushed the nagisa/request-feature-requests-for-features branch from c32957a to 4b66ad5 Compare September 24, 2021 15:07
@rust-log-analyzer

This comment has been minimized.

@nagisa nagisa force-pushed the nagisa/request-feature-requests-for-features branch from 4b66ad5 to e25afb2 Compare September 24, 2021 16:37
@nagisa nagisa marked this pull request as ready for review September 24, 2021 16:37
@nagisa
Copy link
Member Author

nagisa commented Sep 24, 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 Sep 24, 2021
@bors
Copy link
Contributor

bors commented Sep 24, 2021

⌛ Trying commit e25afb20b11f9cc764ea5e1f6d593b504bc1807f with merge 0c190043f011023658fc263567eaf5bc76f3cb4b...

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Sep 24, 2021

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

@rust-timer
Copy link
Collaborator

Queued 0c190043f011023658fc263567eaf5bc76f3cb4b with parent a0648ea, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0c190043f011023658fc263567eaf5bc76f3cb4b): comparison url.

Summary: This change led to very large relevant regressions 😿 in compiler performance.

  • Very large regression in instruction counts (up to 4651.6% on incr-unchanged builds of webrender)

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

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

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Sep 24, 2021
@nagisa
Copy link
Member Author

nagisa commented Sep 24, 2021

4 digits of regression. Nice.

I think I can guess what the reason is. As the global_backend_features is now a run-always+no-hash query, it will be run and not hashed every single time the compiler is invoked. We are probably calling the query somewhere early in the dependency tree which causes us to invalidate large parts of the tree.

Things to try: remove no_hash…?

@nagisa nagisa force-pushed the nagisa/request-feature-requests-for-features branch 2 times, most recently from ea176be to 69025a6 Compare September 24, 2021 20:57
@nagisa
Copy link
Member Author

nagisa commented Sep 24, 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 Sep 24, 2021
@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 1, 2022
@bors
Copy link
Contributor

bors commented Mar 1, 2022

⌛ Testing commit df701a2 with merge 7d6dc0530f5c4fcb230282f143f7ac71bedf105c...

@bors
Copy link
Contributor

bors commented Mar 1, 2022

💥 Test timed out

@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 Mar 1, 2022
@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@matthiaskrgr
Copy link
Member

@bors retry timeout

@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 Mar 1, 2022
@bors
Copy link
Contributor

bors commented Mar 2, 2022

⌛ Testing commit df701a2 with merge 39a3b52...

@bors
Copy link
Contributor

bors commented Mar 2, 2022

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 39a3b52 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 2, 2022
@bors bors merged commit 39a3b52 into rust-lang:master Mar 2, 2022
@rustbot rustbot added this to the 1.61.0 milestone Mar 2, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (39a3b52): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Mar 26, 2022
…s-for-features, r=estebank

Direct users towards using Rust target feature names in CLI

This PR consists of a couple of changes on how we handle target features.

In particular there is a bug-fix wherein we avoid passing through features that aren't prefixed by `+` or `-` to LLVM. These appear to be causing LLVM to assert, which is pretty poor a behaviour (and also makes it pretty clear we expect feature names to be prefixed).

The other commit, I anticipate to be somewhat more controversial is outputting a warning when users specify a LLVM-specific, or otherwise unknown, feature name on the CLI. In those situations we request users to either replace it with a known Rust feature name (e.g. `bmi` -> `bmi1`) or file a feature request. I've a couple motivations for this: first of all, if users are specifying these features on the command line, I'm pretty confident there is also a need for these features to be usable via `#[cfg(target_feature)]` machinery.  And second, we're growing a fair number of backends recently and having ability to provide some sort of unified-ish interface in this place seems pretty useful to me.

Sponsored by: standard.ai
ojeda added a commit to ojeda/linux that referenced this pull request May 19, 2022
…t spec file

Only a subset of the LLVM codegen features are recognized by `rustc`,
and since rust-lang/rust#87402 (Rust 1.61.0)
the compiler gives a warning about it:

    warning: unknown feature specified for `-Ctarget-feature`: `mmx`
      |
      = note: it is still passed through to the codegen backend
      = note: consider filing a feature request

...since those features may be renamed or removed at any point:

    $ rustc --print target-features

    [...]

    Code-generation features cannot be used in cfg or #[target_feature],
    and may be renamed or removed in a future version of LLVM or rustc.

Thus move them back to the target spec generated file.

See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this pull request May 19, 2022
…t spec file

Only a subset of the LLVM codegen features are recognized by `rustc`,
and since rust-lang/rust#87402 (Rust 1.61.0)
the compiler gives a warning about it:

    warning: unknown feature specified for `-Ctarget-feature`: `mmx`
      |
      = note: it is still passed through to the codegen backend
      = note: consider filing a feature request

...since those features may be renamed or removed at any point:

    $ rustc --print target-features

    [...]

    Code-generation features cannot be used in cfg or #[target_feature],
    and may be renamed or removed in a future version of LLVM or rustc.

Thus move them back to the target spec generated file.

See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jul 25, 2022
Add `sign-ext` target feature to the WASM target

Some target features are still missing from that list.
See rust-lang#97808 for basically the same PR by `@alexcrichton.`

Related issue: rust-lang#96472.
PR introducing this issue: rust-lang#87402.
ojeda added a commit to ojeda/linux that referenced this pull request Aug 1, 2022
…t spec file

Similar to c5eae3a ("x86: rust: move unknown-to-`rustc` codegen
features back to the target spec file"), but for `retpoline-external-thunk`:

    Only a subset of the LLVM codegen features are recognized by `rustc`,
    and since rust-lang/rust#87402 (Rust 1.61.0)
    the compiler gives a warning about it:

        warning: unknown feature specified for `-Ctarget-feature`: `mmx`
          |
          = note: it is still passed through to the codegen backend
          = note: consider filing a feature request

    ...since those features may be renamed or removed at any point:

        $ rustc --print target-features

        [...]

        Code-generation features cannot be used in cfg or #[target_feature],
        and may be renamed or removed in a future version of LLVM or rustc.

    Thus move them back to the target spec generated file.

    See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
ojeda added a commit to ojeda/linux that referenced this pull request Aug 1, 2022
…t spec file

Similar to c5eae3a ("x86: rust: move unknown-to-`rustc` codegen
features back to the target spec file"), but for `retpoline-external-thunk`:

    Only a subset of the LLVM codegen features are recognized by `rustc`,
    and since rust-lang/rust#87402 (Rust 1.61.0)
    the compiler gives a warning about it:

        warning: unknown feature specified for `-Ctarget-feature`: `mmx`
          |
          = note: it is still passed through to the codegen backend
          = note: consider filing a feature request

    ...since those features may be renamed or removed at any point:

        $ rustc --print target-features

        [...]

        Code-generation features cannot be used in cfg or #[target_feature],
        and may be renamed or removed in a future version of LLVM or rustc.

    Thus move them back to the target spec generated file.

    See rust-lang/rust#96472 as well for a report.

Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.