[LLVMdev] IC profiling infrastructure
Betul Buyukkurt
betulb at codeaurora.org
Wed Jul 8 11:37:45 PDT 2015
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
>
More information about the llvm-dev
mailing list