Feat - #[inline(always)] + #[target_feature(enable = "....")] #2

[Jamesbarford]

This is broadly a resurrection of #[inline(always)] + #[target_feature(enable = "...")].

• Applies alwaysinline on the function definition and the callsite. (though I point out below that I'm not sure why we are applying inlining attributes to both function definitions and callsites).
• version < LLVM23 downgrades to an inline hint.
• Remove the check for using target features + inline always
• I'm not sure why we are applying inline hints at both the callsite; compiler/rustc_codegen_llvm/src/builder.rs and the function definition; compiler/rustc_codegen_llvm/src/attributes.rs. I've just left it as it is.

This should now be sound thanks to

• [InlineCost] Never inline functions with incompatible target features llvm/llvm-project#205113
• [AlwaysInliner] Do not inline on attribute mismatches llvm/llvm-project#209345

Therefore, it should be okay to just allow this combination. But we can also have a period of unstable experimentation if t-lang prefers.

r? @RalfJung

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[RalfJung]

I am not available for reviews at the moment, sorry.
r? @nikic

[RalfJung]

Given LLVM is the validator it seems defensible to remove any Rust-side validation. Thus I removed the code for checking mismatching target features.

Which code are you referring to here?

[Jamesbarford]

Given LLVM is the validator it seems defensible to remove any Rust-side validation. Thus I removed the code for checking mismatching target features.

Which code are you referring to here?

I was referring to previous attempts at this such as; #155426 Where I made Rust-side error messages.

[RalfJung]

But you're not removing any checks that currently exist in the compiler, right? (Except for the ban on inline(always) + target_features of course.)

[Jamesbarford]

But you're not removing any checks that currently exist in the compiler, right? (Except for the ban on inline(always) + target_features of course.)

Yes 👍, I've made those removals another commit along with removing the tests bedba2e

[RalfJung]

But you're not removing any checks that currently exist in the compiler, right? (Except for the ban on inline(always) + target_features of course.)

Yes 👍, I've made those removals another commit along with removing the tests bedba2e

I was asking about the original PR description. You now have changed the PR in response to my other comment, right?

That's fine but very confusing the way you are talking about it.

---

I see no reason to have this as an unstable feature. This was only ever working around LLVM bugs anyway (or, LLVM choices we weren't happy with, depending on how you want to put it). Now that LLVM is fixed we can rip off the bandaid. Let's nominate for t-lang then since this allows more code to compile -- are you happy with allowing inline(always) + target_feature, provided that we are reasonably sure that this is sound to do now? Or would you like to see some period of unstable experimentation first?

@rustbot label +I-lang-nominated
Cc @rust-lang/opsem

[saethlin]

• m not sure why we are applying inline hints at both the callsite; compiler/rustc_codegen_llvm/src/builder.rs and the function definition; compiler/rustc_codegen_llvm/src/attributes.rs. I've just left it as it is.

I never got around to investigating this, but I did confirm that codegen changes somewhere if you remove the call site attributes. So yes it's goofy but I think someone needs to understand what our call site attributes are doing before we make the change.

[Jamesbarford]

But you're not removing any checks that currently exist in the compiler, right? (Except for the ban on inline(always) + target_features of course.)

Yes 👍, I've made those removals another commit along with removing the tests bedba2e

I was asking about the original PR description. You now have changed the PR in response to my other comment, right?

That's fine but very confusing the way you are talking about it.

I see no reason to have this as an unstable feature. This was only ever working around LLVM bugs anyway (or, LLVM choices we weren't happy with, depending on how you want to put it). Now that LLVM is fixed we can rip off the bandaid. Let's nominate for t-lang then since this allows more code to compile -- are you happy with allowing inline(always) + target_feature, provided that we are reasonably sure that this is sound to do now? Or would you like to see some period of unstable experimentation first?

@rustbot label +I-lang-nominated Cc @rust-lang/opsem

I originally changed it to remove the tracking issue. However I can see it's somewhat nebulous as to what I'm referring to so I have changed it again so I'm only mentioning this PR.

[RalfJung]

Removed features never get removed from this file. Please undo the change here.

View changes since the review

[RalfJung]

Removed lints never get removed from this file. Please undo the change here. You may want to update the justification string, but you have to keep the register_removed.

View changes since the review