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

[Adam Nemet via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL264678: [PGO] More comments how function pointers for 
indirect calls are mapped (authored by anemet).

Changed prior to commit:
  http://reviews.llvm.org/D18489?vs=51713=51845#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D18489

Files:
  llvm/trunk/include/llvm/ProfileData/InstrProfData.inc

Index: llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
===
--- llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
+++ llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
@@ -133,6 +133,15 @@
 #else
 #define INSTR_PROF_DATA_DEFINED
 #endif
+/* 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.
+ *
+ * For this remapping the ProfData is used.  ProfData contains both the 
function
+ * name hash and the function address.
+ */
 VALUE_PROF_KIND(IPVK_IndirectCallTarget, 0)
 /* These two kinds must be the last to be
  * declared. This is to make sure the string


Index: llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
===
--- llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
+++ llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
@@ -133,6 +133,15 @@
 #else
 #define INSTR_PROF_DATA_DEFINED
 #endif
+/* 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.
+ *
+ * For this remapping the ProfData is used.  ProfData contains both the function
+ * name hash and the function address.
+ */
 VALUE_PROF_KIND(IPVK_IndirectCallTarget, 0)
 /* These two kinds must be the last to be
  * declared. This is to make sure the string
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Nemet via cfe-commits]

anemet accepted this revision.
anemet added a reviewer: anemet.
anemet added a comment.
This revision is now accepted and ready to land.

Was accepted by davidxl.


http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Xinliang David Li via cfe-commits]

Sure.

thanks,

David

On Mon, Mar 28, 2016 at 12:41 PM, Adam Nemet  wrote:

> anemet added a comment.
>
> In http://reviews.llvm.org/D18489#384851, @davidxl wrote:
>
> > What I meant is that putting the comments in InstrProfData.inc file, and
> >  update the one in CodeGenPGO.cpp to reference that.
>
>
> Sounds good.  You mean CGCall.cpp instead of CodeGenPGO.cpp though,
> correct?
>
> So to summarize, I'll move this comment to InstrProfData.inc and just have
> a reference to it in CGCall.cpp before the call to valueProfile.
>
> OK?
>
>
> http://reviews.llvm.org/D18489
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Nemet via cfe-commits]

anemet added a comment.

In http://reviews.llvm.org/D18489#384851, @davidxl wrote:

> What I meant is that putting the comments in InstrProfData.inc file, and
>  update the one in CodeGenPGO.cpp to reference that.


Sounds good.  You mean CGCall.cpp instead of CodeGenPGO.cpp though, correct?

So to summarize, I'll move this comment to InstrProfData.inc and just have a 
reference to it in CGCall.cpp before the call to valueProfile.

OK?


http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [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 
wrote:

>
>
> On Mon, Mar 28, 2016 at 12:31 PM, Adam Nemet  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 

Re: [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  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?
>
>

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

[Adam Nemet via cfe-commits]

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?

Adam 

Re: [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  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

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

[Adam Nemet via cfe-commits]

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.
``` 

> 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.



http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Li via cfe-commits]

davidxl 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:
> > 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.)


http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Adam Nemet via cfe-commits]

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:
> 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.


http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[David Li via cfe-commits]

davidxl 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

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.


http://reviews.llvm.org/D18489



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits