On Mon, Mar 28, 2016 at 10:40 AM, 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 > ---------------- > 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. > This sounds great! > ``` > > > 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. Right. > 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 am fine putting them here. So LGTM with your revised version. David > I can also put this in the function comment if you think that's better. > > > > http://reviews.llvm.org/D18489 > > > >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits