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

[Xinliang David Li via cfe-commits]

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/4aa7590b/attachment-0001.html>