thr3ads.net - llvm dev - [LLVMdev] IC profiling infrastructure [Jul 2015]

If this information is useful, please help other people find it:
Share via:

Xinliang David Li

2015-Jul-03 05:49 UTC

[LLVMdev] IC profiling infrastructure

My suggestion:

1. Keep the value kind enum, but remove the reserved kinds
2. In compiler-rt code, assert the kind to be icalltarget and remove the
loop
3. Keep the indexed format (as is now)
4. Add assertion in profile use that icalltarget is the only expected kind.

Justin, does this sound reasonable?

David
On Jul 2, 2015 1:10 PM, "Betul Buyukkurt" <betulb at
codeaurora.org> wrote:
> Any decision on below? Is everyone OK w/ removing the value_kind and
> keeping
> the raw profile writer and reader code to do exactly what we need it to do
> today. Justin, if you agree, I'll make/upload the changes to right
away?
>
> Thanks,
> -Betul
> -----Original Message-----
> From: Betul Buyukkurt [mailto:betulb at codeaurora.org]
> Sent: Tuesday, June 30, 2015 4:18 PM
> To: Justin Bogner
> Cc: Xinliang David Li; Betul Buyukkurt; llvmdev
> Subject: Re: [LLVMdev] IC profiling infrastructure
>
> > Xinliang David Li <davidxl at google.com> writes:
> >> Justin, thanks for the reply.
> >> I would like to point out that value_kind is actually not really
> >> stored
> in the raw profile data (nor do we intend to do so), other than the minimal
> information about per value kind  NumOfSites info in per-function profile
> header. Basically the data is organized per value kind instead of stored
> intermixed. That is about it.
> >> More replies below.
> >> On Mon, Jun 29, 2015 at 11:53 AM, Justin Bogner
> >> <mail at justinbogner.com>
> > wrote:
> >>> I still have some issues with what we're doing with
value_kind here.
> > Let
> >>> me summarize the arguments I've heard for including the
value_kind
> >>> in
> various places in this set of changes:
> >>> 1. We would like to be able to enable multiple types of value
> profiling
> >>>    at the same time.
> >>> 1a. We would like to be able to selectively use only
particular value
> >>>     kinds from a particular profile.
> >>> 1b. We would like to be able to combine profiles with
different sets
> of
> >>>     profiling options enabled.
> >> yes.
> >>> 2. We will need to know the value kind in order to parse the
raw
> > profile
> >>>    data.
> >> Yes -- but we don't need to store value kind in order to do
that --
> >> we
> simply need to organize the profile data more structurally.
> >>> 3. We'd like to avoid needing to change the .profdata
format when we
> > add
> >>>    new value kinds, since it needs to stay backwards
compatible.
> >>> 4. The frontend work is simpler if the value kind is
explicitly
> > encoded.
> >>> There are a couple of these points that certainly need to be
> addressed,
> >>> and some that are a little bit less clear.
> >>> A. We do need to handle (1) in some way, but the issue can be
stated a
> >>>    little bit more generally. If there are optional parts of
the
> > profile
> >>>    data, we definitely need to know which options are enabled.
> >>> B. (1a) is only an issue if you believe (4). This might be
true or
> >>> it
> > might
> >>>    not, and I don't think we can make an informed decision
until
> >>> we're
> actually using multiple value kinds. For now there's just a bunch of
> >>>    code that's effectively useless - "if value kind
is 2, do nothing".
> > I'm
> >>>    opposed to adding this kind of practically unreachable code
"just
> in
> >>>    case". It adds a kind of complexity and premature
generality
> >>> that,
> > in my
> >>>    experience, will have to be completely re-done when we get
to
> > actually
> >>>    generalizing the code.
> >> In terms of value profiling code, there is no need for the check
you
> mentioned "if value kind is ..." at all, but 'attached'
to the codegen code
> for language construct of interests, for instance,
> >> ... CodeGenFunction::EmitCall(...)
> >> {
> >>     if (enable_indirect_profile) {
> >>            ....profileIndirectCall();     // does both
instrumentation
> >> and profile annotation depending on the context.
> >>     }
> >>    ...
> >> }
> >> Organizing the profile data according value profile kind allows
> >> profile
> data to be independently loaded and used:
> >> .. CodeGenPGO::assignRegionCounters (...) {
> >>    if (enable_indirect_profile) {
> >>      loadValueProfile(VK_indirect_call);
> >>    }
> >>    if (enable_xxx_profile) {
> >>     loadXXXProfile(VK_xxx);
> >>   }
> >> }
> >> if  we end up with inter-mixing all value kind profile data, we
will
> have to load all value profile data into memory regardless of the
> option specified.   It also makes the profile data uses really hard --
> how do we easily match the value profile site with the profile data counter
> index if profile kinds are selectively enabled?
> >> Besides, in the profile-use pass, each value kind's in-memory
format
> >> of
> profile data may be slightly different, so the loader will have to do
> different things ..
> >>> C. (1b) is an interesting one. Is this actually a useful
feature? If
> we
> >>>    want to combine multiple profiles with different subsets of
> > profiling
> >>>    turned on, then we definitely need the information recorded
> somewhere.
> >> I think it is a good flexibility to have.
> >>> D. I have issues with (2). If the different value kinds
actually have
> >>>    different structures and need to be read in / interpreted
> >>>    differently, then writing the support for them now simply
won't
> work.
> >> We choose to have very compact form representing the raw value
data
> >> for
> all value kinds, but to support efficient use of value profile of different
> kinds, the raw profile reader has to do something different here. For
> instance, indirect call profile data needs the translation of the raw
> address which other value kinds do not care.  I think this is something we
> can do without.
> >>>    We're reading them in as if they're interpreted in
the exact same
> way as the indirect call profiling - if they aren't, then we have to
> rewrite
> this code when we add a new one anyway. What's the point of writing
code
> that won't actually work when we try to use
> > it?
> >> I don't think this would be a problem as adding a new kind
requires
> client/use side change too, otherwise we won't be able to use it.
> > This is exactly my point. Why are we writing code *now* to read values
> that we have no way of using? This code *cannot* be tested, because it
> doesn't do anything. It adds a bunch of loops to iterate over values
that
> *must* be zero, because if they aren't we don't actually know what
they
> mean.
> > The compiler-rt patches I've seen so far have all treated each
value
> kind as if it should be treated identically to indirect call target, and
> had
> extra complexity so that they could do this, but you've stated multiple
> times that this won't actually be correct when we use it. We should
write
> the raw profile writer and reader code to do exactly what we need it to do
> today. We can and will change it when we want to add new functionality.
>
> Hi Justin,
>
> Is value_kind the only holding factor keeping these CL's from merging
in?
> If so, then I'm willing to do the change i.e. the writer and reader to
do
> exactly what IC profiling needs, as we'd like to have this effort
upstream.
> This will remove value_kind enum field from the source for now.
> The downside would be that the same enum will get reintroduced once new
> value types are added in, and hence newer format updates to the indexed
> readers.
>
> If all are willing to accept the code in this way, I'll go ahead and
take
> out the value_kind field. David, any objections?
>
> Thanks,
> -Betul
>
> >>> E. There is value in dealing with (3) as long as we're
confident
> >>> that
> > we
> >>>    can get it right. If we're pretty confident that the
value
> >>> profile
> data structure we're encoding in the .profdata file is going to work
> >>>    for multiple value profile kinds, then it makes sense to
allow
> multiple of them and encode which kinds they are. David, is
> "InstrProfValueRecord" in Betul's current patches going to
work to record
> the other types of value profiling information you're
> > planning
> >>>    on looking at?
> >> The raw profile format is certainly good for other kinds.
> > I'm not talking about the raw profile format here. This point is
about
> the stable, indexed format (.profdata, not .profraw). We have no need to
> keep the raw profile format backwards compatible, but we do need to keep
> readers for old indexed format files.
> >> InstrProfValueRecord is mostly there (for instance overloading the
> >> Name
> field for other purpose, but this is minor and can be done as follow up
> when
> support of other kinds are added), so I am ok to leave it as it is now.
> > What do you mean? What do you expect this structure to look like for
> other value kinds? Will we need to change the serialized format for the
> indexed profile format?
> > There are two possibilities. Either:
> > 1. This serialized format is sufficient for other value kinds, so it
is
> >    valuable to encode it in the indexed profile format now, because
> > this
> will avoid needing to change that format later.
> > Or,
> > 2. This is likely not how other value kinds will need to be
serialized,
> >    so we're going to need to change the format later either way.
In
> > this
> case, we should just solve the problem we have *today*, rather than making
> changes we'll need to undo later.
> > I think that given the uncertainty about what the future value
kinds'
> formats will be, we should encode this in a way where it's obvious what
the
> extra data is for (we'll specify a kind in the data), but without
trying to
> accomodate future value kinds until we're ready to talk about them
> concretely.
> > I'll go into more detail about this in my review for the indexed
> > profile
> reader and writer patch, which I should be able to get to tomorrow.
> >>> I'd also like to point out that the instrprof parts of
LLVM try to
> >>> be
> relatively frontend agnostic. We should strive to keep it such that the
> >>> frontend is the piece that decides what a particular profiling
kind
> means if possible, as long as the structure and way the data is laid
> > out
> >>> is suitable. This doesn't affect the current work very
much, but
> >>> it's
> a
> >>> good idea to keep it in mind.
> >> yes. See the above -- FE code does not really need to care about
the
> > format.
> >>> So, with all of that said, I have a couple of suggestions on
how to
> > move
> >>> forward.
> >>> - I don't think we should store the kind in the raw
profile yet. Let's
> >>>   keep this simple, since it's easy to change and will be
easier to
> get
> >>>   right when we're implementing or experimenting with a
second kind
> (based on (D)).
> >> See my reply above -- we don't really store the value kind in
raw
> profile, but we need to organize the profile data according to the kind in
> order to process (read) and use (profile-use) them
> >> efficiently.
> > The only reader of the raw profile is the tool that converts it to the
> indexed format, which is what profile-use consumes. The read will always
> read the entire file in a streaming fashion, and efficiently reading parts
> of the file is not important. We then write this to the indexed format,
> which needs to be efficiently readable.
> >>> - Assuming the answer to my question in (E) is
"yes", let's go ahead
> > and
> >>>   include a value kind for the value records in the .profdata
format.
> >> yes.
> >>>I
> >>>   don't really like the current approach of an array of
> >>>std::vectors,
> because I think we can do this more simply. I'll bring that up on the
> >>>   review thread.
> >>> - In clang, we should error if we see value kinds we don't
understand
> >>>   yet.
> >> yes.
> >>> I think that this addresses the short term concerns without
forcing
> >>> us
> to write unreachable/untestable code that may or may not work for the
> longer
> term concerns. It minimally handles (A ) in that the data that
> > is
> >>> there implies which features are being used, puts off (B)
without
> > making
> >>> it harder to deal with when the time comes, allows us to
implement
> >>> (C)
> without much trouble if we want it, avoids doing work that could cause
> problems with (D), and handles (E) since avoiding it would mean more work
> later.
> >>> What do you think?
> >> See my reply above. For now, I think the things we need to
organize
> >> the
> value profile data in raw profile according to kind (not storing) -- I
> think
> it will handle all the cases where you have concerns. thanks,
> >> David
> >>> Xinliang David Li <davidxl at google.com> writes:
> >>>> Justin, do you have more concerns on keeping value_kind?
> >>>> If there is still disagreement, can we agree to move on
with it for
> now ?  After the initial version of the patches checked in, we can do more
> serious testings with large apps and revisit this if there are problems
> discovered.
> >>>> thanks,
> >>>> David
> >>>> On Mon, Jun 15, 2015 at 10:47 PM, Xinliang David Li
> >>>> <davidxl at google.com> wrote:
> >>>>>> But since the consumer is the frontend, and it
knows which counts
> > are
> >>>>>> which, it knows the kind, no? I don't
understand how storing the
> > kind
> >>>>>> info helps or hurts - it's just redundant.
> >>>>> Yes, the frontend consumer knows, but the raw reader
does not. It
> >>>>> is
> natural to do those processing when processing the raw profile data (for
> instance when function pointer to name mapping still exists).
> >>>>>>>> The other thing we should do is store
which profiling options
> >>>>>>>> are
> enabled, in both formats. When we specify profile-instr-use it'll
> > be
> >>>>>>>> less error prone if we can detect whether
or not this includes
> > indirect
> >>>>>>>> call profiling without checking other
options, and it's
> >>>>>>>> probably
> a
> > good
> >>>>>>>> idea for "llvm-profdata merge"
to disallow merging profiles
> >>>>>>>> that
> > are
> >>>>>>>> gathering different sets of data. A
bitfield seems suitable for
> > this.
> >>>>>>> For value profiling, allowing profile-gen and
profile-use passes
> > using
> >>>>>>> different options is a useful feature.
Consider the following
> scenarios:
> >>>>>>> 1) collect the same profile data once with all
kinds of value
> > profile
> >>>>>>>    data collected. The exact same profile data
can be used in
> > performance
> >>>>>>>    experiments with different kinds of value
profiling
> > enabled/disabled
> >>>>>> This use case is pretty easy to accomodate for
with the model
> >>>>>> where
> > the
> >>>>>> profile stores its type. We could autodetect the
type to decide
> >>>>>> how
> > to
> >>>>>> interpret the profile, and if more specific
profile flags are set
> > simply
> >>>>>> ignore some of the data. The thing that the file
format storing
> >>>>>> the
> > type
> >>>>>> gives us is that it's harder to misinterpret
the data by
> >>>>>> supplying
> > the
> >>>>>> mismatching flags between reading and writing.
> >>>>> Yes, this is achievable with some extra effort -- for
instance by
> collecting the profile sites of all kinds regardless of the flags.
> After profiling matching, simply drop selected sites according to the
> >>>>> flags.
> >>>>> This adds complexity of the code. With value kinds
encoded in
> profile
> >>>>> data, the handling is much simpler -- each value
profiler only
> >>>>> needs
> to deal with its own kind.
> >>>>>>> 2) work around profile-use/value
transformation bug selectively
> for
> >>>>>>>    some file without the need to change
anything in
> instrumentation
> > pass
> >>>>>> I'm not sure I understand what you mean here.
> >>>>> Similar to the above, but for correctness.
> >>>>>>> Besides, with the latest patch, the value_kind
is not recorded
> >>>>>>> in
> > each
> >>>>>>> profile value thus the overhead is minimized.
> >>>>>> The overhead is low, sure, but the code complexity
of dealing
> >>>>>> with
> > the
> >>>>>> multiple kinds in this way is quite real.
> >>>>> I actually believe having value-kind can help reduce
code
> >>>>> complexity
> instead of the other way around. What is the extra complexity?
> thanks,
> >>>>> David
> >>>>>>Since I'm not convinced we're
> >>>>>> getting real benefit from storing the kind, I
don't believe this
> trade-off is worth it.
> >>>>>>> thanks,
> >>>>>>> David
>
>
>
>
>
>-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150702/019b88a8/attachment.html>

