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]

rustc_codegen_ssa is the logical spot for stuff shared between backends, because all backends depend on it.

That's not correct, the cranelift backend does not. The comment you cite seems to be outdated. In my understanding, rustc_codegen_ssa is a convenience wrapper that a codegen backend can choose to use, but not all do.

cranelift does not currently support target features, but it'd be a bad idea to fundamentally exclude it from using the shared target feature infrastructure in the future.

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

I agree, which is why I didn't like putting things there -- but for the reason given above, I'm not sure where else to put it.

[nnethercote]

That's not correct, the cranelift backend does not.

rust/compiler/rustc_codegen_cranelift/src/lib.rs

Line 22 in 38081f2

extern crate rustc_codegen_ssa;

[RalfJung]

The last thing I heard from @bjorn3 is that it does not use codegen_ssa. Maybe it uses just some helper types, or maybe that changed -- @bjorn3, could you clarify?

But if it imports that crate then I guess even if it doesn't use the actual SSA builder part of codegen_ssa, codegen_ssa is still a good place to put shared helpers. Good point!

[bjorn3]

cg_clif doesn't use the MIR building infrastructure or codegen scheduling code of cg_ssa, but it does use the linking code, archive builder, naked asm implementation and a bunch of other things from cg_ssa. My goal is to slowly make more and more of cg_ssa usable for cg_clif and then migrate cg_clif to use those as opposed to its own code, reducing the maintenance burden of cg_clif.

[RalfJung]

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

I did this now. Thanks for catching this!