davidxl 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 ---------------- 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.) http://reviews.llvm.org/D18489 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
