[PATCH] D104654: [llvm][Inliner] Make PriorityInlineOrder lazily updated

[Liqiang Tao via Phabricator via llvm-commits]

taolq marked 4 inline comments as done.
taolq added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:732
 
   static bool cmp(const T &P1, const T &P2) { return P1.second > P2.second; }
 
----------------
kazu wrote:
> Could we split this function into two like so?
> 
> ```
> // Return true if S1 is more desirable than S2.
> static bool isMoreDesirable(int S1, int S2) { return S1 < S2; }
> 
> static bool cmp(const T &P1, const T &P2) { return isMoreDesirable(P2.second, P1.second); }
> ```
> 
> The reason I am suggesting this is that the current implementation of `cmp` encapsulates two things -- the order of arguments dictated by `std::make_heap` and our logic about which one is more desirable. 
> 
> By hiding `>` inside `isMoreDesirable`, the reader doesn't have to worry about whether this metric is "the higher the better" or "the lower the better".
> 
> See below for an additional use of `isMoreDesirable`.
This refactoring is sexy. I love it! Thanks a lot.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104654/new/

https://reviews.llvm.org/D104654