Justin Bogner

2015-Jul-08 14:13 UTC

head link

[LLVMdev] IC profiling infrastructure

Xinliang David Li <davidxl at google.com> writes:> My suggestion:
>
> 1. Keep the value kind enum, but remove the reserved kinds
Agreed.
> 2. In compiler-rt code, assert the kind to be icalltarget and remove the
loop
Yep, the compiler-rt code need only handle the indirect call kind for now.
> 3. Keep the indexed format (as is now)
This isn't quite ready as-is. I've gone into detail about how to move
forward on it in its review in the "Value profiling - patchset 3"
thread.
> 4. Add assertion in profile use that icalltarget is the only expected kind.
Shouldn't be necessary - the frontend will ask for a particular type of
profile where it's consuming it, no? If it only ever asks for indirect
call there's nothing to assert about.
>
> Justin, does this sound reasonable?
>
> David
>
> On Jul 2, 2015 1:10 PM, "Betul Buyukkurt" <betulb at
codeaurora.org> wrote:
>
>     Any decision on below? Is everyone OK w/ removing the value_kind and
>     keeping
>     the raw profile writer and reader code to do exactly what we need it to
do
>     today. Justin, if you agree, I'll make/upload the changes to right
away?
>    
>     Thanks,
>     -Betul
>     -----Original Message-----
>     From: Betul Buyukkurt [mailto:betulb at codeaurora.org]
>     Sent: Tuesday, June 30, 2015 4:18 PM
>     To: Justin Bogner
>     Cc: Xinliang David Li; Betul Buyukkurt; llvmdev
>     Subject: Re: [LLVMdev] IC profiling infrastructure
>    
>     > Xinliang David Li <davidxl at google.com> writes:
>     >> Justin, thanks for the reply.
>     >> I would like to point out that value_kind is actually not
really
>     >> stored
>     in the raw profile data (nor do we intend to do so), other than the
>     minimal
>     information about per value kind  NumOfSites info in per-function
profile
>     header. Basically the data is organized per value kind instead of
stored
>     intermixed. That is about it.
>     >> More replies below.
>     >> On Mon, Jun 29, 2015 at 11:53 AM, Justin Bogner
>     >> <mail at justinbogner.com>
>     > wrote:
>     >>> I still have some issues with what we're doing with
value_kind here.
>     > Let
>     >>> me summarize the arguments I've heard for including
the value_kind
>     >>> in
>     various places in this set of changes:
>     >>> 1. We would like to be able to enable multiple types of
value
>     profiling
>     >>>    at the same time.
>     >>> 1a. We would like to be able to selectively use only
particular value
>     >>>     kinds from a particular profile.
>     >>> 1b. We would like to be able to combine profiles with
different sets
>     of
>     >>>     profiling options enabled.
>     >> yes.
>     >>> 2. We will need to know the value kind in order to parse
the raw
>     > profile
>     >>>    data.
>     >> Yes -- but we don't need to store value kind in order to
do that --
>     >> we
>     simply need to organize the profile data more structurally.
>     >>> 3. We'd like to avoid needing to change the .profdata
format when we
>     > add
>     >>>    new value kinds, since it needs to stay backwards
compatible.
>     >>> 4. The frontend work is simpler if the value kind is
explicitly
>     > encoded.
>     >>> There are a couple of these points that certainly need to
be
>     addressed,
>     >>> and some that are a little bit less clear.
>     >>> A. We do need to handle (1) in some way, but the issue can
be stated a
>     >>>    little bit more generally. If there are optional parts
of the
>     > profile
>     >>>    data, we definitely need to know which options are
enabled.
>     >>> B. (1a) is only an issue if you believe (4). This might be
true or
>     >>> it
>     > might
>     >>>    not, and I don't think we can make an informed
decision until
>     >>> we're
>     actually using multiple value kinds. For now there's just a bunch
of
>     >>>    code that's effectively useless - "if value
kind is 2, do nothing".
>     > I'm
>     >>>    opposed to adding this kind of practically unreachable
code "just
>     in
>     >>>    case". It adds a kind of complexity and premature
generality
>     >>> that,
>     > in my
>     >>>    experience, will have to be completely re-done when we
get to
>     > actually
>     >>>    generalizing the code.
>     >> In terms of value profiling code, there is no need for the
check you
>     mentioned "if value kind is ..." at all, but
'attached' to the codegen
>     code
>     for language construct of interests, for instance,
>     >> ... CodeGenFunction::EmitCall(...)
>     >> {
>     >>     if (enable_indirect_profile) {
>     >>            ....profileIndirectCall();     // does both
instrumentation
>     >> and profile annotation depending on the context.
>     >>     }
>     >>    ...
>     >> }
>     >> Organizing the profile data according value profile kind
allows
>     >> profile
>     data to be independently loaded and used:
>     >> .. CodeGenPGO::assignRegionCounters (...) {
>     >>    if (enable_indirect_profile) {
>     >>      loadValueProfile(VK_indirect_call);
>     >>    }
>     >>    if (enable_xxx_profile) {
>     >>     loadXXXProfile(VK_xxx);
>     >>   }
>     >> }
>     >> if  we end up with inter-mixing all value kind profile data,
we will
>     have to load all value profile data into memory regardless of the
>     option specified.   It also makes the profile data uses really hard --
>     how do we easily match the value profile site with the profile data
>     counter
>     index if profile kinds are selectively enabled?
>     >> Besides, in the profile-use pass, each value kind's
in-memory format
>     >> of
>     profile data may be slightly different, so the loader will have to do
>     different things ..
>     >>> C. (1b) is an interesting one. Is this actually a useful
feature? If
>     we
>     >>>    want to combine multiple profiles with different
subsets of
>     > profiling
>     >>>    turned on, then we definitely need the information
recorded
>     somewhere.
>     >> I think it is a good flexibility to have.
>     >>> D. I have issues with (2). If the different value kinds
actually have
>     >>>    different structures and need to be read in /
interpreted
>     >>>    differently, then writing the support for them now
simply won't
>     work.
>     >> We choose to have very compact form representing the raw value
data
>     >> for
>     all value kinds, but to support efficient use of value profile of
>     different
>     kinds, the raw profile reader has to do something different here. For
>     instance, indirect call profile data needs the translation of the raw
>     address which other value kinds do not care.  I think this is something
we
>     can do without.
>     >>>    We're reading them in as if they're interpreted
in the exact same
>     way as the indirect call profiling - if they aren't, then we have
to
>     rewrite
>     this code when we add a new one anyway. What's the point of writing
code
>     that won't actually work when we try to use
>     > it?
>     >> I don't think this would be a problem as adding a new kind
requires
>     client/use side change too, otherwise we won't be able to use it.
>     > This is exactly my point. Why are we writing code *now* to read
values
>     that we have no way of using? This code *cannot* be tested, because it
>     doesn't do anything. It adds a bunch of loops to iterate over
values that
>     *must* be zero, because if they aren't we don't actually know
what they
>     mean.
>     > The compiler-rt patches I've seen so far have all treated each
value
>     kind as if it should be treated identically to indirect call target,
and
>     had
>     extra complexity so that they could do this, but you've stated
multiple
>     times that this won't actually be correct when we use it. We should
write
>     the raw profile writer and reader code to do exactly what we need it to
do
>     today. We can and will change it when we want to add new functionality.
>    
>     Hi Justin,
>    
>     Is value_kind the only holding factor keeping these CL's from
merging in?
>     If so, then I'm willing to do the change i.e. the writer and reader
to do
>     exactly what IC profiling needs, as we'd like to have this effort
>     upstream.
>     This will remove value_kind enum field from the source for now.
>     The downside would be that the same enum will get reintroduced once new
>     value types are added in, and hence newer format updates to the indexed
>     readers.
>    
>     If all are willing to accept the code in this way, I'll go ahead
and take
>     out the value_kind field. David, any objections?
>    
>     Thanks,
>     -Betul
>    
>     >>> E. There is value in dealing with (3) as long as we're
confident
>     >>> that
>     > we
>     >>>    can get it right. If we're pretty confident that
the value
>     >>> profile
>     data structure we're encoding in the .profdata file is going to
work
>     >>>    for multiple value profile kinds, then it makes sense
to allow
>     multiple of them and encode which kinds they are. David, is
>     "InstrProfValueRecord" in Betul's current patches going
to work to record
>     the other types of value profiling information you're
>     > planning
>     >>>    on looking at?
>     >> The raw profile format is certainly good for other kinds.
>     > I'm not talking about the raw profile format here. This point
is about
>     the stable, indexed format (.profdata, not .profraw). We have no need
to
>     keep the raw profile format backwards compatible, but we do need to
keep
>     readers for old indexed format files.
>     >> InstrProfValueRecord is mostly there (for instance overloading
the
>     >> Name
>     field for other purpose, but this is minor and can be done as follow up
>     when
>     support of other kinds are added), so I am ok to leave it as it is now.
>     > What do you mean? What do you expect this structure to look like
for
>     other value kinds? Will we need to change the serialized format for the
>     indexed profile format?
>     > There are two possibilities. Either:
>     > 1. This serialized format is sufficient for other value kinds, so
it is
>     >    valuable to encode it in the indexed profile format now,
because
>     > this
>     will avoid needing to change that format later.
>     > Or,
>     > 2. This is likely not how other value kinds will need to be
serialized,
>     >    so we're going to need to change the format later either
way. In
>     > this
>     case, we should just solve the problem we have *today*, rather than
making
>     changes we'll need to undo later.
>     > I think that given the uncertainty about what the future value
kinds'
>     formats will be, we should encode this in a way where it's obvious
what
>     the
>     extra data is for (we'll specify a kind in the data), but without
trying
>     to
>     accomodate future value kinds until we're ready to talk about them
>     concretely.
>     > I'll go into more detail about this in my review for the
indexed
>     > profile
>     reader and writer patch, which I should be able to get to tomorrow.
>     >>> I'd also like to point out that the instrprof parts of
LLVM try to
>     >>> be
>     relatively frontend agnostic. We should strive to keep it such that the
>     >>> frontend is the piece that decides what a particular
profiling kind
>     means if possible, as long as the structure and way the data is laid
>     > out
>     >>> is suitable. This doesn't affect the current work very
much, but
>     >>> it's
>     a
>     >>> good idea to keep it in mind.
>     >> yes. See the above -- FE code does not really need to care
about the
>     > format.
>     >>> So, with all of that said, I have a couple of suggestions
on how to
>     > move
>     >>> forward.
>     >>> - I don't think we should store the kind in the raw
profile yet. Let's
>     >>>   keep this simple, since it's easy to change and will
be easier to
>     get
>     >>>   right when we're implementing or experimenting with
a second kind
>     (based on (D)).
>     >> See my reply above -- we don't really store the value kind
in raw
>     profile, but we need to organize the profile data according to the kind
in
>     order to process (read) and use (profile-use) them
>     >> efficiently.
>     > The only reader of the raw profile is the tool that converts it to
the
>     indexed format, which is what profile-use consumes. The read will
always
>     read the entire file in a streaming fashion, and efficiently reading
parts
>     of the file is not important. We then write this to the indexed format,
>     which needs to be efficiently readable.
>     >>> - Assuming the answer to my question in (E) is
"yes", let's go ahead
>     > and
>     >>>   include a value kind for the value records in the
.profdata format.
>     >> yes.
>     >>>I
>     >>>   don't really like the current approach of an array
of
>     >>>std::vectors,
>     because I think we can do this more simply. I'll bring that up on
the
>     >>>   review thread.
>     >>> - In clang, we should error if we see value kinds we
don't understand
>     >>>   yet.
>     >> yes.
>     >>> I think that this addresses the short term concerns
without forcing
>     >>> us
>     to write unreachable/untestable code that may or may not work for the
>     longer
>     term concerns. It minimally handles (A ) in that the data that
>     > is
>     >>> there implies which features are being used, puts off (B)
without
>     > making
>     >>> it harder to deal with when the time comes, allows us to
implement
>     >>> (C)
>     without much trouble if we want it, avoids doing work that could cause
>     problems with (D), and handles (E) since avoiding it would mean more
work
>     later.
>     >>> What do you think?
>     >> See my reply above. For now, I think the things we need to
organize
>     >> the
>     value profile data in raw profile according to kind (not storing) -- I
>     think
>     it will handle all the cases where you have concerns. thanks,
>     >> David
>     >>> Xinliang David Li <davidxl at google.com> writes:
>     >>>> Justin, do you have more concerns on keeping
value_kind?
>     >>>> If there is still disagreement, can we agree to move
on with it for
>     now ?  After the initial version of the patches checked in, we can do
more
>     serious testings with large apps and revisit this if there are problems
>     discovered.
>     >>>> thanks,
>     >>>> David
>     >>>> On Mon, Jun 15, 2015 at 10:47 PM, Xinliang David Li
>     >>>> <davidxl at google.com> wrote:
>     >>>>>> But since the consumer is the frontend, and it
knows which counts
>     > are
>     >>>>>> which, it knows the kind, no? I don't
understand how storing the
>     > kind
>     >>>>>> info helps or hurts - it's just redundant.
>     >>>>> Yes, the frontend consumer knows, but the raw
reader does not. It
>     >>>>> is
>     natural to do those processing when processing the raw profile data
(for
>     instance when function pointer to name mapping still exists).
>     >>>>>>>> The other thing we should do is store
which profiling options
>     >>>>>>>> are
>     enabled, in both formats. When we specify profile-instr-use it'll
>     > be
>     >>>>>>>> less error prone if we can detect
whether or not this includes
>     > indirect
>     >>>>>>>> call profiling without checking other
options, and it's
>     >>>>>>>> probably
>     a
>     > good
>     >>>>>>>> idea for "llvm-profdata
merge" to disallow merging profiles
>     >>>>>>>> that
>     > are
>     >>>>>>>> gathering different sets of data. A
bitfield seems suitable for
>     > this.
>     >>>>>>> For value profiling, allowing profile-gen
and profile-use passes
>     > using
>     >>>>>>> different options is a useful feature.
Consider the following
>     scenarios:
>     >>>>>>> 1) collect the same profile data once with
all kinds of value
>     > profile
>     >>>>>>>    data collected. The exact same profile
data can be used in
>     > performance
>     >>>>>>>    experiments with different kinds of
value profiling
>     > enabled/disabled
>     >>>>>> This use case is pretty easy to accomodate for
with the model
>     >>>>>> where
>     > the
>     >>>>>> profile stores its type. We could autodetect
the type to decide
>     >>>>>> how
>     > to
>     >>>>>> interpret the profile, and if more specific
profile flags are set
>     > simply
>     >>>>>> ignore some of the data. The thing that the
file format storing
>     >>>>>> the
>     > type
>     >>>>>> gives us is that it's harder to
misinterpret the data by
>     >>>>>> supplying
>     > the
>     >>>>>> mismatching flags between reading and writing.
>     >>>>> Yes, this is achievable with some extra effort --
for instance by
>     collecting the profile sites of all kinds regardless of the flags.
>     After profiling matching, simply drop selected sites according to the
>     >>>>> flags.
>     >>>>> This adds complexity of the code. With value kinds
encoded in
>     profile
>     >>>>> data, the handling is much simpler -- each value
profiler only
>     >>>>> needs
>     to deal with its own kind.
>     >>>>>>> 2) work around profile-use/value
transformation bug selectively
>     for
>     >>>>>>>    some file without the need to change
anything in
>     instrumentation
>     > pass
>     >>>>>> I'm not sure I understand what you mean
here.
>     >>>>> Similar to the above, but for correctness.
>     >>>>>>> Besides, with the latest patch, the
value_kind is not recorded
>     >>>>>>> in
>     > each
>     >>>>>>> profile value thus the overhead is
minimized.
>     >>>>>> The overhead is low, sure, but the code
complexity of dealing
>     >>>>>> with
>     > the
>     >>>>>> multiple kinds in this way is quite real.
>     >>>>> I actually believe having value-kind can help
reduce code
>     >>>>> complexity
>     instead of the other way around. What is the extra complexity?
>     thanks,
>     >>>>> David
>     >>>>>>Since I'm not convinced we're
>     >>>>>> getting real benefit from storing the kind, I
don't believe this
>     trade-off is worth it.
>     >>>>>>> thanks,
>     >>>>>>> David

