Page MenuHomePhabricator

[llvm][Inline] Add a module level inliner
Needs RevisionPublic

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)Fri, Aug 27, 5:27 PM
taolq added a comment.Fri, Aug 27, 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.Fri, Sep 10, 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.