[LLVMdev] IC profiling infrastructure
Justin Bogner
mail at justinbogner.com
Wed Jul 8 07:13:16 PDT 2015
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