What I meant is that putting the comments in InstrProfData.inc file, and update the one in CodeGenPGO.cpp to reference that.
David On Mon, Mar 28, 2016 at 12:35 PM, Xinliang David Li <davi...@google.com> wrote: > > > On Mon, Mar 28, 2016 at 12:31 PM, Adam Nemet <ane...@apple.com> wrote: > >> 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? >> >> > > Sounds ok -- but perhaps you can update the comment in the other place to > point to this one. > > David > > >> 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
