Page MenuHomePhabricator

[Inliner] Run always-inliner in inliner-wrapper
ClosedPublic

Authored by aeubanks on Sep 1 2020, 4:14 PM.

Details

Summary

An alwaysinline function may not get inlined in inliner-wrapper due to
the inlining order.

Previously for the following, the inliner would first inline @a() into @b(),

define void @a() {
entry:
  call void @b()
  ret void
}

define void @b() alwaysinline {
entry:
  br label %for.cond

for.cond:
  call void @a()
  br label %for.cond
}

making @b() recursive and unable to be inlined into @a(), ending at

define void @a() {
entry:
  call void @b()
  ret void
}

define void @b() alwaysinline {
entry:
  br label %for.cond

for.cond:
  call void @b()
  br label %for.cond
}

Running always-inliner first makes sure that we respect alwaysinline in more cases.

Fixes https://bugs.llvm.org/show_bug.cgi?id=46945.

Diff Detail

Event Timeline

aeubanks created this revision.Sep 1 2020, 4:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 1 2020, 4:14 PM
aeubanks requested review of this revision.Sep 1 2020, 4:14 PM

I'm not sure this is the proper way to fix this.
Will update failing tests if this lg.

What is the compile time implication?

The key issue here is handling of recursive calls. With this change, it is also possible to suppress more always_inline which can happen without this change. For instance in your example, if 'a' is also marked with always_inline, then not allowing b to be inlined into 'a' will enable 'a' to be inlined into its other callers. With this change, it is suppressed.

What is the compile time implication?

AlwaysInlinerPass looks at every function in the module and skips any without the alwaysinline function attribute, so I imagine it shouldn't be too bad in the typical case of no always_inline functions.
+nikic, is there any way to set off a job at http://llvm-compile-time-tracker.com/ with this patched in?

The key issue here is handling of recursive calls. With this change, it is also possible to suppress more always_inline which can happen without this change. For instance in your example, if 'a' is also marked with always_inline, then not allowing b to be inlined into 'a' will enable 'a' to be inlined into its other callers. With this change, it is suppressed.

As long as the always_inline call graph is a DAG this should work, otherwise always_inline can't always be respected no matter what order things are inlined, right? Taking care of the DAG always_inline call graph case seems fairly important.

The AlwaysInliner does not operate on DAG, but depend on function order in the module, so it is possible to regress with the change. I don't have strong opinion on this, but you want want to hear others opinion.

Compile times look unaffected: http://llvm-compile-time-tracker.com/?config=O3&stat=instructions&remote=aeubanks (thanks nikic!)

The AlwaysInliner does not operate on DAG, but depend on function order in the module, so it is possible to regress with the change. I don't have strong opinion on this, but you want want to hear others opinion.

I don't mean that the pass is a CGSCC pass, I mean that this change makes any modules with non-mutually recursive always_inline functions work as intended, whereas before that didn't always work.
Mutually recursive always_inline functions can't work, so we shouldn't care about that too much.

Who else should I ask about this?

One major problem with this patch is that the AlwaysInliner is dummy, it is not aware of profile information thus won't be able to perform proper profile update after the inlining of always inline callees. This can be an issue for Frontend PGO or when thinLTO/LTO is on (where there is cross module calls to always inline functions -- though it is not common).

Any suggestions on how to fix this? Add an alwaysinline phase in InlinerPass that runs before the rest of inlining?

I am ok adding an internal option to control this with it being default for now.

Thanks!
I discovered that some tests regarding optimization remarks were failing since AlwaysInlinerPass didn't emit optimization remarks, separated that out into https://reviews.llvm.org/D88067.

aeubanks updated this revision to Diff 293339.Sep 21 2020, 10:12 PM

add option to disable adding AlwaysInlinerPass

davidxl added inline comments.Sep 24 2020, 11:19 PM
llvm/lib/Transforms/IPO/Inliner.cpp
98

this should be true by default for now.

aeubanks added inline comments.Sep 24 2020, 11:31 PM
llvm/lib/Transforms/IPO/Inliner.cpp
98

