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

Move versioned Apple LLVM targets from rustc_target to rustc_codegen_ssa #131037

Merged
merged 3 commits into from
Nov 2, 2024

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Sep 29, 2024

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like MACOSX_DEPLOYMENT_TARGET, IPHONEOS_DEPLOYMENT_TARGET etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from #130883 to do #118204. See also #129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in #130435.

r? @petrochenkov

@rustbot rustbot added O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) 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 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Sep 29, 2024

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

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

@rust-log-analyzer

This comment has been minimized.

@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from c4b18fc to d472728 Compare September 30, 2024 14:08
@bors

This comment was marked as resolved.

@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch 4 times, most recently from 190c9d9 to 7750660 Compare October 3, 2024 14:56
@madsmtm madsmtm mentioned this pull request Oct 10, 2024
9 tasks
compiler/rustc_target/src/spec/base/apple/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/base/apple/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_target/src/spec/base/apple/mod.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/link.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

Sorry for the delays.
@rustbot author

@rustbot rustbot 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 Oct 21, 2024
@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from 7750660 to 9a50775 Compare October 31, 2024 18:45
@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 31, 2024

Sorry for the delays.

No problem! Resolved your concerns, so @rustbot ready

@rustbot rustbot 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 Oct 31, 2024
@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from 9a50775 to 43d42f0 Compare October 31, 2024 19:09
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 1, 2024

I just found out when going through cc code that the powerpc-unknown-freebsd seems to use a (hardcoded) versioned LLVM target as well - should I also move that to rustc_codegen_ssa?

@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from 43d42f0 to 7f4f39c Compare November 1, 2024 15:52
@petrochenkov
Copy link
Contributor

I just found out when going through cc code that the powerpc-unknown-freebsd seems to use a (hardcoded) versioned LLVM target as well - should I also move that to rustc_codegen_ssa?

This regex llvm_target: ".*\..*" finds 2 versioned built-in targets - aarch64-unknown-solaris2.11 and powerpc-unknown-freebsd13.0.
Not sure how target versions work for those targets, it's probably better to ask their maintainers, and keep this PR apple-centric.

compiler/rustc_codegen_ssa/src/back/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/back/apple.rs Outdated Show resolved Hide resolved
@petrochenkov
Copy link
Contributor

r=me with a couple of visibilities reduced.
@rustbot author

@rustbot rustbot 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 Nov 1, 2024
@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from 7f4f39c to ee66c70 Compare November 1, 2024 16:05
The OS version depends on the deployment target environment variables,
the access of which we want to move to later in the compilation pipeline
that has access to more information, for example `env_depinfo`.
To align with the general decision to have this sort of information
there instead.

Also use the visionOS values added in newer `object` release.
@madsmtm madsmtm force-pushed the move-llvm-target-versioning branch from ee66c70 to 1ef1af1 Compare November 1, 2024 16:07
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 1, 2024

@rustbot ready

I just found out when going through cc code that the powerpc-unknown-freebsd seems to use a (hardcoded) versioned LLVM target as well - should I also move that to rustc_codegen_ssa?

This regex llvm_target: ".*\..*" finds 2 versioned built-in targets - aarch64-unknown-solaris2.11 and powerpc-unknown-freebsd13.0. Not sure how target versions work for those targets, it's probably better to ask their maintainers, and keep this PR apple-centric.

I've added a FIXME for now.

@rustbot rustbot 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 Nov 1, 2024
@petrochenkov
Copy link
Contributor

Thanks!
@bors r+

@bors
Copy link
Contributor

bors commented Nov 1, 2024

📌 Commit 1ef1af1 has been approved by petrochenkov

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 Nov 1, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 1, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `@petrochenkov`
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132147 (Tweak E0277 output when a candidate is available)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132471 (Add a bunch of mailmap entries)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#125579 (Add `--print host-tuple` to print host target tuple)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132471 (Add a bunch of mailmap entries)

Failed merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ```@petrochenkov```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ````@petrochenkov````
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Nov 2, 2024
…g, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? `````@petrochenkov`````
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 11 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132170 (Add a Few Codegen Tests)
 - rust-lang#132333 (rust_analyzer_helix.toml: add library/ manifest)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132411 (CI: switch dist-x86_64-musl to free runner)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132466 (Account for late-bound depth when capturing all opaque lifetimes.)
 - rust-lang#132471 (Add a bunch of mailmap entries)
 - rust-lang#132488 (Remove or fix some more `FIXME(async_closure)`)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit bb544f8 into rust-lang:master Nov 2, 2024
6 checks passed
@rustbot rustbot added this to the 1.84.0 milestone Nov 2, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#131037 (Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`)
 - rust-lang#132398 (Add a couple of intra-doc links to str)
 - rust-lang#132453 (Also treat `impl` definition parent as transparent regarding modules)
 - rust-lang#132457 (Remove needless #![feature(asm_experimental_arch)] from loongarch64 inline assembly test)
 - rust-lang#132465 (refactor(config): remove FIXME statement in comment of `omit-git-hash`)
 - rust-lang#132466 (Account for late-bound depth when capturing all opaque lifetimes.)
 - rust-lang#132471 (Add a bunch of mailmap entries)
 - rust-lang#132488 (Remove or fix some more `FIXME(async_closure)`)

r? `@ghost`
`@rustbot` modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 2, 2024
Rollup merge of rust-lang#131037 - madsmtm:move-llvm-target-versioning, r=petrochenkov

Move versioned Apple LLVM targets from `rustc_target` to `rustc_codegen_ssa`

Fully specified LLVM targets contain the OS version on macOS/iOS/tvOS/watchOS/visionOS, and this version depends on the deployment target environment variables like `MACOSX_DEPLOYMENT_TARGET`, `IPHONEOS_DEPLOYMENT_TARGET` etc.

We would like to move this to later in the compilation pipeline, both because it feels impure to access environment variables when fetching target information, but mostly because we need access to more information from rust-lang#130883 to do rust-lang#118204. See also rust-lang#129342 (comment) for some discussion.

The first and second commit does the actual refactor, it should be a non-functional change, the third commit adds diagnostics for invalid deployment targets, which are now possible to do because we have access to the session.

Tested with the same commands as in rust-lang#130435.

r? ``````@petrochenkov``````
@madsmtm madsmtm deleted the move-llvm-target-versioning branch November 2, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) 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.

6 participants