[PATCH] D139933: [InlineOrder] Add TopDownInlineOrder

[IBricchi via Phabricator via llvm-commits]

IBricchi added inline comments.


================
Comment at: llvm/lib/Analysis/InlineOrder.cpp:321
+
+    return left_count > right_count;
+  }
----------------
kazu wrote:
> 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
> ```
> 
You make a good point, I forgot to decrease the call count when we pop the Caller node. This should be addressed now.

I've provided a more indepth explanation as a response to one of your later comments.


================
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);
----------------
kazu wrote:
> 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.
The idea is basically that we only ever consider callers that appear the least number of times in our "NodeCallCount" however as we pop calls we also remove them from our "NodeCallCount" so in the above example.


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

If we decide not to inline A->B or A->C, once poped we would effectively end up counts that represent a call graph like:


```
B C
| |
D E
| |
F G
| |
H I
```

Which would then consider B->D or C->E, and so on...

If the calls do get inlined we would end up with counts for a similar call graph:


```
AB AC
|  |
D  E
|  |
F  G
|  |
H  I
```

The bulk of the call sites an the heap aren't necessarily in a particular order, only the top of the heap, but since it's only ever accessed by popping the top is the only value that really needs to be in the right place.


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

https://reviews.llvm.org/D139933