This is an archive of the discontinued LLVM Phabricator instance.

[NewPM][Inliner] Move mandatory inliner inside function simplification pipeline
AbandonedPublic

Authored by aeubanks on Jan 15 2021, 10:19 AM.

Details

Reviewers
mtrofin

Diff Detail

Event Timeline

aeubanks created this revision.Jan 15 2021, 10:19 AM
aeubanks requested review of this revision.Jan 15 2021, 10:19 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 15 2021, 10:19 AM

Wondering if this makes sense to you (before I go and finish cleaning up all the tests)

I think InlineAdvisor::getAdvice needs to take a bool MandatoryOnly, which Inliner then passes. This allows us to then use the same advisor for both the always case and the policy-driven inliner instances, which allows that advisor to correctly book-keep any module wide state it may want to.

llvm/include/llvm/Transforms/IPO/Inliner.h
111

Nit: const bool

llvm/lib/Transforms/IPO/Inliner.cpp
661

This should move to line 675: if we have installed an advisor, we use it, and the inliner pass passes to getAdvice() whether it only needs mandatory advice. All advisors need to support the mandatory case anyway.

aeubanks added inline comments.Jan 15 2021, 10:51 AM
llvm/lib/Transforms/IPO/Inliner.cpp
661

The whole point of https://bugs.llvm.org/show_bug.cgi?id=46945 was that we need to run the alwaysinliner first. Even if all advisors support the mandatory case, they may inline in the wrong order. That can either be fixed by running alwaysinliner first, or by having the inliner look at alwaysinline calls first.

If we have an advisor like the ML one, we will always use that one, then potentially end up inlining in the wrong order.

llvm/lib/Transforms/IPO/Inliner.cpp