[LLVMdev] IC profiling infrastructure

[Xinliang David Li]

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

I think the real question is not whether the related change is needed
now or not, but whether they are needed when more than one kind is
supported in the near future. If we predict it is needed, making
future change simpler with a good foundation is a good thing to have.

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

Can you be specific here? I don't see any indirect_call specific code
in there. The only thing related is to sum up the total number of
sites across all value kinds.

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

Which one of the statements about correctness issue is here?

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

Basically the Name field is specific to indirect call target. For
other value kind, we may need have a summary record for each site,
which is represented by the InstrProfValueRecord. The summary record
can be something like average etc -- but they are not strictly needed
either as they can be derived by the client who uses the profile data.
In short, we are strong reason to believe InstrProfValueRecord is
sufficient for other kinds.

>
> 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 believe 1 is the right way -- it will make the incremental cost of
adding new kind really low.

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

thanks,

David

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