[PATCH] D22491: Added ThinLTO inlining statistics

[Piotr Padlewski via llvm-commits]

Prazek added inline comments.

================
Comment at: include/llvm/Transforms/IPO/InlinerStats.h:22
@@ +21,3 @@
+/// Then starting from non external functions that have some inlined calls
+/// inside, it walks to every inlined function and increment counter.
+///
----------------
mehdi_amini wrote:
> Prazek wrote:
> > mehdi_amini wrote:
> > > Prazek wrote:
> > > > tejohnson wrote:
> > > > > mehdi_amini wrote:
> > > > > > It is not clear to me why you need a graph and can't just increment counters as the inlined moves on?
> > > > > This is to distinguish between inlines that eventually make it into a non-imported function (i.e. what Piotr refers to as a "real inline" and I am suggesting something like "inline to importing module"). I.e. if we have call chain A->B->C where A was originally in the module (non-imported) and B/C were imported, when we inline C into B we don't know if they will ultimately be inlined into A. Presumably there could be another SCC that we perform inlines on in between the B->C and A->BC inlines so we can't easily track this without some side structure.
> > > > indeed. I am gonna change that comment to be more clear
> > > OK for a side structure, does it need to build a graph? I'm not sure if a map `F -> { real, unreal }` + the global counters would not suffice?
> > > 
> > > I mean, it depends on the semantics of these counters, and I'm still not sure about this. For example:
> > > 
> > > `A->B->C` and `D->B->C`, with C and B the only imported functions. If we import entirely (C into B, and B into A and D), what is expected to be computed?
> > > 
> > Then the number of inlines for B: 2, for C: 1, and number of inline to importing module (aka real inlines) will be B: 2, C: 2.
> > 
> > The graph is to calculate the number of real inlines. Without it we won't know if the function was realy inlined into some function that wasn't imported and we won't be able to see for example
> > that all functions that we have imported were never really inlined.
> Something does not line up, the description in the revision is "The difference between first and the second is that first stat counts all performed inlines, and second one only the functions that really have been inlined to some not imported function."
> 
> This sentence makes me think that the first number is always greater than the second, however you provided 1 and 2 for C. 
> 
> 
Yes, I also though so when designing it, but because the (1) only counts direct inlines (how many times this function have been inlined), and (2) counts indirect inlines to not imported function, then in example like you have shown it might happen that (2) > (1). I am fixing the code also to handle the cases like:

Adding X->A and X->D to your example, where X is also not imported function and A and D have been inlined to X should give us
(1) X: 0, A: 1, D: 1, B: 2, C: 1
(2) X: 0, A: 1, D: 1, B: 2, C: 2

Right now it would give
(2) X: 0, A: 1, D: 1, B: 3, C: 3

because I don't start only from "roots" (not imported functions that have not been inlined anywhere).
I have to think more about this and decide what metric should be better.
I have this doc that is trying to get all the things into one pice, but there might be some differences with the thing that I am trying to do right now.
https://docs.google.com/a/google.com/document/d/1ccMEfxUUbwWVr7VwDPzRgtWNy7iLDQrhBlN6XrKNIzc/edit?usp=sharing


https://reviews.llvm.org/D22491