This is an archive of the discontinued LLVM Phabricator instance.

Added ThinLTO inlining statistics
ClosedPublic

Authored by Prazek on Jul 18 2016, 4:56 PM.

Details

Summary

copypasta doc of InlinerStats class
/ \brief Calculate and dump ThinLTO specific inliner stats.
/ The main statistics are:
/ (1) Number of inlined imported functions,
/ (2) Number of imported functions inlined into importing module (indirect),
/ (3) Number of non imported functions inlined into importing module (indirect).
/ The difference between first and the second is that first stat counts
/ all performed inlines on imported functions, but the second one only the
/ functions that have been eventually inlined to a function in the importing
/ module (by a chain of inlines). Because llvm uses bottom-up inliner, it is
/ possible to e.g. import function A, B and then inline B to A,
/ and after this A might be too big to be inlined into some other function
/ that calls it. It calculates this statistic by building graph, where
/ the nodes are functions, and edges are performed inlines and then by marking
/ the edges starting from not imported function.
/
/ If Verbose is set to true, then it also dumps statistics
/ per each inlined function, sorted by the greatest inlines count like
/ - number of performed inlines
/// - number of performed inlines to importing module

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
tejohnson added inline comments.Jul 19 2016, 7:14 AM
include/llvm/Transforms/IPO/InlinerStats.h
12 ↗(On Diff #64429)

Perhaps instead of referring to these as a "real inline" it would be clearer to say "inline to importing module" or something more descriptive like that.

Prazek added inline comments.Jul 19 2016, 8:44 AM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

indeed. I am gonna change that comment to be more clear

55 ↗(On Diff #64429)

because I am moving all the pairs from map to vector, so either I will do a copy of everything or I will move all the stuff

59 ↗(On Diff #64429)

Yes, but this label splits private functions from private members. It is not necessary, but the code is more readable this way.
I also think I saw usages like this in clang.

lib/Transforms/IPO/Inliner.cpp
99

exactly, the fields are set to null after inlining

lib/Transforms/IPO/InlinerStats.cpp
17 ↗(On Diff #64429)

Ok so there are two things

  1. It is true that the function might be added here multiple times. I can solve it easily by sorting the vector and then by adding +n to the counter in DFS, where n is how many times Fun have been pushed.
  1. I think walking all the functions would be much slower. I don't expect this list to be as big as all the functions. probably 5-25% in thinlto. So rather than iterating all the functions, and having some boolean to distinguish if I should start DFS from this or not, I can do the option (1) which I think is better.
tejohnson added inline comments.Jul 19 2016, 9:44 AM
lib/Transforms/IPO/InlinerStats.cpp
17 ↗(On Diff #64429)

How about use a set instead of a list then. It avoids the need to sort.

I'm not sure on why you need a count of how many times it was pushed (inlined into)? Since you keep appending the functions inlined into each function here in its InlinedFunctions list, why do you need to invoke dfs() on the caller each time?

Prazek added inline comments.Jul 19 2016, 12:14 PM
lib/Transforms/IPO/InlinerStats.cpp
17 ↗(On Diff #64429)

yes, that is true, I was wrong. I will just sort it and unique it. It will be much faster than using set.

eraman added inline comments.Jul 19 2016, 1:28 PM
include/llvm/Transforms/IPO/InlinerStats.h
1 ↗(On Diff #64426)

Transforms/Utils is a better place to have these two files. I will also rename it to ThinLTOInliner stats because it is mostly specific to ThinLTO. Actually, you can get rid of the header file and declare getInlinerStatistics in, say, InlinerPass.h and move the class to the .cpp file.

10 ↗(On Diff #64426)

"It calculates and dumps the following stats" is more readable

16 ↗(On Diff #64429)

"inlined to not imported function" -> "eventually inlined to a function in the importing module"

19 ↗(On Diff #64429)

"the real values" -> "this statistic"

lib/Transforms/IPO/InlinerStats.cpp
12 ↗(On Diff #64429)

Naming the arguments Caller and Callee makes it consistent with the terminology used in the inliner

72 ↗(On Diff #64429)

I think the graph can have cycle. Consider a mutual recursion A->B->A. Let's say B gets inlined into A. This would normally result in a self-recursion (A->A). This would prevent A from being inlined into B. But, if we are in a situation where B first gets inlined into A and the A->A call is DCEd. Then A could be inlined into B causing a cycle in your InlineGraph.

73 ↗(On Diff #64429)

Let's say A and B are local functions (functions in the importing module) and C is an imported function and there is a call chain A->B->C. Let's say C first gets inlined into B and then B gets inlined into A. You'll have nodes for A, B and C and A and B are in the NonExternalFunctions. You'll dfs from both A and B. NumberofRealInlines of B will be 2 which not what you want.

Prazek added inline comments.Jul 19 2016, 1:47 PM
include/llvm/Transforms/IPO/InlinerStats.h
2 ↗(On Diff #64429)

actually not right now, because I call member functions.
I may change it to have addInlineStatistic and dumpStats functions that would call the members.

If this is good and nobody will disagree then I can change it like that.

lib/Transforms/IPO/InlinerStats.cpp
73 ↗(On Diff #64429)

yes, I am fixing it right now.

mehdi_amini added inline comments.Jul 19 2016, 2:26 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

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?

Prazek added inline comments.Jul 19 2016, 2:49 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

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.

mehdi_amini added inline comments.Jul 19 2016, 2:54 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

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.

Prazek added inline comments.Jul 19 2016, 3:14 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

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

mehdi_amini added inline comments.Jul 19 2016, 3:20 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

Thanks for clarifying.

So, according to you example, it seems that we'll count the number of "real" inline for an imported function only once (I.e. at the boundary between non-imported and imported).

Because in the final optimized binary, we would codegen X and A and D that both contains C (and X contains two copies of C), so 4, but the counter for C is only 2.

Also what is X is an imported function?
What if X is now also inlined in a non-imported function?

tejohnson added inline comments.Jul 19 2016, 3:45 PM
lib/Transforms/IPO/InlinerStats.cpp
51 ↗(On Diff #64429)

s/not external/not imported/

Also, it would be useful to know here whether the caller that we are inlining into is imported or not.

61 ↗(On Diff #64429)

Should be trivial I think to compute the reverse as well: how many imported functions were not inlined into a non-imported function (possibly via other inlines).

tejohnson added inline comments.Jul 19 2016, 3:52 PM
include/llvm/Transforms/IPO/InlinerStats.h
22 ↗(On Diff #64429)

I think it is more important to simply flag whether >0 of the inlines are "real", perhaps we don't really want to try to count # of real inlines, or it gets tricky to define compared to the total number of inlines as shown above.

Prazek marked 9 inline comments as done.Jul 20 2016, 2:53 PM
Prazek added inline comments.
lib/Transforms/IPO/InlinerStats.cpp
17 ↗(On Diff #64429)

I changed the name to NonImportedCallers

Prazek marked 4 inline comments as done and an inline comment as not done.Jul 20 2016, 4:43 PM
Prazek added inline comments.
lib/Transforms/IPO/InlinerStats.cpp
51 ↗(On Diff #64429)

But we can have multiple callers. I think the real inlines gives you the information that you need.

61 ↗(On Diff #64429)

I am not sure if I should compute it here. The number of all imported functions is counted in importer.

72 ↗(On Diff #64429)

fixed.

Prazek updated this revision to Diff 64801.Jul 20 2016, 5:58 PM
Prazek marked 3 inline comments as done.
Prazek edited edge metadata.
  • fixes
  • fixes
  • fixes

I will do the refactor that Easwaran mentioned tomorrow.

tejohnson added inline comments.Jul 20 2016, 9:12 PM
include/llvm/Transforms/IPO/InlinerStats.h
43 ↗(On Diff #64801)

Maybe "Number of inlines into non imported function (possibly indirect via intermediate inlines)"

lib/Transforms/IPO/Inliner.cpp
99

Maybe then change comment to say "Callee and Caller will be set to null in CS after inlining"

lib/Transforms/IPO/InlinerStats.cpp
22 ↗(On Diff #64801)

s/form/from/

24 ↗(On Diff #64801)

Even with imported functions, by not putting intra-module inlines in the graph it should reduce the size and the traversal overhead. Although I would expect that most intra module inlines would have been done already in the compile step. Maybe just note that in your comment.

52 ↗(On Diff #64801)

Oh got it, I missed the fact that we only have the callee info here and aren't walking from the caller.

57 ↗(On Diff #64801)

I would remove this early exit as it makes assumptions about the sorting done, and if the sorting algorithm changes this would need to change too. Maybe just continue so we don't get a message under EnableListStats below when there are no inlines.

70 ↗(On Diff #64801)

s/inlined imported functions to/imported functions inlined into/

72 ↗(On Diff #64801)

s/inlined not imported functions to/non-imported functions inlined into/

Prazek added inline comments.Jul 20 2016, 10:02 PM
lib/Transforms/IPO/InlinerStats.cpp
24 ↗(On Diff #64801)

yea, but if you just pass those flags to see the statistics for non thinlto then it is useless to compute it by graph. I gonnna mention it.

Prazek marked 11 inline comments as done.Jul 21 2016, 12:21 PM
Prazek marked 2 inline comments as done.Jul 21 2016, 3:24 PM
Prazek added inline comments.
include/llvm/Transforms/IPO/InlinerStats.h
23 ↗(On Diff #64801)

Ok right now it works exactly like Teresa says. Is it all clear Mehdi?

Prazek updated this revision to Diff 64976.Jul 21 2016, 3:24 PM
  • fixes
  • fixes
  • fixes
  • other fixes
  • options
  • Other fixes
Prazek updated this revision to Diff 64978.Jul 21 2016, 3:25 PM
  • Other fixes
eraman added inline comments.Jul 21 2016, 4:36 PM
include/llvm/Transforms/Utils/InlinerStats.h
2

As discussed offline, incorporate ThinLTO to the name or at the minimum add comments to indicate that the primary intent is to use with ThinLTO. Perhaps rename it to InlinerFunctionImportStats.h - as used in the option name.

17

Nit: I prefer recordInlining or similar small name, but will leave it to you

lib/Transforms/IPO/Inliner.cpp
61

use the string "Basic" instead of an empty string

99

You can not call getCallee() or getCalledFunction() because the call instruction does not exist after inlining and those methods access methods of the call instruction.

As an aside, this comment is not specific to this patch and typically could just add the comment separately (which doesn't need any review). I don't mind it being part of this patch though.

lib/Transforms/Utils/InlinerStats.cpp
26

typo: imporrted -> imported

32

merge the next line here

39

Comment out of sync with the code

56

I don't expect the number to exceed max value of int16_t in real programs, but perhaps possible in generated test cases. Why not make it int32_t ?

194

As discussed offline, this is not threadsafe

Prazek updated this revision to Diff 64998.Jul 21 2016, 5:05 PM
Prazek marked 11 inline comments as done.
  • Last fix?
lib/Transforms/Utils/InlinerStats.cpp
56

sure

Prazek updated this revision to Diff 64999.Jul 21 2016, 5:06 PM
  • Last fix?
tejohnson added inline comments.Jul 22 2016, 9:42 AM
include/llvm/Transforms/Utils/ThinLTOInlinerStats.h
1 ↗(On Diff #64999)

"Generating inliner statistics for imported functions"

16 ↗(On Diff #64999)

doxygen comment

17 ↗(On Diff #64999)

s/InlinerStatistics/ThinLTOInlinerStatistics/ (suggested below)

lib/Transforms/IPO/InlinerStats.cpp
61 ↗(On Diff #64429)

Yes but then the client has to find both stats and subtract rather than get the stat immediately. I think it would be useful to walk over the functions here, and simply count how many have the imported metadata tag, then emit that info here, along with the percentage of imported funcs that were inlined into the importing module and that weren't.

lib/Transforms/Utils/ThinLTOInlinerStats.cpp
1 ↗(On Diff #64999)

"Generating inliner statistics for imported functions" is clearer. If too long, probably clarify further down in the file head comment block.

26 ↗(On Diff #64999)

Here and in below line, I think it is clearer to restructure like "Number of imported functions inlined into...".

42 ↗(On Diff #64999)

s/InlinerStatistics/ThinLTOInlinerStatistics/

91 ↗(On Diff #64999)

What is the multithreaded case we are concerned about? Is it when the ThinLTO backend threads are launched in parallel via a ThreadPool? In that case we want each thread to have a completely independent set of stats, which I don't think this mutex provides.

Prazek marked 5 inline comments as done.Jul 22 2016, 11:41 AM
Prazek added inline comments.
lib/Transforms/Utils/ThinLTOInlinerStats.cpp
26 ↗(On Diff #64999)

wtf I was pretty sure I changed that.

91 ↗(On Diff #64999)

I am not sure if there is any problem right now, but we also probably don't want to risk breaking it in the future. The overhead should not be very big and it won't be runned frequently.

Prazek updated this revision to Diff 65117.Jul 22 2016, 11:45 AM
  • Last fix?
mehdi_amini edited edge metadata.Jul 22 2016, 12:10 PM

I prefer the middle end to stay as much agnostic of the exact "use case" (ThinLTO) and focus on the "feature" from a library point of view: so I'd rather see "functionimport" used instead of "ThinLTO".
I.e. any system that would use some importing mechanism to implement a runtime in a JIT environment may be interested in this. We design LLVM as reusable and independent components.

lib/Transforms/Utils/ThinLTOInlinerStats.cpp
92 ↗(On Diff #65117)

We don't add mutexes without motivation. You need to explain why *this* one makes sense (I.e. how it could get use in a multi-threaded environment in the future).

Prazek added inline comments.Jul 22 2016, 12:58 PM
lib/Transforms/Utils/ThinLTOInlinerStats.cpp
92 ↗(On Diff #65117)

ok so we discussed it offline, and the main problem here is that multiple ThinLTO threads would use it. The solution is to create new InlinerStatistic object in Inliner and remove mutexes.

mehdi_amini added inline comments.Jul 22 2016, 1:26 PM
lib/Transforms/Utils/ThinLTOInlinerStats.cpp
203 ↗(On Diff #65117)

Global mutable state is seldom the right choice, I assume this is what you're gonna move to be state in the inliner?

Prazek added inline comments.Jul 22 2016, 1:54 PM
lib/Transforms/Utils/ThinLTOInlinerStats.cpp
203 ↗(On Diff #65117)

Exactly

Prazek updated this object.Jul 22 2016, 2:01 PM
Prazek edited edge metadata.
Prazek updated this revision to Diff 65198.Jul 22 2016, 5:42 PM
  • Refactor
Prazek updated this revision to Diff 65199.Jul 22 2016, 5:46 PM
    • Refactor
  • other small changes

Looks pretty close. Just some minor comments below.

lib/Transforms/IPO/Inliner.cpp
595

This indentation doesn't look right to me - make sure you clang format.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
9 ↗(On Diff #65199)

s/usefull/useful/

50 ↗(On Diff #65199)

Add empty line above.

131 ↗(On Diff #65199)

suggest adding "anywhere" at the end to distinguish from below stat.

136 ↗(On Diff #65199)

Make this consistent with other lines, e.g. maybe "non-imported functions inlined anywhere".

test/Transforms/Inline/inline_stats.ll
18

It isn't clear what the percentages represent (e.g. some are percent of total # functions and some are percent of inlined functions). Probably need to add a tag - e.g.:

; CHECK: inlined functions: 5 [50% of all functions]
; CHECK: imported functions inlined: 4 [57.14% of imported functions]
; CHECK: imported functions inlined into importing module: 3 [75% of inlined imported functions]
; CHECK: Inlined not imported functions: 1 [33.33% of non-imported functions]
; CHECK: non-imported functions inlined into importing module: 1 [100% of non-imported inlined functions]

However, after typing that out, I think for the stat relating to how many are inlined into the importing module, it would be much more useful to have the percentage in terms of the total number of that type of function. E.g. instead of expressing the "imported functions inlined into importing module" as a percentage of "imported functions inlined", it would be more useful to have it be a percentage of "imported functions". Ditto for non-imported functions inlined into importing module.

tejohnson added inline comments.Jul 25 2016, 9:53 AM
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
9 ↗(On Diff #65199)

s/usefull/useful/

tejohnson added inline comments.Jul 25 2016, 10:02 AM
test/Transforms/Inline/inline_stats.ll
18

Could you add one more stat - the # of imported functions not inlined into importing module (along with percentage of all imported functions)? I know it is computable from the other stats, but it would be useful to be able to get this directly.

Prazek updated this revision to Diff 65393.Jul 25 2016, 11:47 AM
Prazek marked 9 inline comments as done.
  • Changed out
Prazek updated this revision to Diff 65395.Jul 25 2016, 11:48 AM
  • Changed out

LGTM, other than one missing stat I suggested adding below. Please wait for Easwaran's remaining comments though.

test/Transforms/Inline/inline_stats.ll
18

Could you add one more stat - the # of imported functions not inlined into importing module (along with percentage of all imported functions)? I know it is computable from the other stats, but it would be useful to be able to get this directly.

Please add this one.

Prazek added inline comments.Jul 25 2016, 12:41 PM
lib/Transforms/IPO/Inliner.cpp
595

This was actually formatted with clang-format. I guess the arguments are not in the same column because it would look the same as the next arguments to function.

test/Transforms/Inline/inline_stats.ll
18

I am not sure if it is good way, but I will change it. What I am woried is that the last stat won't tell much at the fist glance, because the number will be low

19

you mean substract imported functions from inlined imported functions?

like
imported functions not inlined anywhere: 3 [~43% of imported functions]

tejohnson added inline comments.Jul 25 2016, 12:49 PM
test/Transforms/Inline/inline_stats.ll
19

Meaning subtract "imported functions inlined into importing module" from "imported functions" (4 in the above example).

19

I'm less sure about how we will use the last one (non-imported functions inlined into importing module vs non-imported functions inlined anywhere), but for the imported functions what we primarily want to see is how many/what percentage were inlined into the importing module.

eraman edited edge metadata.Jul 25 2016, 1:00 PM

Generally looks good. Most of the comments are minor. Please add a test case for the non-verbose case.

lib/Transforms/IPO/Inliner.cpp
66

"Inliner stats for imported functions" or something like that. Since the option name does not have ThinLTO, remove it from the description.

99

To reiterate, the problem is that the call instruction does not exist and calling any methods on that CallInst accesses freed memory. CallSite does not explicitly store Caller and Callee and so there is no question of "Callee and Caller will be set to null".

lib/Transforms/Utils/CMakeLists.txt
19

Indentation.

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
106 ↗(On Diff #65395)

Node.second.NumberOfInlines is > 0 here, so you don't need the (Node.second.NumberOfInlines > 0) * 1. You could simply increment the count by 1.

108 ↗(On Diff #65395)

No need to do * 1

24 ↗(On Diff #65199)

Inliner contains an object of ImportedFunctionsInliningStatistics. By default you are not generating statistics and yet this will call reserve unnecessarily. Not a big deal - we are talking about <2KB per module compilation here, but still unnecessary.

test/Transforms/Inline/inline_stats.ll
2

This doesn't test the non-verbose case. Just use this test case and use a different filecheck prefix.

Prazek marked 4 inline comments as done.Jul 25 2016, 4:47 PM
Prazek added inline comments.
lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
106 ↗(On Diff #65395)

I know about this, I wanted to make it more explicit. The implicit conversion from bool is bugprone, so I will just add int() here so
int(Node.second.NumberOfInlines > 0)

test/Transforms/Inline/inline_stats.ll
19

Is this line ok?

imported functions inlined into importing module: 3 [42.86% of imported functions], left: 4 [57.14% of imported functions]

Prazek updated this revision to Diff 65443.Jul 25 2016, 5:12 PM
Prazek marked 4 inline comments as done.
Prazek edited edge metadata.
  • Changed out
Prazek updated this revision to Diff 65444.Jul 25 2016, 5:12 PM
  • Changed out
Prazek marked 4 inline comments as done.Jul 25 2016, 5:13 PM
Prazek updated this revision to Diff 65450.Jul 25 2016, 5:19 PM
Prazek marked an inline comment as done.
  • Changed out
tejohnson added inline comments.Jul 25 2016, 5:20 PM
test/Transforms/Inline/inline_stats.ll
20

Sure, or "remaining", or "not inlined" to be explicit

tejohnson added inline comments.Jul 26 2016, 9:21 AM
lib/Transforms/IPO/Inliner.cpp
103

New comment doesn't make sense. I think you missed a word or two? Maybe:
"Storing because getCalledFunction() and getCaller() are not accessible after inlining, due to call instruction being eliminated."

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
104 ↗(On Diff #65450)

As Easwaran mentioned, due to the earlier check/continue, you are guaranteed that NumberOfInlines > 0. So simply change this to a ++.

108 ↗(On Diff #65450)

Ditto.

139 ↗(On Diff #65450)

s/NewLinefalse/LineEnd/

Prazek updated this revision to Diff 65581.Jul 26 2016, 1:18 PM
Prazek marked 9 inline comments as done.
  • Changed out
Prazek updated this revision to Diff 65584.Jul 26 2016, 1:22 PM
  • Changed out
tejohnson accepted this revision.Jul 26 2016, 1:23 PM
tejohnson edited edge metadata.

LGTM! Just wait for Easwaran to double check

This revision is now accepted and ready to land.Jul 26 2016, 1:23 PM
Prazek updated this revision to Diff 65642.Jul 26 2016, 6:37 PM
Prazek edited edge metadata.
  • Fixed bugs, still something wrong
Prazek updated this revision to Diff 65797.Jul 27 2016, 1:25 PM

Final update

Couple minor comments on the comments, but otherwise looks ok. I'm not convinced the complexity of the new handling in recordInlineto avoid the extra map lookup is necessary since this is just stat counting code not on by default though, but I don't have a strong objection to it either.

include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
89 ↗(On Diff #65797)

Not so much because iterator is invalidated (although an iterator being invalidated would be a side effect of the same issue) - it is because the DenseMap value may be moved, and you need to store pointers to the InlineGraphNodes elsewhere. IIUC that is. You could just change this to "Unique pointer to InlineGraphNode used since the node pointers are also saved in the InlinedCallees vector."

91 ↗(On Diff #65797)

Might want to note that this is ok because you don't dereference the key (using the NameMap to avoid needing to).

Couple minor comments on the comments, but otherwise looks ok. I'm not convinced the complexity of the new handling in recordInlineto avoid the extra map lookup is necessary since this is just stat counting code not on by default though, but I don't have a strong objection to it either.

I trieid different code with insert/emplace etc, and it was even uglier. The 2 step indirection is the bad guy here, but at least 1 lookup and 1 allocation is the good guy

eraman added inline comments.Jul 27 2016, 2:21 PM
include/llvm/Transforms/IPO/InlinerPass.h
73 ↗(On Diff #65797)

Formatting - the empty line has been removed after this.

include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
89 ↗(On Diff #65797)

The comment is not very clear to me. What does the value type got to with invalidation?

97 ↗(On Diff #65797)

I'm missing something here. Why not make the function name the key to the other map and remove this NameMap? This will change NonImportedCallers as well...

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
30 ↗(On Diff #65797)

I find this quite difficult to read. I prefer using if(...) here instead of ternary operator. Also, you don't need to make ValueLookup a pointer to mapped_type. Keeping it a reference will remove one * operator's use.

Also add a comment saying that the second lookup could result in the resizing map and that's why you can't just keep a reference to the uniquq_ptr returned by the lookup.

Prazek updated this revision to Diff 65816.Jul 27 2016, 3:22 PM
Prazek marked 4 inline comments as done.
  • fix
Prazek marked an inline comment as done.Jul 27 2016, 3:24 PM
Prazek added inline comments.
lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
31 ↗(On Diff #65816)

you were right. It's funny that I didn't thought about the solution that is right now. It is even better becase the functionname and Imported is set only once

Prazek marked an inline comment as done.Jul 27 2016, 3:24 PM
Prazek added inline comments.
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
90 ↗(On Diff #65816)

Is is more clear right now? (line 93)

Like the new handling for inserting the Nodes, looks much cleaner.

include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
63 ↗(On Diff #65821)

Instead, I like Easwaran's suggestion to make NodesMap a StringMap.

Then NonImportedCallers should be a vector of strings. Note that StringMap stores a copy of the key when you insert, so you could store the resulting StringRef (returned when the StringMap entry is inserted) in the NonImportedCallers vector instead of a std::string copy.

It is cleaner than using keys that are pointers to possibly deleted memory.

95 ↗(On Diff #65821)

s/Ff/If/

lib/Transforms/Utils/ImportedFunctionsInliningStatistics.cpp
61 ↗(On Diff #65821)

Good catch. Call this from the ImportedFunctionsInliningStatistics constructor and save the results there instead.

Prazek added inline comments.Jul 27 2016, 4:38 PM
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
63 ↗(On Diff #65821)

I talked with Easwaran and I don't think there is a point changit the map. It will be much slower and won't get any new information, and it will take a time to change everything again.

tejohnson added inline comments.Jul 27 2016, 4:46 PM
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
63 ↗(On Diff #65821)

It's an issue of robustness rather than getting more information. I mentioned earlier too that it seems brittle to use pointers to deleted functions as keys - the code may not try to dereference them now, but someone else may come along and try to do something with the key, dereferencing the pointer to get some additional info, not realizing that it is possibly deallocated.

If Easwaran is ok with the code as is, then I'm ok with this for now, but something to clean up later. Please add a FIXME?

tejohnson added inline comments.Jul 27 2016, 5:08 PM
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
63 ↗(On Diff #65821)

I mentioned earlier too that it seems brittle to use pointers to deleted functions as keys

Sorry, it appears I did not raise that in the review (think I just asked about it in our chat). In any case, I'd prefer taking a bit more time and making it more robust to avoid the type of future error I described, but I am ok with doing that as a follow-on.

Prazek marked 2 inline comments as done.Jul 27 2016, 5:24 PM
Prazek added inline comments.Jul 27 2016, 5:26 PM
include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
64 ↗(On Diff #65845)

I will fix it in next patch. I want to push it right now because I don't want to deal with conflicts later.

Ok, lgtm with two minor nits.

include/llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h
72 ↗(On Diff #65845)

doxygen comment

100 ↗(On Diff #65845)

extra space inadvertently added "i nvariant"?

Prazek marked 6 inline comments as done.Jul 28 2016, 5:14 PM
Prazek updated this revision to Diff 66064.Jul 28 2016, 5:33 PM

fixed bug

This revision was automatically updated to reflect the committed changes.
yaxunl added a subscriber: yaxunl.Jul 29 2016, 7:36 AM

Hi,

This commit caused build failure in Visual Studio 2013:

55>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted
55>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted
59> CGOpenMPRuntime.cpp
58> clangBasic.vcxproj -> C:\p4\stg_win60\git-llvm-tot\build\Debug\lib\clangBasic.lib
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineSimple.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineSimple.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineAlways.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineAlways.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\Inliner.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\Inliner.cpp)

Hi,

This commit caused build failure in Visual Studio 2013:

55>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted
55>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted
59> CGOpenMPRuntime.cpp
58> clangBasic.vcxproj -> C:\p4\stg_win60\git-llvm-tot\build\Debug\lib\clangBasic.lib
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineSimple.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineSimple.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineAlways.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\InlineAlways.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(51): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::InlineGraphNode(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\Inliner.cpp)
56>C:\p4\stg_win60\git-llvm-tot\llvm\include\llvm/Transforms/Utils/ImportedFunctionsInliningStatistics.h(52): error C2610: 'llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &llvm::ImportedFunctionsInliningStatistics::InlineGraphNode::operator =(llvm::ImportedFunctionsInliningStatistics::InlineGraphNode &&)' : is not a special member function which can be defaulted (C:\p4\stg_win60\git-llvm-tot\llvm\lib\Transforms\IPO\Inliner.cpp)

fixed long time a go just for record.