[PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Adam Nemet via cfe-commits]

anemet added inline comments.

================
Comment at: lib/CodeGen/CodeGenPGO.cpp:758
@@ -757,1 +757,3 @@
 
+  // During instrumentation, function pointers are collected for the different
+  // indirect call targets.  Then as part of the conversion from raw to merged
----------------
davidxl wrote:
> CodeGenPGO::valueProfile is a common API which can also be used for other kinds of value profile, so the comments belong to the callsite of this method in CGCall.cpp.
> 
> Suggested wording:
> 
> For indirect function call value profiling, the addresses of the target functions are profiled by the instrumented code. The target addresses are written in the raw profile data and converted to target function name's MD5 hash by the profile reader during deserialization.
Hi David,

Thanks for taking a look.

I don't mind rewording the comment if you have some specific issues but your version drops many of the details that was painful for me to discover.  I carefully worded my comment to describe some of the design details for indirect profiling that are not covered elsewhere:

1) the remapping from function pointer to hashes of function names happens during profdata merging

  It was surprisingly hard to figure out what the PGO file contained for indirect call targets: function pointers or func name hashes?  And of course as stated, the answer is -- it depends.

2) the remapping happens pretty deep in the infrastructure code during deserializing of the rawdata

  I was looking at several more logical places to find where this happens and this was a bit unexpected location for this functionality.

3) how do we know the function addresses necessary for the mapping from function address -> func name -> hash

  The entire code to populate the FunctionAddr using macro magic in InstrProfiling::getOrCreateRegionCounters is pretty hard to follow.  I much rather have an overview of the logic somewhere centrally.

>From the above list I feel that your version dropped 1 and 3 and only kept 2.  Of course, feel free to add more context to my comments and fix inaccuracies/oversimplifications but I would want want to drop any of the points mentioned above.

Thanks.


http://reviews.llvm.org/D18489