Extract some shared code from codegen backend target feature handling

[RalfJung]

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions in rustc_codegen_ssa.

The first two commits should be purely refactoring. I am fairly sure the LLVM-side behavior stays the same; if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not?

The third commit extracts some shared logic out of the functions that populate cfg(target_feature) and the backend target feature set, respectively. This one actually has some slight functional changes:

• Before, with -Ctarget-feature=-feat, if there is some other feature x that implies feat we would not add -x to the backend target feature set. Now, we do. This fixes Target feature implications for negative features are handled inconsistently between codegen and cfg(target_feature) #134792.
• The logic that removes x from cfg(target_feature) in this case also changed a bit, avoiding a large number of calls to the (uncached) sess.target.implied_target_features (if there were a large number of positive features listed before a negative feature) but instead constructing a full inverse implication map when encountering the first negative feature. Ideally this would be done with queries but the backend target feature logic runs before tcx so we can't use that...
• Previously, if feature "a" implied "b" and "b" was unstable, then using -Ctarget-feature=+a would also emit a warning about b. I had to remove this since when accounting for negative implications, this emits a ton of warnings in a bunch of existing tests... I assume this was unintentional anyway.

The fourth commit increases consistency of the GCC backend with the LLVM backend.

@bjorn3 I did not touch the cranelift backend here, since AFAIK it doesn't really support target features. But if you ever do, please use the new helpers. :)

Cc @workingjubilee

[rustbot]

r? @compiler-errors

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

[rustbot]

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

Some changes occurred in compiler/rustc_codegen_ssa/src/codegen_attrs.rs

cc @jdonszelmann

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

[RalfJung]

There's a lot more logic in this file that seems like it should be shared across backends (almost all of fn codegen_fn_attrs), but I shied away from the huge refactor that would have been.

[RalfJung]

Turns out properly adding negative implications to the target_features we set for LLVM tends to add a lot of target features -- we have some quite extensive implication chains. Is that a problem? Cc @nikic @taiki-e

[antoyo]

if the GCC side deliberately diverges from this then I may have missed that. I did account for one divergence, which I do not know is deliberate or not: GCC does not seem to use the -Ctarget-feature flag to populate cfg(target_feature). That seems odd, since the -Ctarget-feature flag is used to populate the return value of global_gcc_features which controls the target features actually used by GCC. @GuillaumeGomez @antoyo is there a reason target_config ignores -Ctarget-feature but global_gcc_features does not?

There is no reason for that. I probably missed some stuff when I implemented this.
I'd be happy to have the same behavior as LLVM here.

[nikic]

Turns out properly adding negative implications to the target_features we set for LLVM tends to add a lot of target features -- we have some quite extensive implication chains. Is that a problem? Cc @nikic @taiki-e

Anything in particular you're concerned about? But generally, no, it shouldn't be a problem.

[RalfJung]

I can imagine two concerns:

• So many attributes cause some sort of perf issue?
• We actually have more implications in rustc than LLVM recognizes, so now we're disabling more things than before. This is probably a bugfix but still it'd be good to know if there is such a case...

[bors]

☔ The latest upstream changes (presumably #141238) made this pull request unmergeable. Please resolve the merge conflicts.

[compiler-errors]

r? compiler

[bors]

☔ The latest upstream changes (presumably #135160) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

This is somewhat bitrotty... and to my knowledge rather outside your typical review areas, @nnethercote.

@workingjubilee @bjorn3 @nikic maybe one of you could take this?

[nnethercote]

There's a bunch of code duplication between the GCC and LLVM backends in target feature handling. This moves that into new shared helper functions.

That sounds reasonable.

I placed those in rustc_middle since I couldn't think of a better place to put them... I'm open for suggestions. :)

I think you've gotten off-track here.

rustc_codegen_ssa is the logical spot for stuff shared between backends, because all backends depend on it. The doc comment at the top of its lib.rs says //! This crate contains codegen code that is used by all codegen backends (LLVM and others).

Also, rustc_middle is already too big and shouldn't get anything added to it that doesn't need to be.

Here's part of the crate graph. You can see how rustc_codegen_llvm depends directly on rustc_codegen_ssa, while rustc_middle is several layers further down. rustc_codegen_gcc isn't included, I think because it's in a sub-repo, but it would sit next to rustc_codegen_llvm and also depend directly on rustc_codegen_ssa.

Crate graph generated with:

cargo +nightly depgraph --all-deps --dedup-transitive-deps --workspace-only > ~/graph.dot;
dot -Tpng ~/graph.dot > ~/graph.png

Based on this, I think significant parts of this PR are unnecessary:

• Anything moved from rustc_codegen_ssa to rustc_middle should be left where it is. (E.g. you moved rustc_codegen_ssa/src/target_features.rs to rustc_middle.)
• Anything moved from rustc_codegen_{llvm,gcc} to rustc_middle should instead be moved to rustc_codegen_ssa.

This will significantly reduce the size of the PR, e.g. the entire first commit would disappear, along with part of the third commit.

[RalfJung]

I added a reference to #142412 since I think RUSTC_SPECIAL_FEATURES needs to die a fiery death... but that's for a future PR. This one should be good to go then, thanks all. :)

@bors r=nnethercote,WaffleLapkin

[bors]

📌 Commit e790e54 has been approved by nnethercote,WaffleLapkin

It is now in the queue for this repository.

[workingjubilee]

Thank you for doing this cleanup, and thank you @nnethercote and @WaffleLapkin for covering the review!

[bors]

☔ The latest upstream changes (presumably #142443) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

I'm afraid this will need another review... the conflicts with #135927 were enormous. Also, that PR added some spaghetti to target_features that I found easier to remove during conflict resolution, than to incorporate into this PR. So this PR is now also a cleanup of retpoline flag handling, removing a bunch of unneeded complexity. Only the 2nd commit ("move -Ctarget-feature handling into shared code") saw major changes.