-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
Convert -Ctarget-cpu into a target-modifier for AVR, AMDGCN and NVPTX
#150732
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
base: main
Are you sure you want to change the base?
Conversation
-Ctarget-cpu a target-modifier on NVPTX
|
@rustbot label: +O-NVPTX |
-Ctarget-cpu a target-modifier on NVPTX-Ctarget-cpu into a target-modifier on NVPTX
-Ctarget-cpu into a target-modifier on NVPTX-Ctarget-cpu into a target-modifier for NVPTX
7164067 to
a8f5e96
Compare
|
Some changes occurred in src/doc/rustc/src/platform-support cc @Noratrieb These commits modify compiler targets. |
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
r? @bjorn3 as you suggested this change. |
|
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 |
|
I can't vouch for the implementation, but the approach sounds good. :) Cc @Darksonn |
|
I have added some explanation of why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| pub(super) fn target_cpu( | ||
| sess: &Session, | ||
| l: &TargetModifier, | ||
| r: Option<&TargetModifier>, | ||
| ) -> bool { | ||
| if sess.target.cpu_is_target_modifier { | ||
| if let Some(r) = r { | ||
| return l.extend().tech_value == r.extend().tech_value; | ||
| } else { | ||
| return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if -Ctarget-cpu appears multiple times on the command line? Please add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the very least we have to ensure that whatever values rustc actually uses and what is recorded as target modifier are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last argument taking precedence is the desired functionality:
Corresponding MCP
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Behavior as per MCP is fine. Whichever value ends up being used should be tracked and verified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is checked now by tests/run-make/target-cpu-is-target-modifier/rmake.rs
Does that match what you had in mind?
tests/ui/target_modifiers/incompatible_target_cpu.error_generated.stderr
Outdated
Show resolved
Hide resolved
a8f5e96 to
e5cbf49
Compare
|
Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter cc @rust-lang/miri, @RalfJung, @oli-obk, @lcnr The reflection data structures are tied exactly to the implementation cc @oli-obk
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 |
This comment has been minimized.
This comment has been minimized.
e5cbf49 to
7a07c8d
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
7a07c8d to
7333203
Compare
-Ctarget-cpu into a target-modifier for NVPTX-Ctarget-cpu into a target-modifier for AVR, AMDGCN and NVPTX
This comment has been minimized.
This comment has been minimized.
7333203 to
c9bc79b
Compare
|
The run-make-support library was changed cc @jieyouxu |
|
Thanks for the reviews so far! I have updated the PR and its description. I hope that I was able to address all mentioned concerns:
I split the changes into 4 commits which I hope makes them easier to review. When updating at one point I mistakenly included commits that didn't belong to this PR. So some wrong labels had been added: |
This comment has been minimized.
This comment has been minimized.
This commit renames the `need_explicit_cpu` flag in the target specification into `requires_explicit_and_consistent_cpu`. This flag is now additionally enabled for NVPTX. Crates built for AVR, AMDGCN and NVPTX that specify different values for `-Ctarget-cpu` cannot soundly be linked together. Therefore this commit converts `-Ctarget-cpu` into a target-modifier. However, the agreement about it is only enforced when compiling for targets where `requires_explicit_and_consistent_cpu` is set. This makes `-Ctarget-cpu` effectively a target-modifier only for AVR, AMDGCN and NVPTX.
- Boolean target modifiers are now mentioned without a trailing `=` in the messages. - Wording improved for unset target modifiers.
c9bc79b to
ba01dc2
Compare
|
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. |
This comment has been minimized.
This comment has been minimized.
- added `-Ctarget-cpu` flags - removed `core`-dependency where possible and added `minicore` where necessary - reenabled some tests
ba01dc2 to
5cbc2a1
Compare
Crates built for AVR, AMDGCN and NVPTX that specify different values for
-Ctarget-cpucannot be soundly linked together. Thereforeneed_explicit_cpuwas already set in their target specifications (NVPTX has the same issue, but conversely this flag was never set for it).This PR attempts to make
rustcensure that no crates with disagreeing values for-Ctarget-cpuare linked together by it. This is achieved by converting-Ctarget-cpuinto a target-modifier. The agreement is only enforced for the mentioned targets based on the value inneed_explicit_cpuTo express this additional requirement, the flag is renamed intorequires_explicit_and_consistent_cpu.Why should
-Ctarget-cpube a target-modifier fornvptx?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:
Crate A is a dependency of crate B. In the rustc invocation of crate B
This should now ideally create an LLVM error, because the linked bitcode contains code paths that were selected under
sm_70assumptions but the final NVPTX codegen is targetingsm_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-cpube a target-modifier foramdgcnandavr?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