inline(always) on no-op conversions

[joonazan]

A From::from call that does nothing should always be inlined.

Inlining the From -> Into implementation reduced code size in my
project but I'm not 100% sure that it is always good.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @hanna-kruppe (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[hanna-kruppe]

The first question I have about every inline(always) is: can't this be a plain inline?

Empirically, LLVM is very eager to inline aggressively even without being "forced", and a plain inline is usually sufficient to unlock that capability by making the definition of the function widely available. So if inline suffices, jumping straight to inline(always) has no upsides and several (marginal but non-negligible) downsides.

[joonazan]

@hanna-kruppe I tried plain inline first but inline(always) reduced binary size significantly in my application and inline didn't.

I haven't tried all four combinations but in that case we should also try the change on a representative sample of applications, not just mine.

I did get some interesting qualitative information: My application benefits a lot but it also has one perfectly normal looking case that doesn't inline even with inline(always) on both. That case doesn't inline even if I use From::from directly.

If I make the application simpler in ways that are unrelated to constructing that call, it starts to inline. At a certain amount of complexity it inlines if I add a feature to my Cargo.toml even though I don't compile with it and don't change the code.

To me it is pretty obvious that the conversion is happening as part of constructing a literal which doesn't depend on the surrounding code at all, but the inlining decision obviously doesn't see that. Actually, it might be interesting to change the code a bit. Currently it looks like this: stack.push(literal). But let literal = ...; stack.push(literal) might compile better.

[joonazan]

I tested using let, but it generated more code in one simplified version and didn't change anything in the real code.

[hanna-kruppe]

So, I'm not sure what to do about this PR. Like you said, it would be good to have a broad benchmark suite supporting this decision (even if it doesn't really make sense why this would make a difference) but we don't really have a standard one for Rust. Nor do I have the time and energy to do at least "due diligence" myself.

[bors]

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

[joonazan]

Where does the cost of inline(always) come from? I understand that it is a very bad idea if it duplicates code but it this case it just removes code. Does considering to inlining but then deciding not to also have a higher cost with inline(always)? If not, then this particular inlining is only beneficial.

[hanna-kruppe]

There's several ways in which inline(always) can have negative effects, even in apparently trivial cases (not a complete list):

Even with a trivial (forwarding) link in a call graph, forcing the decision to remove that link by inlining can have knock-on effects on the overall result of inlining e.g., due to the bottom-up behavior of inlining: with the Into::into -> From::from "forwarding" impl, the actual conversion might already be inlined from from to into, and an alwaysinline on the latter might then lead to lots of inlining that otherwise would not have happened out of code size concerns. I believe you're seeing such knock-on decisions even in your application and they just happen to be beneficial, since merely removing Into -> From forwarding and identity From impls would probably only save dozens of bytes overall. There's also other conceivable interactions, depending on surrounding code and the details of the inlining heuristic.

Then there's the impact on opt-level=0, where even inlining trivial identity functions (From<T> for T) can be a net negative because the lack of other optimizations means that the cruft duplicated by inlining (debug info metadata, stack variable, loads and stores to that variable) is not cleaned up afterwards.

And of course, in the end, essentially any change at any point of the optimization pipeline can have happen to have unfortunate interactions with later passes. This is never intended, but it happens from time to time, so it's important to evaluate optimization pipelines end-to-end and on a variety of benchmarks.

[hanna-kruppe]

knock-on effects on the overall result of inlining e.g., due to the bottom-up behavior of inlining

Here's an (artificial, pathological) example illustrating this https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=0827278dc1001539d1734d997ad1cc6b
Uncommenting the inline(always) on the wrapper function greatly increases code size significantly because big_func gets inlined into the wrapper first.

[joonazan]

Ok, so inlining the Into implementation is definitely not safe because the From implementation could already be inlined into it. So this change is a bad idea.

But it seems like inlining could be improved. Is inline(always) processed by rustc or LLVM?

[hanna-kruppe]

Primarily in LLVM (though there's work on MIR inlining and these attributes affect how codegen units are generated by rustc). In any case, if we're in agreement that this change isn't good, do you want to close the PR and perhaps file an issue for the code size difference you're seeing in your application?

[joonazan]

I think filing an issue for my application specifically wouldn't be very helpful as it is pretty much the opposite of a minimal reproduction.

I may investigate the phenomenon that inlines that would reduce code size don't always happen further, as I find it somewhat interesting. One could maybe compute how common it is in published crates or in random code if there is something like CSmith for Rust.