Pass through of target features to llvm-bitcode-linker and handling them

[kulst]

When using the llvm-bitcode-linker (linker-flavor=llbc) target-features are not passed through and are not handled by it.
The llvm-bitcode-linker is mainly used as a self contained linker to link llvm bitcode for the nvptx64 target. It uses llvm-link, opt and llc internally. To produce a .ptx file of a specific ptx-version it is necessary to pass the version to llc with the --mattr option. Without explicitly setting it, the emitted .ptx-version is the minimum supported version of the --target-cpu.

I would like to be able to explicitly set the ptx version as some llvm problems only occur in earlier .ptx-versions.

Therefore this pull request adds support for passing target features to llvm-bitcode-linker and handling them.
I was not quite sure if adding these features to rustc_target/src/target_features.rs is necessary or not. If so I will gladly add these.

r? @kjetilkjeka 

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Noratrieb (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[workingjubilee]

r? @kjetilkjeka

but I don't think it will work 🤔

[rustbot]

Failed to set assignee to kjetilkjeka: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

[kjetilkjeka]

Thanks for wanting to fix this bug in the llbm-bitcode-linker.

Therefore this pull request adds support for passing target features to llvm-bitcode-linker and handling them.
I was not quite sure if adding these features to rustc_target/src/target_features.rs is necessary or not. If so I will gladly add these.

The features should be added in rustc_target/src/target_features.rs but it doesn't have to be done in this PR. It's sufficient that this PR fixes the behavior of llvm-bitcode-linker and then a different PR can add target_features for nvptx.

After this PR is merged it's already possible to use +ptx85 as a feature but a warning will be emited

warning: unknown and unstable feature specified for `-Ctarget-feature`: `ptx85`
  |
  = note: it is still passed through to the codegen backend, but use of this feature might be unsound and the behavior of this feature can change in the future
  = help: consider filing a feature request

By adding the features to rustc_target/src/target_features.rs it's possible to avoid the warning and reason about incompatible and implied features (I haven't tested the llvm behavior but assume that if +ptx85 is specified then either +ptx78 should be incompatible or implied).

[kulst]

Thanks for the feedback.

I would prefer to add these features to rustc_target/src/target_features.rs in a different PR.

The behavior of llc that I could observe during my tests is the highest ptx version specified wins. The logic in llvm is generated by tablegen and looks like this:

if (Bits[NVPTX::PTX32] && PTXVersion < 32) PTXVersion = 32;
if (Bits[NVPTX::PTX40] && PTXVersion < 40) PTXVersion = 40;
// ...
if (Bits[NVPTX::PTX87] && PTXVersion < 87) PTXVersion = 87;

So -mattr=+ptx85,+ptx78 produces ptx85.

[kulst]

I changed the implementation so that the existing global_backend_features query is used. This query calls global_llvm_features which to my knowledge is the current logic used to collect the target features that get passed to llvm.
To make this possible it is neccessary to query before link stage, as the TyCtxt is no longer present there. Therefore an additional field is added for the CrateInfo type to persist the target features until link stage.

Other options would have been:

1. Using sess.target_features: These are collected by target_features_cfg which does only incorporate features known to rustc. This would contradict with the current behavior of rustc to pass also target_features to llvm unknown to rustc.
2. Duplicating the logic that is used by either of the two mentioned functions: As we already have two functions that use a similar logic this did not seem to be the right solution to me.

[kulst]

I slightly changed the implementation. rustc_codegen_ssa acts now as a provider for the default global_backend_features query. This ensures the query is always registered even if a CodegenBackend does not register it by its provide method.
I am now quite happy with the implementation and am looking forward getting additional feedback.

[kjetilkjeka]

It looks like you found the right place to hook onto the target_feature.

It seems to me like CrateInfo is an old name and since it was merged with LinkerInfo it has also contained fields like target_cpu. Based on that I agree that it's also appropriate to include the target_features 👍

I'm not an approved reviewer for rustc, but it looks great from my perspective. If I could I would have r+'ed.

For the follow up, where you add the ptx features. Also add a couple of tests where we check that using features like ptx85 and/or ptx75 gives the correct behavior in ptx. I see that there's currently no tests for global_llvm_features and a comment that express a bit uncertainty about the exact precedence.

Thanks a lot for contributing to the llvm-bitcode-linker and also for help improving the experience of using the nvptx in Rust. I greatly look forward to unlock everything that comes with the debug symbols. ❤️

[kulst]

Thanks again for the review and the positive feedback!
I will take your comments into consideration for the follow up.
I wasn't sure if you are allowed to officially review rustc pull requests as an nvptx target maintainer. But since I was interested in your feedback anyway, I decided to give it a try.

[Noratrieb]

r? bjorn3 I think you're the best person to judge the approach from a compiler side

[bjorn3]

@bors r+

[bors]

📌 Commit 831d9f3 has been approved by bjorn3

It is now in the queue for this repository.