This is an archive of the discontinued LLVM Phabricator instance.

[llvm][Inline] Add a module level inliner
ClosedPublic

Authored by taolq on Jul 21 2021, 7:54 AM.

Details

Summary

Add module level inliner, which is a minimum viable product at this point.
Also add some tests for it.

The module inliner does inlining in module level, which is a more general level
than SCC level. The main benefit is that the inliner order could be released.

With this module inliner, the inline order is not limited to bottom-up order, which
is limited in SCC inliner. Also, we could evaluate more globally scope inline order heuristics.
It's possible to explore the improvement with different inline orders with this module inliner.

RFC: https://lists.llvm.org/pipermail/llvm-dev/2021-August/152297.html

Diff Detail

Event Timeline

taolq created this revision.Jul 21 2021, 7:54 AM
taolq requested review of this revision.Jul 21 2021, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2021, 7:54 AM
taolq retitled this revision from add module inliner to [WIP] [llvm][Inline] Add module level inliner.Jul 21 2021, 8:03 AM
taolq edited the summary of this revision. (Show Details)
taolq added reviewers: kazu, mtrofin, teemperor.

This could use some more words to explain what that is and how it's different from the already-existing stuff.

taolq updated this revision to Diff 361424.Jul 24 2021, 2:10 AM
  • move code to a separated file

This could use some more words to explain what that is and how it's different from the already-existing stuff.

Note that arc doesn't update review's description from the updated commit message.
Also, that seems like a lot of code duplication.

taolq updated this revision to Diff 363483.Aug 2 2021, 8:12 AM
  • add module inliner to opt pipeline
  • TODO: remove duplicated code
  • TODO: add more description
kazu added a comment.Aug 4 2021, 4:45 PM

This patch looks good to me in general. The huge amount of duplicate code is a concern, but I am not too worried as long as it's all contained in ModuleInliner.cpp.

I'd like to see the clang-tidy messages addressed before you check this in. By the way, I think you can safely ignore the one about erase_if.

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

I think you can remove this line.

This patch still doesn't explain what it does and why it does what it does.
Links to previous mails about this will be helpful.

taolq retitled this revision from [WIP] [llvm][Inline] Add module level inliner to [llvm][Inline] Add a module level inliner.Aug 9 2021, 8:33 AM
taolq edited the summary of this revision. (Show Details)
taolq updated this revision to Diff 365193.Aug 9 2021, 8:34 AM
  • fix bugs
  • add more description
  • fix bugs
  • add more description

I think my question is: why is the code worth it? what's the short-term plan? what's the long-term plan?
Having *so* much intentional code duplication seems counter-intuitive.

taolq edited the summary of this revision. (Show Details)Aug 9 2021, 8:39 AM
taolq added a comment.Aug 9 2021, 8:45 AM
  • fix bugs
  • add more description

I think my question is: why is the code worth it? what's the short-term plan? what's the long-term plan?
Having *so* much intentional code duplication seems counter-intuitive.

Thanks for your reviews.
I wonder whether the updated summary makes sense?
If not, I would like to add more explanation.

taolq updated this revision to Diff 366007.EditedAug 12 2021, 8:46 AM
  • remove duplicate code: InlineOrder
lebedev.ri requested changes to this revision.Aug 12 2021, 9:49 AM

The error is likely on my side, but i'm still not quite seeing it.
I don't see the point in asking yet again, so i would like to instead see an RFC at llvm-dev list.

This revision now requires changes to proceed.Aug 12 2021, 9:49 AM
taolq edited the summary of this revision. (Show Details)Aug 27 2021, 5:27 PM
taolq added a comment.Aug 27 2021, 5:40 PM

The error is likely on my side, but i'm still not quite seeing it.
I don't see the point in asking yet again, so i would like to instead see an RFC at llvm-dev list.

FYI, let me put the RFC here, https://lists.llvm.org/pipermail/llvm-dev/2021-August/152297.html
How about this one? Looking forward to your comments.

taolq added a comment.Sep 10 2021, 2:53 AM

The error is likely on my side, but i'm still not quite seeing it.
I don't see the point in asking yet again, so i would like to instead see an RFC at llvm-dev list.

FYI, let me put the RFC here, https://lists.llvm.org/pipermail/llvm-dev/2021-August/152297.html
How about this one? Looking forward to your comments.

This is just a friendly reminder for the RFC and this revision.
Please let me know if there still is anything confusing.

taolq updated this revision to Diff 380390.Oct 18 2021, 7:24 AM
  • rebase to the newest main branch
taolq updated this revision to Diff 380666.Oct 19 2021, 6:01 AM
  • fix bug
kazu added inline comments.Oct 21 2021, 9:14 PM
llvm/lib/Passes/PassBuilder.cpp
112

You should be able to remove this line if you are not otherwise changing PassBuilder.cpp at all.

