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

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added a comment.

In D143624#4116171 <https://reviews.llvm.org/D143624#4116171>, @mtrofin wrote:

> In D143624#4115986 <https://reviews.llvm.org/D143624#4115986>, @aeubanks wrote:
>
>> __clang_hip_math.hip is annoying...
>>
>> We'll need to remove the `MandatoryFirst` inliner in `ModuleInlinerWrapperPass`, although not sure if @mtrofin has any issues with that or not
>
> IIRC we had at a point a mandatory , whole-module pass. The idea wast that let's not have N inliners, and that AlwaysInliner had some limitations, and for similar reasons @aemerson  pointed out, it'd make sense to first perform the mandatory inlines (D91567 <https://reviews.llvm.org/D91567>). In D94825 <https://reviews.llvm.org/D94825> we went away from that. I don't remember why. @aeubanks? (it's referencing a patch you started)
>
>> This isn't quite what I had initially thought, but this might be better. (I was thinking that we sort the calls in the inliner to visit alwaysinline calls first, but that might cause more compile time issues since we have to update the call graph after visiting all the calls in a function, but we might be visiting every function twice if we first batch process the alwaysinline calls then all other calls)

Without knowing that there were inliner explosion issues, I still think it makes more sense to not visit all functions twice. e.g. it helps with cache locality when compiling if you don't visit functions on two separate walks of the call graph.

But if this solves the issue then I think this patch is good. Getting rid of the mandatory inline advisor is a nice cleanup.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143624