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 ---------------- anemet wrote: > 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. > David,
> 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. How would you actually feel about moving this comment to InstrProfData.inc as well, just before the definition of IPVK_IndirectCallTarget? I think that's a better place in terms of its centrality since this applies to both early and late instrumentation. What do you think? Adam http://reviews.llvm.org/D18489 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
