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