[PATCH] D143624: Inlining: Run the legacy AlwaysInliner before the regular inliner.

Wed May 3 12:28:31 PDT 2023

nikic added a comment.

In D143624#4316357 <https://reviews.llvm.org/D143624#4316357>, @aeubanks wrote:

> In D143624#4315508 <https://reviews.llvm.org/D143624#4315508>, @nikic wrote:
>
>> In D143624#4315468 <https://reviews.llvm.org/D143624#4315468>, @dmgreen wrote:
>>
>>> It looks like there is quite a lot more optimization that happens to the function being always-inlined (__SSAT) before this change. Through multiple rounds of instcombine, almost to the end of the pass pipeline. The new version runs a lot less before inlining, only running instcombine->simplifycfg and not seeing another instcombine to clean up the results. Is that because the AlwaysInlinePass is a module pass and it now only runs the passes up to that point?
>>
>> Yes, which is why I personally think this change isn't a good idea. This essentially breaks our invariant that functions get simplified before they are inlined. This significantly alters the way alwaysinline functions will be optimized relative to normally inlined functions.
>
> That invariant shouldn't matter if we're not using heuristics to inline. The normal heuristic-based inliner will still work on simplified callees, but now with the additional benefit of seeing the state of an SCC where there may be alwaysinline calls after the inlinings that must happen having happened.

I agree in the sense that for alwaysinling the simplification doesn't matter for cost modelling purposes. However, it can still have other benefits. One is that we don't repeat unnecessary work. Any simplification we do before inlining doesn't have to be repeated for every (potentially transitive) caller it gets inlined into. The other (and in a way, opposite) is that we simplify the function before inlining and then again the callee after inlining, which may paper over phase ordering issues (or the problem discussed above, which is kind of in the same category). Of course, this is not what this is intended for and we should endeavor to fix such issues -- but the practical outcome is still that you'll probably get worse optimization if you use alwaysinline vs inline after this change, which seems kind of unintuitive.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143624/new/

https://reviews.llvm.org/D143624