[PATCH] D18489: [PGO, clang] Comment how function pointers for indirect calls are mapped to function names

[Xinliang David Li via cfe-commits]

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 <davidxl at google.com>
wrote:

>
>
> On Mon, Mar 28, 2016 at 12:31 PM, Adam Nemet <anemet at 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
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160328/22db69fb/attachment.html>