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

[Jamesbarford]

View all comments

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

[rustbot]

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.

[Jamesbarford]

@bors try @rust-timer queue

[rust-bors]

☀️ Try build successful (CI)
Build commit: 93fa2b5 (93fa2b547ea8b60d7d246505f4379269b43cc06d)
Base parent: c999cef (c999cef531ea9059e189e82fe0e82c5daf249bc9)

[rust-timer]

Finished benchmarking commit (93fa2b5): comparison URL.

Overall result: ✅ improvements - no action needed

Benchmarking means the PR may be perf-sensitive. Consider adding rollup=never if this change is not fit for rolling up.

@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

Our most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.3%	[-0.4%, -0.3%]	7
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results (primary -2.5%, secondary -0.4%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.3%	[3.3%, 3.3%]	2
Improvements ✅ (primary)	-2.5%	[-2.8%, -2.2%]	2
Improvements ✅ (secondary)	-2.3%	[-3.1%, -1.8%]	4
All ❌✅ (primary)	-2.5%	[-2.8%, -2.2%]	2

Cycles

Results (primary 1.8%, secondary -0.3%)

A less reliable metric. May be of interest, but not used to determine the overall result above.

	mean	range	count
Regressions ❌ (primary)	1.8%	[1.8%, 1.8%]	1
Regressions ❌ (secondary)	1.9%	[1.9%, 1.9%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.5%	[-2.5%, -2.5%]	1
All ❌✅ (primary)	1.8%	[1.8%, 1.8%]	1

Binary size

This perf run didn't have relevant results for this metric.

Bootstrap: 500.132s -> 495.548s (-0.92%)
Artifact size: 406.81 MiB -> 406.90 MiB (0.02%)