Is there any reason to delay flipping this? Else it won't have any effect being behind a flag since nobody will turn it on. Unless you'd like the flip to be in a separate change?

The reason is that the always inliner is still not in-par with regular inliner -- if you look at the IFI setup -- missing BFI information.

I see, I'll work on that.
(I'm not familiar with the intricacies of the inliner, so any pointers like that to remaining blockers are appreciated).

Are there any more blockers for this?

Wait a few days in case others have comments.

aeubanks edited reviewers, added: rnk; removed: nikic.Oct 15 2020, 11:25 AM
rnk accepted this revision.Oct 22 2020, 3:30 PM

lgtm, I think David is waiting for input from others, and does not object.

This revision is now accepted and ready to land.Oct 22 2020, 3:30 PM
davidxl accepted this revision.Oct 22 2020, 3:32 PM

lgtm

This revision was automatically updated to reflect the committed changes.

Any idea why we have the AlwaysInliner as a separate codebase from the 'normal' inliner? I was considering adding a mode to the normal inliner that only performs mandatory inlines. The benefits would be:

  • simplify codebase (one less pass)
  • plays correctly with InlineAdvisor, allowing interested implementations to keep track of inline effects
  • address limitations of the AlwaysInliner - see AlwaysInliner.h comments (lines 23 onward)
  • simplify the problem to the 'normal' inliner, as we can (and should) run passes like DCE

I realize there's a concern re. compilation time, don't have answers at the moment.

I also wonder if davidxl's concerns about BFI, etc, would be more easily addressable having this proposed model.

Any pushback to exploring this, am I missing something?

Thanks!

That would be awesome.
One thing is that AlwaysInliner is a required pass (http://llvm.org/docs/WritingAnLLVMNewPMPass.html#required-passes), meaning it'll run even in the case that something like opt-bisect tells it to be skipped. That's because we need to respect alwaysinline semantics. But if we make InlinerPass required, it'll run even when it's not supposed to. If we could make it a separate pass but use the same exact infra but with a different InlineAdvisor(?) that would work for me. So something like factoring out InlinerPass::run.

That would be awesome.
One thing is that AlwaysInliner is a required pass (http://llvm.org/docs/WritingAnLLVMNewPMPass.html#required-passes), meaning it'll run even in the case that something like opt-bisect tells it to be skipped. That's because we need to respect alwaysinline semantics. But if we make InlinerPass required, it'll run even when it's not supposed to. If we could make it a separate pass but use the same exact infra but with a different InlineAdvisor(?) that would work for me. So something like factoring out InlinerPass::run.

Ack - let me get together a patch then next week - just wanted to first see if there's a fundamental pushback. Thanks!

That would be awesome.
One thing is that AlwaysInliner is a required pass (http://llvm.org/docs/WritingAnLLVMNewPMPass.html#required-passes), meaning it'll run even in the case that something like opt-bisect tells it to be skipped. That's because we need to respect alwaysinline semantics. But if we make InlinerPass required, it'll run even when it's not supposed to. If we could make it a separate pass but use the same exact infra but with a different InlineAdvisor(?) that would work for me. So something like factoring out InlinerPass::run.

Ack - let me get together a patch then next week - just wanted to first see if there's a fundamental pushback. Thanks!

https://reviews.llvm.org/D91446 looks like a bug that would be obsolete with your proposal.

mtrofin added a comment.EditedSat, Nov 14, 5:01 PM

That would be awesome.
One thing is that AlwaysInliner is a required pass (http://llvm.org/docs/WritingAnLLVMNewPMPass.html#required-passes), meaning it'll run even in the case that something like opt-bisect tells it to be skipped. That's because we need to respect alwaysinline semantics. But if we make InlinerPass required, it'll run even when it's not supposed to. If we could make it a separate pass but use the same exact infra but with a different InlineAdvisor(?) that would work for me. So something like factoring out InlinerPass::run.

Ack - let me get together a patch then next week - just wanted to first see if there's a fundamental pushback. Thanks!

https://reviews.llvm.org/D91446 looks like a bug that would be obsolete with your proposal.

Ugh - got sidetracked, sorry. I'll get back to this.

Created D91567