Xinliang David Li

2015-Jul-08 18:06 UTC

head link

[LLVMdev] IC profiling infrastructure

On Wed, Jul 8, 2015 at 7:13 AM, Justin Bogner <mail at justinbogner.com>
wrote:> Xinliang David Li <davidxl at google.com> writes:
>> My suggestion:
>>
>> 1. Keep the value kind enum, but remove the reserved kinds
>
> Agreed.
>
>> 2. In compiler-rt code, assert the kind to be icalltarget and remove
the loop
>
> Yep, the compiler-rt code need only handle the indirect call kind for now.
>
>> 3. Keep the indexed format (as is now)
>
> This isn't quite ready as-is. I've gone into detail about how to
move
> forward on it in its review in the "Value profiling - patchset 3"
> thread.
>
>> 4. Add assertion in profile use that icalltarget is the only expected
kind.
>
> Shouldn't be necessary - the frontend will ask for a particular type of
> profile where it's consuming it, no? If it only ever asks for indirect
> call there's nothing to assert about.
>
Sure.

thanks,

David
>>
>> Justin, does this sound reasonable?
>>
>> David
>>
>> On Jul 2, 2015 1:10 PM, "Betul Buyukkurt" <betulb at
codeaurora.org> wrote:
>>
>>     Any decision on below? Is everyone OK w/ removing the value_kind
and
>>     keeping
>>     the raw profile writer and reader code to do exactly what we need
it to do
>>     today. Justin, if you agree, I'll make/upload the changes to
right away?
>>
>>     Thanks,
>>     -Betul
>>     -----Original Message-----
>>     From: Betul Buyukkurt [mailto:betulb at codeaurora.org]
>>     Sent: Tuesday, June 30, 2015 4:18 PM
>>     To: Justin Bogner
>>     Cc: Xinliang David Li; Betul Buyukkurt; llvmdev
>>     Subject: Re: [LLVMdev] IC profiling infrastructure
>>
>>     > Xinliang David Li <davidxl at google.com> writes:
>>     >> Justin, thanks for the reply.
>>     >> I would like to point out that value_kind is actually not
really
>>     >> stored
>>     in the raw profile data (nor do we intend to do so), other than the
>>     minimal
>>     information about per value kind  NumOfSites info in per-function
profile
>>     header. Basically the data is organized per value kind instead of
stored
>>     intermixed. That is about it.
>>     >> More replies below.
>>     >> On Mon, Jun 29, 2015 at 11:53 AM, Justin Bogner
>>     >> <mail at justinbogner.com>
>>     > wrote:
>>     >>> I still have some issues with what we're doing
with value_kind here.
>>     > Let
>>     >>> me summarize the arguments I've heard for
including the value_kind
>>     >>> in
>>     various places in this set of changes:
>>     >>> 1. We would like to be able to enable multiple types
of value
>>     profiling
>>     >>>    at the same time.
>>     >>> 1a. We would like to be able to selectively use only
particular value
>>     >>>     kinds from a particular profile.
>>     >>> 1b. We would like to be able to combine profiles with
different sets
>>     of
>>     >>>     profiling options enabled.
>>     >> yes.
>>     >>> 2. We will need to know the value kind in order to
parse the raw
>>     > profile
>>     >>>    data.
>>     >> Yes -- but we don't need to store value kind in order
to do that --
>>     >> we
>>     simply need to organize the profile data more structurally.
>>     >>> 3. We'd like to avoid needing to change the
.profdata format when we
>>     > add
>>     >>>    new value kinds, since it needs to stay backwards
compatible.
>>     >>> 4. The frontend work is simpler if the value kind is
explicitly
>>     > encoded.
>>     >>> There are a couple of these points that certainly need
to be
>>     addressed,
>>     >>> and some that are a little bit less clear.
>>     >>> A. We do need to handle (1) in some way, but the issue
can be stated a
>>     >>>    little bit more generally. If there are optional
parts of the
>>     > profile
>>     >>>    data, we definitely need to know which options are
enabled.
>>     >>> B. (1a) is only an issue if you believe (4). This
might be true or
>>     >>> it
>>     > might
>>     >>>    not, and I don't think we can make an informed
decision until
>>     >>> we're
>>     actually using multiple value kinds. For now there's just a
bunch of
>>     >>>    code that's effectively useless - "if
value kind is 2, do nothing".
>>     > I'm
>>     >>>    opposed to adding this kind of practically
unreachable code "just
>>     in
>>     >>>    case". It adds a kind of complexity and
premature generality
>>     >>> that,
>>     > in my
>>     >>>    experience, will have to be completely re-done when
we get to
>>     > actually
>>     >>>    generalizing the code.
>>     >> In terms of value profiling code, there is no need for the
check you
>>     mentioned "if value kind is ..." at all, but
'attached' to the codegen
>>     code
>>     for language construct of interests, for instance,
>>     >> ... CodeGenFunction::EmitCall(...)
>>     >> {
>>     >>     if (enable_indirect_profile) {
>>     >>            ....profileIndirectCall();     // does both
instrumentation
>>     >> and profile annotation depending on the context.
>>     >>     }
>>     >>    ...
>>     >> }
>>     >> Organizing the profile data according value profile kind
allows
>>     >> profile
>>     data to be independently loaded and used:
>>     >> .. CodeGenPGO::assignRegionCounters (...) {
>>     >>    if (enable_indirect_profile) {
>>     >>      loadValueProfile(VK_indirect_call);
>>     >>    }
>>     >>    if (enable_xxx_profile) {
>>     >>     loadXXXProfile(VK_xxx);
>>     >>   }
>>     >> }
>>     >> if  we end up with inter-mixing all value kind profile
data, we will
>>     have to load all value profile data into memory regardless of the
>>     option specified.   It also makes the profile data uses really hard
--
>>     how do we easily match the value profile site with the profile data
>>     counter
>>     index if profile kinds are selectively enabled?
>>     >> Besides, in the profile-use pass, each value kind's
in-memory format
>>     >> of
>>     profile data may be slightly different, so the loader will have to
do
>>     different things ..
>>     >>> C. (1b) is an interesting one. Is this actually a
useful feature? If
>>     we
>>     >>>    want to combine multiple profiles with different
subsets of
>>     > profiling
>>     >>>    turned on, then we definitely need the information
recorded
>>     somewhere.
>>     >> I think it is a good flexibility to have.
>>     >>> D. I have issues with (2). If the different value
kinds actually have
>>     >>>    different structures and need to be read in /
interpreted
>>     >>>    differently, then writing the support for them now
simply won't
>>     work.
>>     >> We choose to have very compact form representing the raw
value data
>>     >> for
>>     all value kinds, but to support efficient use of value profile of
>>     different
>>     kinds, the raw profile reader has to do something different here.
For
>>     instance, indirect call profile data needs the translation of the
raw
>>     address which other value kinds do not care.  I think this is
something we
>>     can do without.
>>     >>>    We're reading them in as if they're
interpreted in the exact same
>>     way as the indirect call profiling - if they aren't, then we
have to
>>     rewrite
>>     this code when we add a new one anyway. What's the point of
writing code
>>     that won't actually work when we try to use
>>     > it?
>>     >> I don't think this would be a problem as adding a new
kind requires
>>     client/use side change too, otherwise we won't be able to use
it.
>>     > This is exactly my point. Why are we writing code *now* to
read values
>>     that we have no way of using? This code *cannot* be tested, because
it
>>     doesn't do anything. It adds a bunch of loops to iterate over
values that
>>     *must* be zero, because if they aren't we don't actually
know what they
>>     mean.
>>     > The compiler-rt patches I've seen so far have all treated
each value
>>     kind as if it should be treated identically to indirect call
target, and
>>     had
>>     extra complexity so that they could do this, but you've stated
multiple
>>     times that this won't actually be correct when we use it. We
should write
>>     the raw profile writer and reader code to do exactly what we need
it to do
>>     today. We can and will change it when we want to add new
functionality.
>>
>>     Hi Justin,
>>
>>     Is value_kind the only holding factor keeping these CL's from
merging in?
>>     If so, then I'm willing to do the change i.e. the writer and
reader to do
>>     exactly what IC profiling needs, as we'd like to have this
effort
>>     upstream.
>>     This will remove value_kind enum field from the source for now.
>>     The downside would be that the same enum will get reintroduced once
new
>>     value types are added in, and hence newer format updates to the
indexed
>>     readers.
>>
>>     If all are willing to accept the code in this way, I'll go
ahead and take
>>     out the value_kind field. David, any objections?
>>
>>     Thanks,
>>     -Betul
>>
>>     >>> E. There is value in dealing with (3) as long as
we're confident
>>     >>> that
>>     > we
>>     >>>    can get it right. If we're pretty confident
that the value
>>     >>> profile
>>     data structure we're encoding in the .profdata file is going to
work
>>     >>>    for multiple value profile kinds, then it makes
sense to allow
>>     multiple of them and encode which kinds they are. David, is
>>     "InstrProfValueRecord" in Betul's current patches
going to work to record
>>     the other types of value profiling information you're
>>     > planning
>>     >>>    on looking at?
>>     >> The raw profile format is certainly good for other kinds.
>>     > I'm not talking about the raw profile format here. This
point is about
>>     the stable, indexed format (.profdata, not .profraw). We have no
need to
>>     keep the raw profile format backwards compatible, but we do need to
keep
>>     readers for old indexed format files.
>>     >> InstrProfValueRecord is mostly there (for instance
overloading the
>>     >> Name
>>     field for other purpose, but this is minor and can be done as
follow up
>>     when
>>     support of other kinds are added), so I am ok to leave it as it is
now.
>>     > What do you mean? What do you expect this structure to look
like for
>>     other value kinds? Will we need to change the serialized format for
the
>>     indexed profile format?
>>     > There are two possibilities. Either:
>>     > 1. This serialized format is sufficient for other value kinds,
so it is
>>     >    valuable to encode it in the indexed profile format now,
because
>>     > this
>>     will avoid needing to change that format later.
>>     > Or,
>>     > 2. This is likely not how other value kinds will need to be
serialized,
>>     >    so we're going to need to change the format later
either way. In
>>     > this
>>     case, we should just solve the problem we have *today*, rather than
making
>>     changes we'll need to undo later.
>>     > I think that given the uncertainty about what the future value
kinds'
>>     formats will be, we should encode this in a way where it's
obvious what
>>     the
>>     extra data is for (we'll specify a kind in the data), but
without trying
>>     to
>>     accomodate future value kinds until we're ready to talk about
them
>>     concretely.
>>     > I'll go into more detail about this in my review for the
indexed
>>     > profile
>>     reader and writer patch, which I should be able to get to tomorrow.
>>     >>> I'd also like to point out that the instrprof
parts of LLVM try to
>>     >>> be
>>     relatively frontend agnostic. We should strive to keep it such that
the
>>     >>> frontend is the piece that decides what a particular
profiling kind
>>     means if possible, as long as the structure and way the data is
laid
>>     > out
>>     >>> is suitable. This doesn't affect the current work
very much, but
>>     >>> it's
>>     a
>>     >>> good idea to keep it in mind.
>>     >> yes. See the above -- FE code does not really need to care
about the
>>     > format.
>>     >>> So, with all of that said, I have a couple of
suggestions on how to
>>     > move
>>     >>> forward.
>>     >>> - I don't think we should store the kind in the
raw profile yet. Let's
>>     >>>   keep this simple, since it's easy to change and
will be easier to
>>     get
>>     >>>   right when we're implementing or experimenting
with a second kind
>>     (based on (D)).
>>     >> See my reply above -- we don't really store the value
kind in raw
>>     profile, but we need to organize the profile data according to the
kind in
>>     order to process (read) and use (profile-use) them
>>     >> efficiently.
>>     > The only reader of the raw profile is the tool that converts
it to the
>>     indexed format, which is what profile-use consumes. The read will
always
>>     read the entire file in a streaming fashion, and efficiently
reading parts
>>     of the file is not important. We then write this to the indexed
format,
>>     which needs to be efficiently readable.
>>     >>> - Assuming the answer to my question in (E) is
"yes", let's go ahead
>>     > and
>>     >>>   include a value kind for the value records in the
.profdata format.
>>     >> yes.
>>     >>>I
>>     >>>   don't really like the current approach of an
array of
>>     >>>std::vectors,
>>     because I think we can do this more simply. I'll bring that up
on the
>>     >>>   review thread.
>>     >>> - In clang, we should error if we see value kinds we
don't understand
>>     >>>   yet.
>>     >> yes.
>>     >>> I think that this addresses the short term concerns
without forcing
>>     >>> us
>>     to write unreachable/untestable code that may or may not work for
the
>>     longer
>>     term concerns. It minimally handles (A ) in that the data that
>>     > is
>>     >>> there implies which features are being used, puts off
(B) without
>>     > making
>>     >>> it harder to deal with when the time comes, allows us
to implement
>>     >>> (C)
>>     without much trouble if we want it, avoids doing work that could
cause
>>     problems with (D), and handles (E) since avoiding it would mean
more work
>>     later.
>>     >>> What do you think?
>>     >> See my reply above. For now, I think the things we need to
organize
>>     >> the
>>     value profile data in raw profile according to kind (not storing)
-- I
>>     think
>>     it will handle all the cases where you have concerns. thanks,
>>     >> David
>>     >>> Xinliang David Li <davidxl at google.com>
writes:
>>     >>>> Justin, do you have more concerns on keeping
value_kind?
>>     >>>> If there is still disagreement, can we agree to
move on with it for
>>     now ?  After the initial version of the patches checked in, we can
do more
>>     serious testings with large apps and revisit this if there are
problems
>>     discovered.
>>     >>>> thanks,
>>     >>>> David
>>     >>>> On Mon, Jun 15, 2015 at 10:47 PM, Xinliang David
Li
>>     >>>> <davidxl at google.com> wrote:
>>     >>>>>> But since the consumer is the frontend,
and it knows which counts
>>     > are
>>     >>>>>> which, it knows the kind, no? I don't
understand how storing the
>>     > kind
>>     >>>>>> info helps or hurts - it's just
redundant.
>>     >>>>> Yes, the frontend consumer knows, but the raw
reader does not. It
>>     >>>>> is
>>     natural to do those processing when processing the raw profile data
(for
>>     instance when function pointer to name mapping still exists).
>>     >>>>>>>> The other thing we should do is
store which profiling options
>>     >>>>>>>> are
>>     enabled, in both formats. When we specify profile-instr-use
it'll
>>     > be
>>     >>>>>>>> less error prone if we can detect
whether or not this includes
>>     > indirect
>>     >>>>>>>> call profiling without checking
other options, and it's
>>     >>>>>>>> probably
>>     a
>>     > good
>>     >>>>>>>> idea for "llvm-profdata
merge" to disallow merging profiles
>>     >>>>>>>> that
>>     > are
>>     >>>>>>>> gathering different sets of data.
A bitfield seems suitable for
>>     > this.
>>     >>>>>>> For value profiling, allowing
profile-gen and profile-use passes
>>     > using
>>     >>>>>>> different options is a useful feature.
Consider the following
>>     scenarios:
>>     >>>>>>> 1) collect the same profile data once
with all kinds of value
>>     > profile
>>     >>>>>>>    data collected. The exact same
profile data can be used in
>>     > performance
>>     >>>>>>>    experiments with different kinds of
value profiling
>>     > enabled/disabled
>>     >>>>>> This use case is pretty easy to accomodate
for with the model
>>     >>>>>> where
>>     > the
>>     >>>>>> profile stores its type. We could
autodetect the type to decide
>>     >>>>>> how
>>     > to
>>     >>>>>> interpret the profile, and if more
specific profile flags are set
>>     > simply
>>     >>>>>> ignore some of the data. The thing that
the file format storing
>>     >>>>>> the
>>     > type
>>     >>>>>> gives us is that it's harder to
misinterpret the data by
>>     >>>>>> supplying
>>     > the
>>     >>>>>> mismatching flags between reading and
writing.
>>     >>>>> Yes, this is achievable with some extra effort
-- for instance by
>>     collecting the profile sites of all kinds regardless of the flags.
>>     After profiling matching, simply drop selected sites according to
the
>>     >>>>> flags.
>>     >>>>> This adds complexity of the code. With value
kinds encoded in
>>     profile
>>     >>>>> data, the handling is much simpler -- each
value profiler only
>>     >>>>> needs
>>     to deal with its own kind.
>>     >>>>>>> 2) work around profile-use/value
transformation bug selectively
>>     for
>>     >>>>>>>    some file without the need to
change anything in
>>     instrumentation
>>     > pass
>>     >>>>>> I'm not sure I understand what you
mean here.
>>     >>>>> Similar to the above, but for correctness.
>>     >>>>>>> Besides, with the latest patch, the
value_kind is not recorded
>>     >>>>>>> in
>>     > each
>>     >>>>>>> profile value thus the overhead is
minimized.
>>     >>>>>> The overhead is low, sure, but the code
complexity of dealing
>>     >>>>>> with
>>     > the
>>     >>>>>> multiple kinds in this way is quite real.
>>     >>>>> I actually believe having value-kind can help
reduce code
>>     >>>>> complexity
>>     instead of the other way around. What is the extra complexity?
>>     thanks,
>>     >>>>> David
>>     >>>>>>Since I'm not convinced we're
>>     >>>>>> getting real benefit from storing the
kind, I don't believe this
>>     trade-off is worth it.
>>     >>>>>>> thanks,
>>     >>>>>>> David

