Skip to content

update comment regarding TargetOptions.features - #129863

Merged
bors merged 1 commit into
rust-lang:masterfrom
RalfJung:target-spec-features
Sep 5, 2024
Merged

update comment regarding TargetOptions.features#129863
bors merged 1 commit into
rust-lang:masterfrom
RalfJung:target-spec-features

Conversation

@RalfJung

@RalfJungRalfJung commented Sep 1, 2024

Copy link
Copy Markdown
Member

The claim that -Ctarget-features cannot disable these features set in the target spec is definitely wrong -- I tried it for x86_64-pc-windows-gnu, which enables SSE3 that way. Building with -Ctarget-feature=-sse3 works fine, and cfg!(target_feature = "sse3") is false in that build.

There are also some indications that these are actually intended to be overwritten:

rust/compiler/rustc_target/src/spec/targets/i686_unknown_uefi.rs

Lines 22 to 23 in 3b14526

// If you initialize FP units yourself, you can override these flags with custom linker
// arguments, thus giving you access to full MMX/SSE acceleration.

rust/compiler/rustc_target/src/spec/targets/x86_64h_apple_darwin.rs

Lines 18 to 23 in 84ac80f

// FIXME: Sadly, turning these off here disables them in such a way that they
// aren't re-enabled by `-Ctarget-cpu=native` (on a machine that has them).
// It would be nice if this were not the case, but fixing it seems tricky
// (and given that the main use-case for this target is for use in universal
// binaries, probably not that important).
base.features = "-rdrnd,-aes,-pclmul,-rtm,-fsgsbase".into();

So... let's update the comment to match reality, I guess?

The claim that they overwrite -Ctarget-cpu is based on

  • for native, the comment in the apple target spec quoted above
  • for other CPU strings, the assumption that LLVMRustCreateTargetMachine will apply these features after doing whatever the base CPU model does. I am not sure how to check that, I hope some LLVM backend people can chime in. :)

@rustbot

rustbot commented Sep 1, 2024

Copy link
Copy Markdown
Collaborator

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbotrustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 1, 2024
@rustbot

rustbot commented Sep 1, 2024

Copy link
Copy Markdown
Collaborator

These commits modify compiler targets.
(See the Target Tier Policy.)

@RalfJung

RalfJung commented Sep 1, 2024 via email

Copy link
Copy Markdown
MemberAuthor

@RalfJung

RalfJung commented Sep 1, 2024

Copy link
Copy Markdown
MemberAuthor

LLVMRustCreateTargetMachine boils down to the createTargetMachine method on LLVM's TargetMachine class. The docs don't say whether the CPU or the Features take precedence... but I would expect it is the Features.

@wesleywiser

wesleywiser commented Sep 3, 2024

Copy link
Copy Markdown
Member

@bors r+ rollup

@bors

bors commented Sep 3, 2024

Copy link
Copy Markdown
Collaborator

📌 Commit 5c0dfc6 has been approved by wesleywiser

It is now in the queue for this repository.

@borsbors 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 Sep 3, 2024
@matthiaskrgrmatthiaskrgr mentioned this pull request Sep 3, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
76fb2c2
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
a257dd2
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
916e0e3
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
f59b9d8
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
ddb7c56
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
e158abf
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 4, 2024
@bors
Auto merge ofrust-lang#129936- matthiaskrgr:rollup-0s8xycb, r=matth…
009e738
…iaskrgr
Rollup of 9 pull requests
Successful merges:
- rust-lang#127692 (Suggest `impl Trait` for References to Bare Trait in Function Header)
- rust-lang#128701 (Don't Suggest Labeling `const` and `unsafe` Blocks )
- rust-lang#128934 (Non-exhaustive structs may be empty)
- rust-lang#129630 (Document the broken C ABI of `wasm32-unknown-unknown`)
- rust-lang#129863 (update comment regarding TargetOptions.features)
- rust-lang#129896 (do not attempt to prove unknowable goals)
- rust-lang#129926 (Move `SanityCheck` and `MirPass`)
- rust-lang#129928 (rustc_driver_impl: remove some old dead logic)
- rust-lang#129930 (include 1.80.1 release notes on master)
r? `@ghost`
`@rustbot` modify labels: rollup
@bors
bors merged commit 2f6e855 into rust-lang:masterSep 5, 2024
@rustbotrustbot added this to the 1.83.0 milestone Sep 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
@rust-timer
Unrolled build forrust-lang#129863
c28fa0b
Rollup merge of rust-lang#129863 - RalfJung:target-spec-features, r=wesleywiser
update comment regarding TargetOptions.features
The claim that `-Ctarget-features` cannot disable these features set in the target spec is definitely wrong -- I tried it for `x86_64-pc-windows-gnu`, which enables SSE3 that way. Building with `-Ctarget-feature=-sse3` works fine, and `cfg!(target_feature = "sse3")` is `false` in that build.
There are also some indications that these are actually intended to be overwritten:
https://github.com/rust-lang/rust/blob/3b14526cead4105f82c398d8d4c7954efa3bab6b/compiler/rustc_target/src/spec/targets/i686_unknown_uefi.rs#L22-L23https://github.com/rust-lang/rust/blob/84ac80f1921afc243d71fd0caaa4f2838c294102/compiler/rustc_target/src/spec/targets/x86_64h_apple_darwin.rs#L18-L23
So... let's update the comment to match reality, I guess?
The claim that they overwrite `-Ctarget-cpu` is based on
- for `native`, the comment in the apple target spec quoted above
- for other CPU strings, the assumption that `LLVMRustCreateTargetMachine` will apply these features after doing whatever the base CPU model does. I am not sure how to check that, I hope some LLVM backend people can chime in. :)
@RalfJung
RalfJung deleted the target-spec-features branch September 9, 2024 08:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wesleywiserwesleywiserwesleywiser approved these changes

Assignees

@wesleywiserwesleywiser

Labels

S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

Projects

None yet

Milestone

1.83.0

Development

Successfully merging this pull request may close these issues.

4 participants

@RalfJung@rustbot@wesleywiser@bors