Convert -Ctarget-cpu into a target-modifier for AVR, AMDGCN and NVPTX

[kulst]

View all comments

Crates built for AVR, AMDGCN and NVPTX that specify different values for -Ctarget-cpu cannot be soundly linked together.
This PR attempts to make rustc ensure that no crates with disagreeing values for -Ctarget-cpu are linked together. This is achieved by converting -Ctarget-cpu into a target-modifier depending on --target.
To do this, the consistency check for -Ctarget-cpu considers mismatching values as inconsistent only for targets for which the new flag requires_consistent_cpu is set in their target spec.

Why should -Ctarget-cpu be a target-modifier for nvptx?

PTX is a single-module contract

PTX requires a binary to start with .version (`ptx$$`) then .target (`sm_$$`). If the ptx contains instructions that are not supported by either .version or .target, the binary is ill-formed and will be rejected by ptxas. The concept of features that can be mixed and matched in a binary does not exist for nvptx and is therefore not supported by LLVM.

It prevents the production of bitcode that cannot be codegen'd after linking

A target modifier should prevent configurations that are not composable across crates when those crates are linked together. The most prominent example is when enabling a target feature changes the ABI, making cross-crate calls inherently unsound.

In the case of nvptx, ABI mismatch is (at least for now) not the core problem motivating target modifiers. NVIDIA’s documented PTX calling convention has remained stable since ptx20.

However, in the current state it is possible to produce bitcode that cannot be codegen'd after linking, because some operations are only lowerable for sufficiently new SM/PTX levels. In the best case this results in an LLVM error during the final llc step, but this is not something we should rely on for correctness.

nvptx has a special compilation pipeline where instead of linking the final PTX object, instead LLVM bitcode is linked. The resulting artifact is then compiled in one invocation.
Now consider crate A which is independently compiled into bitcode with the following rustc arguments:

//@ compile-flags: --target nvptx64-nvidia-cuda -C target-cpu=sm_70 --crate-type=rlib
#[cfg(target_feature = "sm_70")]
fn foo() { 
    // cannot be lowered to ptx before "sm_70: so currently produces an LLVM error
}
#[cfg(not(target_feature = "sm_70"))]
fn foo() {
    // can be lowered to ptx before "sm_70"
}
pub fn bar() {
    foo()
}

Crate A is a dependency of crate B. In the rustc invocation of crate B

1. crate B is compiled into bitcode, too
2. both bitcode artifacts are bitcode-linked by llvm-link
3. the resulting bitcode artifact is compiled by llc -mcpu=sm_60

This should now ideally create an LLVM error, because the linked bitcode contains code paths that were selected under sm_70 assumptions but the final NVPTX codegen is targeting sm_60, where those operations are not lowerable. An LLVM error here is better than silent miscompilation, but it’s not a promise we should rely on.

A real example where this could happen is the lowering of atomic loads and stores with non-relaxed orderings, which is known to depend on the selected SM level.

Why should -Ctarget-cpu be a target-modifier for amdgcn and avr?

• In case of AVR the target-cpu defines the ISA, which is encoded in the ELF header flags, amdgcn also encodes the cpu directly into those flags
• To not rely on lld which currently prevents it for both by looking at those flags AVR and amdgcn

Previous discussions about the topic can be found here and here.

I also created a Zulip discussion.

I am unsure if a MCP is needed before proceeding. If you think so please let me know.

Creating target-modifiers for NVPTX target-features is to be done in a follow-up.

cc @kjetilkjeka as target maintainer for NVPTX
cc @Flakebi as target maintainer for amdgcn
cc @Patryk27 as target maintainer for AVR
cc @RalfJung you were very involved in the discussions so far

Target modifier tracking issue: #136966

[kulst]

@rustbot label: +O-NVPTX

[rustbot]

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

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

[rustbot]

r? @nnethercote

rustbot has assigned @nnethercote.
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

[kulst]

r? @bjorn3 as you suggested this change.

[kjetilkjeka]

It works out really nice with -CTarget-cpu being able to be a target modifier.

It would be nice to know what the plan was with the ptx isa version as well. I asked that question here

[RalfJung]

I can't vouch for the implementation, but the approach sounds good. :)

Cc @Darksonn

[kulst]

I have added some explanation of why -Ctarget-cpu should be a target-modifier for nvptx to the PR. Please let me know if something is missing or if there are other assumptions about the topic.

[Darksonn]

There might already be relevant tests, but I want to see tests that this only makes -Ctarget-cpu into a target modifier in this case, and not for all targets.

View changes since this review

[Darksonn]

What happens if -Ctarget-cpu appears multiple times on the command line? Please add a test.

[kulst]

The value of the last appearance in matches is used. In practice this is probably the value of -Ctarget-cpus last appearance in the command line arguments of the rustc invocation.

Will add a test soon, but I am asking myself what is the intended behavior? I assume we should error if a target-modifier appears more than once, would you agree?

[RalfJung]

At the very least we have to ensure that whatever values rustc actually uses and what is recorded as target modifier are the same.

[kulst]

Last argument taking precedence is the desired functionality:
Corresponding MCP

[Darksonn]

Behavior as per MCP is fine. Whichever value ends up being used should be tracked and verified.

[kulst]

Is checked now by tests/run-make/target-cpu-is-target-modifier/rmake.rs
Does that match what you had in mind?

[rustbot]

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr

The reflection data structures are tied exactly to the implementation
in the compiler. Make sure to also adjust `rustc_const_eval/src/const_eval/type_info.rs

cc @oli-obk

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

Some changes occurred in tests/ui/stack-protector

cc @rust-lang/project-exploit-mitigations, @rcvalle

Some changes occurred to the CTFE / Miri interpreter

cc @rust-lang/miri

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

[rustbot]

rustc_codegen_cranelift is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_cranelift instead.

cc @bjorn3

rustc_codegen_gcc is developed in its own repository. If possible, consider making this change to rust-lang/rustc_codegen_gcc instead.

cc @antoyo, @GuillaumeGomez

[kulst]

I've made the requested changes and rebased on top of the current main branch.
@rustbot ready

[kulst]

I have revised the tests and integrated the other suggestions.

• tests/run-make/target-cpu-as-target-modifier now uses a stable test logic to verify that only the intented targets treat -Ctarget-cpu as a target modifier and all other builtin-targets do not.
• the two tests about which -Ctarget-cpu value takes precendence in metadata and llvm-ir are combined into tests/run-make/target-cpu-precedence
• to make it easier to test the values of target modifiers, I have also expanded the -Zls option to be used with target_modifiers
• the target_cpu comparison logic now uses reparse when comparing against a target's default cpu; this way, changing reparse does not have to be in sync with the comparison logic

@rustbot ready

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.