Betul Buyukkurt

2015-Jul-08 18:37 UTC

head link

[LLVMdev] IC profiling infrastructure

Thanks for the responses, I'll add my responses to the review comments in
the Value profiling - patchset 3 thread.

Thanks,
-Betul
> Xinliang David Li <davidxl at google.com> writes:
>> My suggestion:
>>
>> 1. Keep the value kind enum, but remove the reserved kinds
>
> Agreed.
>
>> 2. In compiler-rt code, assert the kind to be icalltarget and remove
the
>> loop
>
> Yep, the compiler-rt code need only handle the indirect call kind for now.
>
>> 3. Keep the indexed format (as is now)
>
> This isn't quite ready as-is. I've gone into detail about how to
move
> forward on it in its review in the "Value profiling - patchset 3"
> thread.
>
>> 4. Add assertion in profile use that icalltarget is the only expected
>> kind.
>
> Shouldn't be necessary - the frontend will ask for a particular type of
> profile where it's consuming it, no? If it only ever asks for indirect
> call there's nothing to assert about.
>
>>
>> Justin, does this sound reasonable?
>>
>> David
>>
>> On Jul 2, 2015 1:10 PM, "Betul Buyukkurt" <betulb at
codeaurora.org> wrote:
>>
>>     Any decision on below? Is everyone OK w/ removing the value_kind
and
>>     keeping
>>     the raw profile writer and reader code to do exactly what we need
it
>> to do
>>     today. Justin, if you agree, I'll make/upload the changes to
right
>> away?
>>
>>     Thanks,
>>     -Betul
>>     -----Original Message-----
>>     From: Betul Buyukkurt [mailto:betulb at codeaurora.org]
>>     Sent: Tuesday, June 30, 2015 4:18 PM
>>     To: Justin Bogner
>>     Cc: Xinliang David Li; Betul Buyukkurt; llvmdev
>>     Subject: Re: [LLVMdev] IC profiling infrastructure
>>
>>     > Xinliang David Li <davidxl at google.com> writes:
>>     >> Justin, thanks for the reply.
>>     >> I would like to point out that value_kind is actually not
really
>>     >> stored
>>     in the raw profile data (nor do we intend to do so), other than the
>>     minimal
>>     information about per value kind  NumOfSites info in per-function
>> profile
>>     header. Basically the data is organized per value kind instead of
>> stored
>>     intermixed. That is about it.
>>     >> More replies below.
>>     >> On Mon, Jun 29, 2015 at 11:53 AM, Justin Bogner
>>     >> <mail at justinbogner.com>
>>     > wrote:
>>     >>> I still have some issues with what we're doing
with value_kind
>> here.
>>     > Let
>>     >>> me summarize the arguments I've heard for
including the
>> value_kind
>>     >>> in
>>     various places in this set of changes:
>>     >>> 1. We would like to be able to enable multiple types
of value
>>     profiling
>>     >>>    at the same time.
>>     >>> 1a. We would like to be able to selectively use only
particular
>> value
>>     >>>     kinds from a particular profile.
>>     >>> 1b. We would like to be able to combine profiles with
different
>> sets
>>     of
>>     >>>     profiling options enabled.
>>     >> yes.
>>     >>> 2. We will need to know the value kind in order to
parse the raw
>>     > profile
>>     >>>    data.
>>     >> Yes -- but we don't need to store value kind in order
to do that
>> --
>>     >> we
>>     simply need to organize the profile data more structurally.
>>     >>> 3. We'd like to avoid needing to change the
.profdata format
>> when
>> we
>>     > add
>>     >>>    new value kinds, since it needs to stay backwards
compatible.
>>     >>> 4. The frontend work is simpler if the value kind is
explicitly
>>     > encoded.
>>     >>> There are a couple of these points that certainly need
to be
>>     addressed,
>>     >>> and some that are a little bit less clear.
>>     >>> A. We do need to handle (1) in some way, but the issue
can be
>> stated a
>>     >>>    little bit more generally. If there are optional
parts of the
>>     > profile
>>     >>>    data, we definitely need to know which options are
enabled.
>>     >>> B. (1a) is only an issue if you believe (4). This
might be true
>> or
>>     >>> it
>>     > might
>>     >>>    not, and I don't think we can make an informed
decision until
>>     >>> we're
>>     actually using multiple value kinds. For now there's just a
bunch of
>>     >>>    code that's effectively useless - "if
value kind is 2, do
>> nothing".
>>     > I'm
>>     >>>    opposed to adding this kind of practically
unreachable code
>> "just
>>     in
>>     >>>    case". It adds a kind of complexity and
premature generality
>>     >>> that,
>>     > in my
>>     >>>    experience, will have to be completely re-done when
we get to
>>     > actually
>>     >>>    generalizing the code.
>>     >> In terms of value profiling code, there is no need for the
check
>> you
>>     mentioned "if value kind is ..." at all, but
'attached' to the
>> codegen
>>     code
>>     for language construct of interests, for instance,
>>     >> ... CodeGenFunction::EmitCall(...)
>>     >> {
>>     >>     if (enable_indirect_profile) {
>>     >>            ....profileIndirectCall();     // does both
>> instrumentation
>>     >> and profile annotation depending on the context.
>>     >>     }
>>     >>    ...
>>     >> }
>>     >> Organizing the profile data according value profile kind
allows
>>     >> profile
>>     data to be independently loaded and used:
>>     >> .. CodeGenPGO::assignRegionCounters (...) {
>>     >>    if (enable_indirect_profile) {
>>     >>      loadValueProfile(VK_indirect_call);
>>     >>    }
>>     >>    if (enable_xxx_profile) {
>>     >>     loadXXXProfile(VK_xxx);
>>     >>   }
>>     >> }
>>     >> if  we end up with inter-mixing all value kind profile
data, we
>> will
>>     have to load all value profile data into memory regardless of the
>>     option specified.   It also makes the profile data uses really hard
>> --
>>     how do we easily match the value profile site with the profile data
>>     counter
>>     index if profile kinds are selectively enabled?
>>     >> Besides, in the profile-use pass, each value kind's
in-memory
>> format
>>     >> of
>>     profile data may be slightly different, so the loader will have to
>> do
>>     different things ..
>>     >>> C. (1b) is an interesting one. Is this actually a
useful
>> feature?
>> If
>>     we
>>     >>>    want to combine multiple profiles with different
subsets of
>>     > profiling
>>     >>>    turned on, then we definitely need the information
recorded
>>     somewhere.
>>     >> I think it is a good flexibility to have.
>>     >>> D. I have issues with (2). If the different value
kinds actually
>> have
>>     >>>    different structures and need to be read in /
interpreted
>>     >>>    differently, then writing the support for them now
simply
>> won't
>>     work.
>>     >> We choose to have very compact form representing the raw
value
>> data
>>     >> for
>>     all value kinds, but to support efficient use of value profile of
>>     different
>>     kinds, the raw profile reader has to do something different here.
>> For
>>     instance, indirect call profile data needs the translation of the
>> raw
>>     address which other value kinds do not care.  I think this is
>> something we
>>     can do without.
>>     >>>    We're reading them in as if they're
interpreted in the exact
>> same
>>     way as the indirect call profiling - if they aren't, then we
have to
>>     rewrite
>>     this code when we add a new one anyway. What's the point of
writing
>> code
>>     that won't actually work when we try to use
>>     > it?
>>     >> I don't think this would be a problem as adding a new
kind
>> requires
>>     client/use side change too, otherwise we won't be able to use
it.
>>     > This is exactly my point. Why are we writing code *now* to
read
>> values
>>     that we have no way of using? This code *cannot* be tested, because
>> it
>>     doesn't do anything. It adds a bunch of loops to iterate over
values
>> that
>>     *must* be zero, because if they aren't we don't actually
know what
>> they
>>     mean.
>>     > The compiler-rt patches I've seen so far have all treated
each
>> value
>>     kind as if it should be treated identically to indirect call
target,
>> and
>>     had
>>     extra complexity so that they could do this, but you've stated
>> multiple
>>     times that this won't actually be correct when we use it. We
should
>> write
>>     the raw profile writer and reader code to do exactly what we need
it
>> to do
>>     today. We can and will change it when we want to add new
>> functionality.
>>
>>     Hi Justin,
>>
>>     Is value_kind the only holding factor keeping these CL's from
>> merging
>> in?
>>     If so, then I'm willing to do the change i.e. the writer and
reader
>> to
>> do
>>     exactly what IC profiling needs, as we'd like to have this
effort
>>     upstream.
>>     This will remove value_kind enum field from the source for now.
>>     The downside would be that the same enum will get reintroduced once
>> new
>>     value types are added in, and hence newer format updates to the
>> indexed
>>     readers.
>>
>>     If all are willing to accept the code in this way, I'll go
ahead and
>> take
>>     out the value_kind field. David, any objections?
>>
>>     Thanks,
>>     -Betul
>>
>>     >>> E. There is value in dealing with (3) as long as
we're confident
>>     >>> that
>>     > we
>>     >>>    can get it right. If we're pretty confident
that the value
>>     >>> profile
>>     data structure we're encoding in the .profdata file is going to
work
>>     >>>    for multiple value profile kinds, then it makes
sense to
>> allow
>>     multiple of them and encode which kinds they are. David, is
>>     "InstrProfValueRecord" in Betul's current patches
going to work to
>> record
>>     the other types of value profiling information you're
>>     > planning
>>     >>>    on looking at?
>>     >> The raw profile format is certainly good for other kinds.
>>     > I'm not talking about the raw profile format here. This
point is
>> about
>>     the stable, indexed format (.profdata, not .profraw). We have no
>> need
>> to
>>     keep the raw profile format backwards compatible, but we do need to
>> keep
>>     readers for old indexed format files.
>>     >> InstrProfValueRecord is mostly there (for instance
overloading
>> the
>>     >> Name
>>     field for other purpose, but this is minor and can be done as
follow
>> up
>>     when
>>     support of other kinds are added), so I am ok to leave it as it is
>> now.
>>     > What do you mean? What do you expect this structure to look
like
>> for
>>     other value kinds? Will we need to change the serialized format for
>> the
>>     indexed profile format?
>>     > There are two possibilities. Either:
>>     > 1. This serialized format is sufficient for other value kinds,
so
>> it
>> is
>>     >    valuable to encode it in the indexed profile format now,
>> because
>>     > this
>>     will avoid needing to change that format later.
>>     > Or,
>>     > 2. This is likely not how other value kinds will need to be
>> serialized,
>>     >    so we're going to need to change the format later
either way.
>> In
>>     > this
>>     case, we should just solve the problem we have *today*, rather than
>> making
>>     changes we'll need to undo later.
>>     > I think that given the uncertainty about what the future value
>> kinds'
>>     formats will be, we should encode this in a way where it's
obvious
>> what
>>     the
>>     extra data is for (we'll specify a kind in the data), but
without
>> trying
>>     to
>>     accomodate future value kinds until we're ready to talk about
them
>>     concretely.
>>     > I'll go into more detail about this in my review for the
indexed
>>     > profile
>>     reader and writer patch, which I should be able to get to tomorrow.
>>     >>> I'd also like to point out that the instrprof
parts of LLVM try
>> to
>>     >>> be
>>     relatively frontend agnostic. We should strive to keep it such that
>> the
>>     >>> frontend is the piece that decides what a particular
profiling
>> kind
>>     means if possible, as long as the structure and way the data is
laid
>>     > out
>>     >>> is suitable. This doesn't affect the current work
very much, but
>>     >>> it's
>>     a
>>     >>> good idea to keep it in mind.
>>     >> yes. See the above -- FE code does not really need to care
about
>> the
>>     > format.
>>     >>> So, with all of that said, I have a couple of
suggestions on how
>> to
>>     > move
>>     >>> forward.
>>     >>> - I don't think we should store the kind in the
raw profile yet.
>> Let's
>>     >>>   keep this simple, since it's easy to change and
will be easier
>> to
>>     get
>>     >>>   right when we're implementing or experimenting
with a second
>> kind
>>     (based on (D)).
>>     >> See my reply above -- we don't really store the value
kind in raw
>>     profile, but we need to organize the profile data according to the
>> kind in
>>     order to process (read) and use (profile-use) them
>>     >> efficiently.
>>     > The only reader of the raw profile is the tool that converts
it to
>> the
>>     indexed format, which is what profile-use consumes. The read will
>> always
>>     read the entire file in a streaming fashion, and efficiently
reading
>> parts
>>     of the file is not important. We then write this to the indexed
>> format,
>>     which needs to be efficiently readable.
>>     >>> - Assuming the answer to my question in (E) is
"yes", let's go
>> ahead
>>     > and
>>     >>>   include a value kind for the value records in the
.profdata
>> format.
>>     >> yes.
>>     >>>I
>>     >>>   don't really like the current approach of an
array of
>>     >>>std::vectors,
>>     because I think we can do this more simply. I'll bring that up
on
>> the
>>     >>>   review thread.
>>     >>> - In clang, we should error if we see value kinds we
don't
>> understand
>>     >>>   yet.
>>     >> yes.
>>     >>> I think that this addresses the short term concerns
without
>> forcing
>>     >>> us
>>     to write unreachable/untestable code that may or may not work for
>> the
>>     longer
>>     term concerns. It minimally handles (A ) in that the data that
>>     > is
>>     >>> there implies which features are being used, puts off
(B)
>> without
>>     > making
>>     >>> it harder to deal with when the time comes, allows us
to
>> implement
>>     >>> (C)
>>     without much trouble if we want it, avoids doing work that could
>> cause
>>     problems with (D), and handles (E) since avoiding it would mean
more
>> work
>>     later.
>>     >>> What do you think?
>>     >> See my reply above. For now, I think the things we need to
>> organize
>>     >> the
>>     value profile data in raw profile according to kind (not storing)
--
>> I
>>     think
>>     it will handle all the cases where you have concerns. thanks,
>>     >> David
>>     >>> Xinliang David Li <davidxl at google.com>
writes:
>>     >>>> Justin, do you have more concerns on keeping
value_kind?
>>     >>>> If there is still disagreement, can we agree to
move on with it
>> for
>>     now ?  After the initial version of the patches checked in, we can
>> do
>> more
>>     serious testings with large apps and revisit this if there are
>> problems
>>     discovered.
>>     >>>> thanks,
>>     >>>> David
>>     >>>> On Mon, Jun 15, 2015 at 10:47 PM, Xinliang David
Li
>>     >>>> <davidxl at google.com> wrote:
>>     >>>>>> But since the consumer is the frontend,
and it knows which
>> counts
>>     > are
>>     >>>>>> which, it knows the kind, no? I don't
understand how storing
>> the
>>     > kind
>>     >>>>>> info helps or hurts - it's just
redundant.
>>     >>>>> Yes, the frontend consumer knows, but the raw
reader does not.
>> It
>>     >>>>> is
>>     natural to do those processing when processing the raw profile data
>> (for
>>     instance when function pointer to name mapping still exists).
>>     >>>>>>>> The other thing we should do is
store which profiling
>> options
>>     >>>>>>>> are
>>     enabled, in both formats. When we specify profile-instr-use
it'll
>>     > be
>>     >>>>>>>> less error prone if we can detect
whether or not this
>> includes
>>     > indirect
>>     >>>>>>>> call profiling without checking
other options, and it's
>>     >>>>>>>> probably
>>     a
>>     > good
>>     >>>>>>>> idea for "llvm-profdata
merge" to disallow merging profiles
>>     >>>>>>>> that
>>     > are
>>     >>>>>>>> gathering different sets of data.
A bitfield seems suitable
>> for
>>     > this.
>>     >>>>>>> For value profiling, allowing
profile-gen and profile-use
>> passes
>>     > using
>>     >>>>>>> different options is a useful feature.
Consider the
>> following
>>     scenarios:
>>     >>>>>>> 1) collect the same profile data once
with all kinds of
>> value
>>     > profile
>>     >>>>>>>    data collected. The exact same
profile data can be used
>> in
>>     > performance
>>     >>>>>>>    experiments with different kinds of
value profiling
>>     > enabled/disabled
>>     >>>>>> This use case is pretty easy to accomodate
for with the model
>>     >>>>>> where
>>     > the
>>     >>>>>> profile stores its type. We could
autodetect the type to
>> decide
>>     >>>>>> how
>>     > to
>>     >>>>>> interpret the profile, and if more
specific profile flags are
>> set
>>     > simply
>>     >>>>>> ignore some of the data. The thing that
the file format
>> storing
>>     >>>>>> the
>>     > type
>>     >>>>>> gives us is that it's harder to
misinterpret the data by
>>     >>>>>> supplying
>>     > the
>>     >>>>>> mismatching flags between reading and
writing.
>>     >>>>> Yes, this is achievable with some extra effort
-- for instance
>> by
>>     collecting the profile sites of all kinds regardless of the flags.
>>     After profiling matching, simply drop selected sites according to
>> the
>>     >>>>> flags.
>>     >>>>> This adds complexity of the code. With value
kinds encoded in
>>     profile
>>     >>>>> data, the handling is much simpler -- each
value profiler only
>>     >>>>> needs
>>     to deal with its own kind.
>>     >>>>>>> 2) work around profile-use/value
transformation bug
>> selectively
>>     for
>>     >>>>>>>    some file without the need to
change anything in
>>     instrumentation
>>     > pass
>>     >>>>>> I'm not sure I understand what you
mean here.
>>     >>>>> Similar to the above, but for correctness.
>>     >>>>>>> Besides, with the latest patch, the
value_kind is not
>> recorded
>>     >>>>>>> in
>>     > each
>>     >>>>>>> profile value thus the overhead is
minimized.
>>     >>>>>> The overhead is low, sure, but the code
complexity of dealing
>>     >>>>>> with
>>     > the
>>     >>>>>> multiple kinds in this way is quite real.
>>     >>>>> I actually believe having value-kind can help
reduce code
>>     >>>>> complexity
>>     instead of the other way around. What is the extra complexity?
>>     thanks,
>>     >>>>> David
>>     >>>>>>Since I'm not convinced we're
>>     >>>>>> getting real benefit from storing the
kind, I don't believe
>> this
>>     trade-off is worth it.
>>     >>>>>>> thanks,
>>     >>>>>>> David
>

llvm dev - Jul 2015 - [LLVMdev] IC profiling infrastructure

[LLVMdev] IC profiling infrastructure

[LLVMdev] IC profiling infrastructure

[LLVMdev] IC profiling infrastructure

[LLVMdev] IC profiling infrastructure