Remove unsound target_feature_inline_always feature

[Jamesbarford]

View all comments

Summary

• Remove target_feature_inline_always
• Update stdarch generators to only use #[inline] & regenerate stdarch.

Why?

Succinctly; the feature relies on LLVMs AlwaysInlinerPass() running before LLVMs heuristic based inliner pass. Which is not a basis for sound code.

This has been discussed in the tracking issue.

If the ordering of the passes were to change, of which they have in the past, it is very possible we could inline functions across callsites with mismatching target features leading to unsound code. Checks proposed in; #155426 would only take into account caller -> callee which is not enough to guard against possibly of generating unsound code if the pass ordering were to change.

There doesn't seem to be a way, presently, this this mechanism to provide soundness guarantees nor does it seem like AlwaysInlinerPass() is a desired feature of LLVM, which this feature relies on.

r? @RalfJung

[rustbot]

stdarch is developed in its own repository. If possible, consider making this change to rust-lang/stdarch instead.

cc @Amanieu, @folkertdev, @sayantn

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

[rustbot]

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

[folkertdev]

That's unfortunate, but makes sense.

One thing I'm worried about here is terrible merge conflicts with rust-lang/stdarch when we perform the next sync, and the changes not being tested by CI here. The actual changes to the generators are pretty small, so would it work to have that as its own PR in rust-lang/stdarch and then sync it over to here?

[RalfJung]

Something went very wrong here, I assume? 😰

[RalfJung]

Compiler will error on #[inline(always)] + #[target_feature(enable = "<...>")].

We already do error for that, so what are you changing here?

So this amounts to "just" removing the target_feature_inline_always feature? I'd recommend first removing its main users, in particular stdarch. That makes the final removal PR much more manageable. stdarch has a separate repo so this also has the benefit that the change can land there and be covered by their CI.

Also Cc @davidtwco

[folkertdev]

No that's what happens when you update the generators and the inline annotation on most generated functions changes. Those changes are uninteresting but really inflate the line count.

[Jamesbarford]

So this amounts to "just" removing the target_feature_inline_always feature?

Yes 👍, I'll remove that from the description

[davidtwco]

I had suggested to @Jamesbarford that we remove the uses in stdarch in this PR just to avoid the hassle of having to wait for syncs to happen before being able to make the change in rustc, but I don't feel too strongly, if we'd prefer to land the change in stdarch first and then land this after a sync, happy to do it that way too.

[folkertdev]

We're not in a rush here I think, and the sync with stdarch is easy enough (not all submodules are that easy to sync) so I'd prefer having simpler PRs and fewer merge conflicts in this case. You can add me as a reviewer on the PR over there, and we can probably have this synced in 24 to 48 hours.

[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.

[RalfJung]

So the stdarch changes have landed and synced, and just the rustc changes remain? Sorry, I didn't realize this was waiting for me.

@rustbot author

View changes since this review

[RalfJung]

I don't know this function so I have no idea if your changes here make sense.
It seems like before #145932 this didn't have tcx at all. Is it possible to exactly revert the changes from that PR, or would that be a bad idea?

Cc @nikic @dianqk for help with reviewing the codegen_llvm changes.

[Jamesbarford]

Happy to revert if you feel it best.

However fn inline_attr(...) as it stands in this PR cleans up the body of fn llfn_attrs_from_instance(...) to be as follows for adding the inline attribute;

if let Some(instance) = instance
    && let Some(inline_attr) = inline_attr(cx, tcx, instance)
{
    to_add.push(inline_attr);
}

It's quite nice the logic is self contained in the fn inline_attr(...) function, this also seems to follow the pattern for the other attributes being added.

[Jamesbarford]

405214d I've reverted as best I can. Due to other code changes unrelated to #[inline(always)] + #[target_feature(enable = "...")] it has rendered a 1:1 reversal implausible.

[RalfJung]

Okay, so someone will have to actually understand the change to this function then to tell whether this is a clean revert. Without digging into this code (which would take some time as I have no context for it) I can't tell why these lines of code got moved around. I hope one of the people I pinged above can take a look as they might already have this context. (Or maybe @saethlin does.)

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.