[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:
> anemet wrote:
> > 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.
> Actually bullet 1 is not dropped from my suggested version: each time when a raw profile data is read, the reader interface will covert the address into MD5 -- profile merging is simply a user of the reader. 
> 
> About bullet 3, it is ok to add it if you think it is useful. ( I did not suggest it because ProfData is needed here is not for the purpose of conversion, but to pass the context for the runtime to find/set the counter arrays.)
> Actually bullet 1 is not dropped from my suggested version: each time when a raw profile data is read, the reader interface will covert the address into MD5 -- profile merging is simply a user of the reader.

Sure but that is still pretty implicit.  I'd like to describe this in terms of the well established phases of PGO: instrumentation, profile merge,  recompile with profile data.

How about:

```
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.  Typically, this happens when the the raw profile data is read during profile merging.
``` 

> About bullet 3, it is ok to add it if you think it is useful. ( I did not suggest it because ProfData is needed here is not for the purpose of conversion, but to pass the context for the runtime to find/set the counter arrays.)

I am not completely sure I understand what you're saying here but if you mean that the comment does not really apply to the surrounding code then that I guess is expected.  I wanted to give a high-level overview of *what* we collect at run-time, and *how* we map that into function names hashes that are then used in the IR.

I can also put this in the function comment if you think that's better.



http://reviews.llvm.org/D18489