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

Remove SSE ABI from i586-pc-windows-msvc #137149

Merged
merged 1 commit into from
Feb 17, 2025
Merged

Remove SSE ABI from i586-pc-windows-msvc #137149

merged 1 commit into from
Feb 17, 2025
+1 −0

Conversation

Noratrieb
Copy link
Member

As an i586 target, it should not have SSE. This caused the following warning to be emitted:

warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue #116344 <https://github.com/rust-lang/rust/issues/116344>

warning: 1 warning emitted

see #116344.

r? RalfJung

@Noratrieb
Remove SSE ABI from i586-pc-windows-msvc
1c66d5b 
As an i586 target, it should not have SSE. This caused the following
warning to be emitted:

```
warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#116344 <rust-lang#116344>

warning: 1 warning emitted
```
@rustbot rustbot 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 Feb 16, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2025

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

@Noratrieb
Copy link
Member Author

Before someone questions why I would use this target, don't worry. I was simply running --print target-spec-json on every target :D

@RalfJung
Copy link
Member

Ah, I must have missed this one -- having it inherit from the "stronger" target makes this quite error-prone.

@bors r+ rollup

I was simply running --print target-spec-json on every target :D

So you're reasonably sure that this is the only problematic one?

@bors
Copy link
Contributor

bors commented Feb 16, 2025

📌 Commit 1c66d5b has been approved by RalfJung

It is now in the queue for this repository.

@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 Feb 16, 2025
@RalfJung RalfJung added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 16, 2025
@RalfJung
Copy link
Member

Nominating for backporting to the beta; this is a tier 2 target and without this PR everyone using it will get the above warning.

@Noratrieb
Copy link
Member Author

Ah, I must have missed this one -- having it inherit from the "stronger" target makes this quite error-prone.

@bors r+ rollup

I was simply running --print target-spec-json on every target :D

So you're reasonably sure that this is the only problematic one?

Yes, I am sure that this is the only target where this warning is emitted. I also double-checked all the other i585 targets and they either override it when inheriting or don't inherit at all.

workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Feb 16, 2025
@workingjubilee
Rollup merge of rust-lang#137149 - Noratrieb:wtf-is-this-target, r=Ra…
759ccca 
…lfJung

Remove SSE ABI from i586-pc-windows-msvc

As an i586 target, it should not have SSE. This caused the following warning to be emitted:

```
warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#116344 <rust-lang#116344>

warning: 1 warning emitted
```

see rust-lang#116344.

r? RalfJung
@workingjubilee workingjubilee mentioned this pull request Feb 16, 2025
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137153 - workingjubilee:rollup-1cjmnuh, r=wor…
57fb317 
…kingjubilee

Rollup of 12 pull requests

Successful merges:

 - rust-lang#136671 (Overhaul `rustc_middle::limits`)
 - rust-lang#136817 (Pattern Migration 2024: clean up and comment)
 - rust-lang#136844 (Use `const_error!` when possible)
 - rust-lang#136953 (rustc_target: import TargetMetadata)
 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137140 (Fix const items not being allowed to be called `r#move` or `r#static`)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@matthiaskrgr matthiaskrgr mentioned this pull request Feb 17, 2025
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@bors
Auto merge of rust-lang#137164 - matthiaskrgr:rollup-dj5826k, r=matth…
2162e9d 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#137095 (Replace some u64 hashes with Hash64)
 - rust-lang#137100 (HIR analysis: Remove unnecessary abstraction over list of clauses)
 - rust-lang#137105 (Restrict DerefPure for Cow<T> impl to T = impl Clone, [impl Clone], str.)
 - rust-lang#137120 (Enable `relative-path-include-bytes-132203` rustdoc-ui test on Windows)
 - rust-lang#137125 (Re-add missing empty lines in the releases notes)
 - rust-lang#137145 (use add-core-stubs / minicore for a few more tests)
 - rust-lang#137149 (Remove SSE ABI from i586-pc-windows-msvc)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 183fc30 into rust-lang:master Feb 17, 2025
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2025
@rust-timer
Unrolled build for rust-lang#137149
aeeb129 
Rollup merge of rust-lang#137149 - Noratrieb:wtf-is-this-target, r=RalfJung

Remove SSE ABI from i586-pc-windows-msvc

As an i586 target, it should not have SSE. This caused the following warning to be emitted:

```
warning: target feature `sse2` must be enabled to ensure that the ABI of the current target can be implemented correctly
  |
  = note: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#116344 <rust-lang#116344>

warning: 1 warning emitted
```

see rust-lang#116344.

r? RalfJung
@rustbot rustbot added this to the 1.87.0 milestone Feb 17, 2025
@Noratrieb Noratrieb deleted the wtf-is-this-target branch February 17, 2025 14:39
@RalfJung
Copy link
Member

(To be clear this is meant for backport to the just-branched beta that will be released in 6 weeks, not the about-to-be-released stable.)

@RalfJung
Copy link
Member

@Noratrieb

Yes, I am sure that this is the only target where this warning is emitted. I also double-checked all the other i585 targets and they either override it when inheriting or don't inherit at all.

Is there some way we could turn this into an automated test? I was wondering how to best ensure that all our built-in targets pass the target feature ABI check, but so far didn't find anything great.

@Noratrieb
Copy link
Member Author

Having a "execute -vV for all targets" test seems reasonable to me. We could run it with -Dwarnings or even ensure that there's no stderr outout at all (there currently is for some other targets, some MIPS thing saying it's experimental in LLVM and #137150

@apiraino
Copy link
Contributor

Yes, I am sure that this is the only target where this warning is emitted. I also double-checked all the other i585 targets and they either override it when inheriting or don't inherit at all.

@Noratrieb Isn't there a proposal to remove this target completely in MCP#840? Are there actual people using it and thus is a backport interesting? (asking more for curiosity)

@RalfJung
Copy link
Member

It seems like a low-risk backport to avoid the situation of everyone using that target getting a bogus warning. That's why I nominated it. I don't think the fact that the target will likely be removed soon changes that calculus.

@Noratrieb
Copy link
Member Author

I wouldn't bother backporting it given that the target useless and will be removed.

@apiraino
Copy link
Contributor

Beta backport accepted as per compiler team on Zulip. A backport PR will be authored by the release team at the end of the current development cycle. Backport labels handled by them.

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Feb 20, 2025
@cuviper cuviper mentioned this pull request Feb 21, 2025
Open
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Feb 21, 2025
@cuviper cuviper modified the milestones: 1.87.0, 1.86.0 Feb 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@RalfJung RalfJung

Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.
6 participants
@Noratrieb @rustbot @RalfJung @bors @apiraino @cuviper