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

[Kazu Hirata via Phabricator via llvm-commits]

kazu 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; }
 
----------------
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`.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:744
+  // the desirability of the front call site decreases, an updated one would be
+  // inserted right back into the queue. To keep simple, those cases where
+  // the desirability of a call site increases are ignored here.
----------------
`For simplicity`



================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:752
+      const int CurrentGoodness = evaluate(CB);
+      Changed = PreviousGoodness < CurrentGoodness;
+      if (Changed) {
----------------
I'd say:

```
Changed = isMoreDesirable(PreviousGoodness, CurrentGoodness);
```

This way, we don't express the comparison operator `<` at multiple places, making it easier to try other cost metrics.


================
Comment at: llvm/lib/Transforms/IPO/Inliner.cpp:851
   std::unique_ptr<InlineOrder<std::pair<CallBase *, int>>> Calls;
-  if (InlineEnablePriorityOrder)
+  if (!InlineEnablePriorityOrder)
     Calls = std::make_unique<PriorityInlineOrder>();
----------------
Do you really mean to put `!` here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104654