IC profiling infrastructure

Xinliang David Li <davidxl@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.

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

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.

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

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

Ping?

Xinliang David Li <davidxl@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.

Xinliang David Li <davidxl@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

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

Thanks,
-Betul