llvm/lib/Passes/PassBuilderPipelines.cpp
683–684 ↗(On Diff #380666)

Why do you need to do this? Performance reasons? Correctness?

llvm/lib/Transforms/IPO/Inliner.cpp
1024–1029

This is a strange place to add your module inliner. ModuleInlinerWrapperPass performs:

for each SCC in the bottom-up order:
  Call the inliner on the SCC
  Run cleanup passes on the SCC

Now, if UseModuleInliner is true, we don't add InlinerPass (see a few lines below), so the loop above essentially becomes:

for each SCC in the bottom-up order:
  Run cleanup passes on the SCC

If we are not inlining functions in the "for each" loop, we don't need to visit each SCC in the bottom-up order.

You might consider adding your module inliner just before:

MPM.addPass(buildInlinerPipeline(Level, Phase));

at PassBuilderPipelines.cpp:891 with an appropriate guard like if (UseModuleInliner).

This way, you don't need to touch the existing inliner at all.

taolq added inline comments.Oct 22 2021, 5:36 AM
llvm/lib/Passes/PassBuilder.cpp
112

I changed PassRegistry.def, here is a compilation error without this line in that file.

llvm/lib/Passes/PassBuilderPipelines.cpp
683–684 ↗(On Diff #380666)

For performance and simplicity.
Deferral logic is necessary for bottom-up inlining, while unnecessary in priority-based inlining. So I decided to disable this logic.

taolq updated this revision to Diff 381550.Oct 22 2021, 7:50 AM
  • refactor to avoid touching the existing inliner
taolq updated this revision to Diff 381551.Oct 22 2021, 7:54 AM
  • remove comment in code
taolq updated this revision to Diff 381552.Oct 22 2021, 7:57 AM
  • remove comment in code
kazu added a comment.Oct 25 2021, 11:59 AM

Your patch is looking good. Would you mind addressing mostly cosmetic comments? Thanks!

llvm/lib/Passes/PassBuilderPipelines.cpp
895–906 ↗(On Diff #381552)

May I suggest you to put this in a function so that you can do something similar to the else clause just a couple of lines below?

MPM.addPass(buildInlinerPipeline(Level, Phase));

Maybe you can create a function named buildModuleInlinerPipeline or something.

This way, the details specific to the module inliner don't clutter buildModuleSimplificationPipeline as much.

903–904 ↗(On Diff #381552)

Does your module inliner honor EnableDeferral at all? If so, and there is a good reason to disable it, please add an appropriate comment.

In any event, you should be able to remove if (EnableModuleInliner) because we are already inside if (EnableModuleInliner) above.

llvm/lib/Transforms/IPO/ModuleInliner.cpp
361

Please add a new line at the end.

kazu added inline comments.Oct 25 2021, 2:53 PM
llvm/lib/Transforms/IPO/ModuleInliner.cpp
136

I just tried to build llvm with your patch. It looks like you need one more argument for IAA. Please rebase your patch. Thanks!

taolq updated this revision to Diff 383365.Oct 29 2021, 8:31 AM
  • clean code
  • rebase to main branch
taolq marked 2 inline comments as done.Oct 29 2021, 8:32 AM
taolq updated this revision to Diff 383368.Oct 29 2021, 8:34 AM
  • clean code
taolq marked an inline comment as done.Oct 29 2021, 6:05 PM
kazu accepted this revision.Nov 2 2021, 2:02 PM

Thank you for updating the patch. The replay machinery changed since you uploaded your latest version. Please see inline comments about that.

LGTM with the replay logic updated. Thanks a lot for patiently updating multiple iterations!

llvm/lib/Transforms/IPO/ModuleInliner.cpp
63–80

I suggest you to remove the replay inliner logic, which is for debugging purposes.

The declaration of ModuleInlineReplayScope doesn't compile because of recent changes to the replay machinery. You can forget about keeping up with the machinery simply by removing the replay logic here.

119–123

I suggest you to drop this.

148–149

Replace the two replay parameters with {} like so:

if (!IAA.tryCreate(Params, Mode, {})) {

The default-constructed instance has an empty string for its ReplayFile field and thus disables replay.

taolq marked 3 inline comments as done.Nov 8 2021, 2:06 AM
taolq updated this revision to Diff 385428.Nov 8 2021, 2:09 AM
  • remove inliner replay logic
kazu accepted this revision.Nov 8 2021, 12:44 PM

Thank you for updating the patch. LGTM.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 8 2021, 7:06 PM
This revision was automatically updated to reflect the committed changes.

I'm surprised the patch landed as-is without more traction or even more recent activity on the RFC.
Also I don't see here a resolution from @lebedev.ri who marked an objection to this revision?

I'm surprised the patch landed as-is without more traction or even more recent activity on the RFC.
Also I don't see here a resolution from @lebedev.ri who marked an objection to this revision?

I simply just don't understand what the high-level endgoal here is.

kazu added a comment.Nov 15 2021, 9:21 PM

I'm surprised the patch landed as-is without more traction or even more recent activity on the RFC.
Also I don't see here a resolution from @lebedev.ri who marked an objection to this revision?

The patch is intended to be a testbed for anybody interested in playing with a priority-based inliner (as opposed to the bottom-up inliner). With the module inliner in place, we can try different priority functions without having to write an entire inliner from scratch.