[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 10:40 AM, 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
> ----------------
> 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
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20160328/0cd554d1/attachment-0001.html>