[PATCH] D139933: [InlineOrder] Add TopDownInlineOrder

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:293
+/// In this case, if we were to first make a decision on the C->D call site
+/// We would lose the ability to make separate decisions for the A->C->D and
+/// B->C->D call paths. And a final inlining graph like:
----------------
Lowercase.  `we`


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:300
+///      D
+/// Could not be generated since we have two different inlining decisions for CD
+/// With the top-down inliner, we can explore all possible inlining decisions.
----------------
Put a period `CD.`.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:302
+/// With the top-down inliner, we can explore all possible inlining decisions.
+class TopDownInlineOrder : public InlineOrder<std::pair<CallBase *, int>> {
+  using T = std::pair<CallBase *, int>;
----------------
Are you duplicating much of `InlineOrder` because you use `NodeCallCount` as cache and do not care about updating the priority while the priority queue is consumed?  I'm wondering if there is a good way for you to take what you need without duplicating the rest.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:305
+
+  // We keep track of how many times a function has been called. This is used
+  // to order call sites in a top-down fashion by selecting the call site with
----------------
Maybe "how many places" instead of "how many times".  Otherwise, we might sound like we are talking about profiling counts.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:309
+
+  // check which call sites caller has least calls
+  bool hasLowerPriority(const CallBase *L, const CallBase *R) const {
----------------
Capitalize the first letter and put a period at the end. `Check which call sites caller has least calls.`


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:311
+  bool hasLowerPriority(const CallBase *L, const CallBase *R) const {
+    int left_count = 0;
+    const auto left_I = NodeCallCount.find(L->getCaller());
----------------
Please rename this to `LeftCount`.

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

This applies to other variables in the same function.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:321
+
+    return left_count > right_count;
+  }
----------------
How do you ensure the top down inlining when you have a run of single edges with `NodeCallCount` alone?  Here, each one of `B`, `C`, `D`, etc is called from exactly one place, but there is a clear order in the call graph.

```
 A
/ \
B C
| |
D E
| |
F G
| |
H I
```



================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:337
+
+    // We keep track of how many times a function is called by other functions
+    // This count can be used to determine the top of the call graph by ordering
----------------
Put a period at the end like `functions.`.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:339
+    // This count can be used to determine the top of the call graph by ordering
+    // the heap form least calls to most calls
+    Function *Callee = CB->getCalledFunction();
----------------
Likewise.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:340-347
+    Function *Callee = CB->getCalledFunction();
+    if (NodeCallCount.find(Callee) == NodeCallCount.end()) {
+      NodeCallCount[Callee] = 0;
+    }
+    NodeCallCount[Callee]++;
+
+    Heap.push_back(CB);
----------------
How does this work?  It looks like `NodeCallCount` keeps changing while you are pushing all call sites at the beginning of the inlining pass (see `ModuleInliner.cpp:145`).  So, a call site inserted at the beginning of the population loop and another call site inserted toward the end of the population loop are based on totally different values of `NodeCallCount` even though you haven't done any inlining at all.  Unless you call `std::make_heap` at the end of the initial population loop, I am not sure if call sites are in any particular order that makes sense.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:375
+  DenseMap<CallBase *, int> InlineHistoryMap;
+  DenseMap<const Function *, int> NodeCallCount;
+};
----------------
Put some comment like:

```
// A map from a function to the number of call sites it is statically called from.
```



================
Comment at: llvm/test/Transforms/Inline/module-inliner-basic.ll:22
 ; CHECK-NOT:     call
-; CHECK:         ret
+; CHECK:         ret
----------------
Please add a new line.  The message "No newline at end of file" is a bit distracting now and when somebody adds more lines to the file and examines the diff.


================
Comment at: llvm/test/Transforms/Inline/top-down-inliner-multiple-heads.ll:33
+; Since we're inlining top-down A and B are the only valid callers
+; CHECK-NOT: Analyzing call of {{C|D|E}}... (caller:{{C|D}})
----------------
Likewise.


================
Comment at: llvm/test/Transforms/Inline/top-down-inliner-recursion.ll:33
+; CHECK: Analyzing call of {{B|C}}... (caller:{{B|C}})
+; CHECK: Analyzing call of {{B|C}}... (caller:{{B|C}})
----------------
Likewise.


================
Comment at: llvm/test/Transforms/Inline/top-down-inliner-simple.ll:34
+; Since we're inlining top-down A is the only valid caller
+; CHECK-NOT: Analyzing call of {{B|C|D|E}}... (caller:{{B|C|D}})
----------------
Likewise.


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

https://reviews.llvm.org/D139933