Page MenuHomePhabricator

[Inliner] Inline alwaysinline calls first
AbandonedPublic

Authored by aeubanks on Wed, Jan 13, 6:58 PM.

Details

Summary

And revert much of D91567.
D91567 ended up putting the mandatory inlining pass in a different CGSCC
pipeline than the function simplification pipeline, causing alwaysinline
functions to not be optimized when inlined into another function,
causing PR48734. Much of the design of D91567 was to potentially support
future work to run the function simplification pipeline twice, once
before inlining. However, that hasn't happened yet and can be revisited
in the future.

This sorts the initial list of CallBases so that we handle alwaysinline
calls first. Since we want to process as many calls in one functions at
a time, we have to use a stable sort.

This actually doesn't handle all alwaysinline cases properly. For
example, within an SCC, if an alwaysinline function contains another
alwaysinline call and is inlined, the newly inlined alwaysinline call
will be processed after other calls since it was not in the initial list
of CallBases. Then we can end up with another case of PR46945 where an
alwaysinline function can't be inlined because a mutually recursive
function was inlined first. However, it seems rare enough to not bother
with this case.

Diff Detail

Event Timeline

aeubanks created this revision.Wed, Jan 13, 6:58 PM
aeubanks requested review of this revision.Wed, Jan 13, 6:58 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptWed, Jan 13, 6:58 PM

Would running the function simplification pipeline after the always inline pass address the issue?

Would running the function simplification pipeline after the always inline pass address the issue?

Probably yes, but last time we measured the compile time implications of that, it was very noticeable, so I'd like to avoid that at least for now.

An alternative is to run the mandatory inliner in the same CGSCC pipeline as everything else, but the way InlineAdvisorAnalysis is setup made it hard to implement

aeubanks edited reviewers, added: dmgreen; removed: greened.Wed, Jan 13, 9:49 PM

An alternative is to run the mandatory inliner in the same CGSCC pipeline as everything else, but the way InlineAdvisorAnalysis is setup made it hard to implement

I don't remember there being a particular challenge with the advisor - it was more that there was the other value of having a more accurate 'view' of the overall shape of the functions, if we want to include more context in inlining decision making. My preference would be to have a robust solution that addresses both this and the previous problem.

So I make sure I understand it correctly: it sounds like in PR48734, the callee wasn't first optimized through the function simplification pipeline, is that correct?

Let's try the mandatory inliner in the same CGSCC - happy to do that. Is PR48734 an emergency (I can take a look at this tomorrow morning PST)

It's not an emergency.
The issue with InlineAdvisorAnalysis is that the ModuleInlineWrapperPass presets the InliningAdvisorMode of the InlineAdvisorAnalysis. We could force override it, but then what's the point of InlineAdvisorAnalysis? Can't we just create an InlineAdvisor in the pass itself rather than using a wrapper like InlineAdvisorAnalysis?

It's not an emergency.
The issue with InlineAdvisorAnalysis is that the ModuleInlineWrapperPass presets the InliningAdvisorMode of the InlineAdvisorAnalysis. We could force override it, but then what's the point of InlineAdvisorAnalysis? Can't we just create an InlineAdvisor in the pass itself rather than using a wrapper like InlineAdvisorAnalysis?

We need to have the advisor module-wide, because its goal is to track module-wide stats, which we leverage in the ML advisor.