This is an archive of the discontinued LLVM Phabricator instance.

[llvm][ModuleInliner] Refactor InlineSizePriority and PriorityInlineOrder
ClosedPublic

Authored by taolq on May 24 2022, 7:15 AM.

Details

Summary

This patch introduces the abstract base class InlinePriority to serve as
the comparison function for the priority queue. A derived class, such
as SizePriority, may choose to cache the priorities for different
functions for performance reasons.

This design shields the type used for the priority away from classes
outside InlinePriority and classes derived from it. In turn,
PriorityInlineOrder no longer needs to be a template class.

Diff Detail

Event Timeline

taolq created this revision.May 24 2022, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 7:15 AM
taolq requested review of this revision.May 24 2022, 7:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2022, 7:15 AM
taolq retitled this revision from refactor InlinePriority to [llvm][Inline] Refactor InlinePriority.May 24 2022, 7:17 AM
taolq edited the summary of this revision. (Show Details)
taolq added reviewers: kazu, mtrofin.
kazu added a comment.May 25 2022, 1:11 PM

May I suggest expanding the summary? You could say something like:

ModuleInliner] Refactor InlineSizePriority and PriorityInlineOrder

This patch introduces abstract base class InlinePriority to serve as
the comparison function for the priority queue.  A derived class, such
as SizePriority, may choose to cache the priorities for different
functions for performance reasons.

This design shields the type used for the priority away from classes
outside InlinePriority and classes derived from it.  In turn,
PriorityInlineOrder no longer needs to be a template class.

Note that you need to click "Edit Revision" to update the summary as Phabricator does not automatically update the summary on arc diff.

llvm/include/llvm/Analysis/InlineOrder.h
106–108

I'm a bit worried about the fact that we are hashing CB three times here. Could we do something like this?

auto It = Priorities.find(CB);
const auto OldPriority = *It;
*It = evaluate(CB);
const auto NewPriority = *It;
taolq updated this revision to Diff 432305.May 26 2022, 9:04 AM
  • use iterator to decrease rehashing
taolq marked an inline comment as done.May 26 2022, 9:04 AM
taolq retitled this revision from [llvm][Inline] Refactor InlinePriority to [llvm][ModuleInliner] Refactor InlineSizePriority and PriorityInlineOrder.
taolq edited the summary of this revision. (Show Details)
tschuett added a subscriber: tschuett.EditedMay 27 2022, 8:42 AM

The name of the function is compare, but the comment reads more like isLessThan.

For compare, I would expect -1, 0, or +1.

taolq updated this revision to Diff 432782.May 29 2022, 6:28 AM
  • rename compare to hasLowerPriority
kazu accepted this revision.Jun 1 2022, 12:28 PM

LGTM. Thank you for updating the patch!

This revision is now accepted and ready to land.Jun 1 2022, 12:28 PM
taolq updated this revision to Diff 433721.Jun 2 2022, 5:24 AM
  • rebase to main
This revision was landed with ongoing or failed builds.Jun 2 2022, 7:29 AM
This revision was automatically updated to reflect the committed changes.

Compiler now complains:

warning: 'llvm::InlinePriority' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class InlinePriority {
      ^
llvm-project/llvm/include/llvm/Analysis/InlineOrder.h:80:7: warning: 'llvm::SizePriority' has virtual functions but non-virtual destructor [-Wnon-virtual-dtor]
class SizePriority : public InlinePriority {
      ^