[LLVMdev] IC profiling infrastructure

Xinliang David Li davidxl at google.com
Wed Jul 8 11:06:59 PDT 2015


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



More information about the llvm-dev mailing list