<p dir="ltr">My suggestion:</p>
<p dir="ltr">1. Keep the value kind enum, but remove the reserved kinds<br>
2. In compiler-rt code, assert the kind to be icalltarget and remove the loop<br>
3. Keep the indexed format (as is now)<br>
4. Add assertion in profile use that icalltarget is the only expected kind.</p>
<p dir="ltr">Justin, does this sound reasonable?</p>
<p dir="ltr">David</p>
<div class="gmail_quote">On Jul 2, 2015 1:10 PM, "Betul Buyukkurt" <<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>> wrote:<br type="attribution"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Any decision on below? Is everyone OK w/ removing the value_kind and keeping<br>
the raw profile writer and reader code to do exactly what we need it to do<br>
today. Justin, if you agree, I'll make/upload the changes to right away?<br>
<br>
Thanks,<br>
-Betul<br>
-----Original Message-----<br>
From: Betul Buyukkurt [mailto:<a href="mailto:betulb@codeaurora.org">betulb@codeaurora.org</a>]<br>
Sent: Tuesday, June 30, 2015 4:18 PM<br>
To: Justin Bogner<br>
Cc: Xinliang David Li; Betul Buyukkurt; llvmdev<br>
Subject: Re: [LLVMdev] IC profiling infrastructure<br>
<br>
> Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> writes:<br>
>> Justin, thanks for the reply.<br>
>> I would like to point out that value_kind is actually not really<br>
>> stored<br>
in the raw profile data (nor do we intend to do so), other than the minimal<br>
information about per value kind NumOfSites info in per-function profile<br>
header. Basically the data is organized per value kind instead of stored<br>
intermixed. That is about it.<br>
>> More replies below.<br>
>> On Mon, Jun 29, 2015 at 11:53 AM, Justin Bogner<br>
>> <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>><br>
> wrote:<br>
>>> I still have some issues with what we're doing with value_kind here.<br>
> Let<br>
>>> me summarize the arguments I've heard for including the value_kind<br>
>>> in<br>
various places in this set of changes:<br>
>>> 1. We would like to be able to enable multiple types of value<br>
profiling<br>
>>> at the same time.<br>
>>> 1a. We would like to be able to selectively use only particular value<br>
>>> kinds from a particular profile.<br>
>>> 1b. We would like to be able to combine profiles with different sets<br>
of<br>
>>> profiling options enabled.<br>
>> yes.<br>
>>> 2. We will need to know the value kind in order to parse the raw<br>
> profile<br>
>>> data.<br>
>> Yes -- but we don't need to store value kind in order to do that --<br>
>> we<br>
simply need to organize the profile data more structurally.<br>
>>> 3. We'd like to avoid needing to change the .profdata format when we<br>
> add<br>
>>> new value kinds, since it needs to stay backwards compatible.<br>
>>> 4. The frontend work is simpler if the value kind is explicitly<br>
> encoded.<br>
>>> There are a couple of these points that certainly need to be<br>
addressed,<br>
>>> and some that are a little bit less clear.<br>
>>> A. We do need to handle (1) in some way, but the issue can be stated a<br>
>>> little bit more generally. If there are optional parts of the<br>
> profile<br>
>>> data, we definitely need to know which options are enabled.<br>
>>> B. (1a) is only an issue if you believe (4). This might be true or<br>
>>> it<br>
> might<br>
>>> not, and I don't think we can make an informed decision until<br>
>>> we're<br>
actually using multiple value kinds. For now there's just a bunch of<br>
>>> code that's effectively useless - "if value kind is 2, do nothing".<br>
> I'm<br>
>>> opposed to adding this kind of practically unreachable code "just<br>
in<br>
>>> case". It adds a kind of complexity and premature generality<br>
>>> that,<br>
> in my<br>
>>> experience, will have to be completely re-done when we get to<br>
> actually<br>
>>> generalizing the code.<br>
>> In terms of value profiling code, there is no need for the check you<br>
mentioned "if value kind is ..." at all, but 'attached' to the codegen code<br>
for language construct of interests, for instance,<br>
>> ... CodeGenFunction::EmitCall(...)<br>
>> {<br>
>> if (enable_indirect_profile) {<br>
>> ....profileIndirectCall(); // does both instrumentation<br>
>> and profile annotation depending on the context.<br>
>> }<br>
>> ...<br>
>> }<br>
>> Organizing the profile data according value profile kind allows<br>
>> profile<br>
data to be independently loaded and used:<br>
>> .. CodeGenPGO::assignRegionCounters (...) {<br>
>> if (enable_indirect_profile) {<br>
>> loadValueProfile(VK_indirect_call);<br>
>> }<br>
>> if (enable_xxx_profile) {<br>
>> loadXXXProfile(VK_xxx);<br>
>> }<br>
>> }<br>
>> if we end up with inter-mixing all value kind profile data, we will<br>
have to load all value profile data into memory regardless of the<br>
option specified. It also makes the profile data uses really hard --<br>
how do we easily match the value profile site with the profile data counter<br>
index if profile kinds are selectively enabled?<br>
>> Besides, in the profile-use pass, each value kind's in-memory format<br>
>> of<br>
profile data may be slightly different, so the loader will have to do<br>
different things ..<br>
>>> C. (1b) is an interesting one. Is this actually a useful feature? If<br>
we<br>
>>> want to combine multiple profiles with different subsets of<br>
> profiling<br>
>>> turned on, then we definitely need the information recorded<br>
somewhere.<br>
>> I think it is a good flexibility to have.<br>
>>> D. I have issues with (2). If the different value kinds actually have<br>
>>> different structures and need to be read in / interpreted<br>
>>> differently, then writing the support for them now simply won't work.<br>
>> We choose to have very compact form representing the raw value data<br>
>> for<br>
all value kinds, but to support efficient use of value profile of different<br>
kinds, the raw profile reader has to do something different here. For<br>
instance, indirect call profile data needs the translation of the raw<br>
address which other value kinds do not care. I think this is something we<br>
can do without.<br>
>>> We're reading them in as if they're interpreted in the exact same<br>
way as the indirect call profiling - if they aren't, then we have to rewrite<br>
this code when we add a new one anyway. What's the point of writing code<br>
that won't actually work when we try to use<br>
> it?<br>
>> I don't think this would be a problem as adding a new kind requires<br>
client/use side change too, otherwise we won't be able to use it.<br>
> This is exactly my point. Why are we writing code *now* to read values<br>
that we have no way of using? This code *cannot* be tested, because it<br>
doesn't do anything. It adds a bunch of loops to iterate over values that<br>
*must* be zero, because if they aren't we don't actually know what they<br>
mean.<br>
> The compiler-rt patches I've seen so far have all treated each value<br>
kind as if it should be treated identically to indirect call target, and had<br>
extra complexity so that they could do this, but you've stated multiple<br>
times that this won't actually be correct when we use it. We should write<br>
the raw profile writer and reader code to do exactly what we need it to do<br>
today. We can and will change it when we want to add new functionality.<br>
<br>
Hi Justin,<br>
<br>
Is value_kind the only holding factor keeping these CL's from merging in?<br>
If so, then I'm willing to do the change i.e. the writer and reader to do<br>
exactly what IC profiling needs, as we'd like to have this effort upstream.<br>
This will remove value_kind enum field from the source for now.<br>
The downside would be that the same enum will get reintroduced once new<br>
value types are added in, and hence newer format updates to the indexed<br>
readers.<br>
<br>
If all are willing to accept the code in this way, I'll go ahead and take<br>
out the value_kind field. David, any objections?<br>
<br>
Thanks,<br>
-Betul<br>
<br>
>>> E. There is value in dealing with (3) as long as we're confident<br>
>>> that<br>
> we<br>
>>> can get it right. If we're pretty confident that the value<br>
>>> profile<br>
data structure we're encoding in the .profdata file is going to work<br>
>>> for multiple value profile kinds, then it makes sense to allow<br>
multiple of them and encode which kinds they are. David, is<br>
"InstrProfValueRecord" in Betul's current patches going to work to record<br>
the other types of value profiling information you're<br>
> planning<br>
>>> on looking at?<br>
>> The raw profile format is certainly good for other kinds.<br>
> I'm not talking about the raw profile format here. This point is about<br>
the stable, indexed format (.profdata, not .profraw). We have no need to<br>
keep the raw profile format backwards compatible, but we do need to keep<br>
readers for old indexed format files.<br>
>> InstrProfValueRecord is mostly there (for instance overloading the<br>
>> Name<br>
field for other purpose, but this is minor and can be done as follow up when<br>
support of other kinds are added), so I am ok to leave it as it is now.<br>
> What do you mean? What do you expect this structure to look like for<br>
other value kinds? Will we need to change the serialized format for the<br>
indexed profile format?<br>
> There are two possibilities. Either:<br>
> 1. This serialized format is sufficient for other value kinds, so it is<br>
> valuable to encode it in the indexed profile format now, because<br>
> this<br>
will avoid needing to change that format later.<br>
> Or,<br>
> 2. This is likely not how other value kinds will need to be serialized,<br>
> so we're going to need to change the format later either way. In<br>
> this<br>
case, we should just solve the problem we have *today*, rather than making<br>
changes we'll need to undo later.<br>
> I think that given the uncertainty about what the future value kinds'<br>
formats will be, we should encode this in a way where it's obvious what the<br>
extra data is for (we'll specify a kind in the data), but without trying to<br>
accomodate future value kinds until we're ready to talk about them<br>
concretely.<br>
> I'll go into more detail about this in my review for the indexed<br>
> profile<br>
reader and writer patch, which I should be able to get to tomorrow.<br>
>>> I'd also like to point out that the instrprof parts of LLVM try to<br>
>>> be<br>
relatively frontend agnostic. We should strive to keep it such that the<br>
>>> frontend is the piece that decides what a particular profiling kind<br>
means if possible, as long as the structure and way the data is laid<br>
> out<br>
>>> is suitable. This doesn't affect the current work very much, but<br>
>>> it's<br>
a<br>
>>> good idea to keep it in mind.<br>
>> yes. See the above -- FE code does not really need to care about the<br>
> format.<br>
>>> So, with all of that said, I have a couple of suggestions on how to<br>
> move<br>
>>> forward.<br>
>>> - I don't think we should store the kind in the raw profile yet. Let's<br>
>>> keep this simple, since it's easy to change and will be easier to<br>
get<br>
>>> right when we're implementing or experimenting with a second kind<br>
(based on (D)).<br>
>> See my reply above -- we don't really store the value kind in raw<br>
profile, but we need to organize the profile data according to the kind in<br>
order to process (read) and use (profile-use) them<br>
>> efficiently.<br>
> The only reader of the raw profile is the tool that converts it to the<br>
indexed format, which is what profile-use consumes. The read will always<br>
read the entire file in a streaming fashion, and efficiently reading parts<br>
of the file is not important. We then write this to the indexed format,<br>
which needs to be efficiently readable.<br>
>>> - Assuming the answer to my question in (E) is "yes", let's go ahead<br>
> and<br>
>>> include a value kind for the value records in the .profdata format.<br>
>> yes.<br>
>>>I<br>
>>> don't really like the current approach of an array of<br>
>>>std::vectors,<br>
because I think we can do this more simply. I'll bring that up on the<br>
>>> review thread.<br>
>>> - In clang, we should error if we see value kinds we don't understand<br>
>>> yet.<br>
>> yes.<br>
>>> I think that this addresses the short term concerns without forcing<br>
>>> us<br>
to write unreachable/untestable code that may or may not work for the longer<br>
term concerns. It minimally handles (A ) in that the data that<br>
> is<br>
>>> there implies which features are being used, puts off (B) without<br>
> making<br>
>>> it harder to deal with when the time comes, allows us to implement<br>
>>> (C)<br>
without much trouble if we want it, avoids doing work that could cause<br>
problems with (D), and handles (E) since avoiding it would mean more work<br>
later.<br>
>>> What do you think?<br>
>> See my reply above. For now, I think the things we need to organize<br>
>> the<br>
value profile data in raw profile according to kind (not storing) -- I think<br>
it will handle all the cases where you have concerns. thanks,<br>
>> David<br>
>>> Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> writes:<br>
>>>> Justin, do you have more concerns on keeping value_kind?<br>
>>>> If there is still disagreement, can we agree to move on with it for<br>
now ? After the initial version of the patches checked in, we can do more<br>
serious testings with large apps and revisit this if there are problems<br>
discovered.<br>
>>>> thanks,<br>
>>>> David<br>
>>>> On Mon, Jun 15, 2015 at 10:47 PM, Xinliang David Li<br>
>>>> <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br>
>>>>>> But since the consumer is the frontend, and it knows which counts<br>
> are<br>
>>>>>> which, it knows the kind, no? I don't understand how storing the<br>
> kind<br>
>>>>>> info helps or hurts - it's just redundant.<br>
>>>>> Yes, the frontend consumer knows, but the raw reader does not. It<br>
>>>>> is<br>
natural to do those processing when processing the raw profile data (for<br>
instance when function pointer to name mapping still exists).<br>
>>>>>>>> The other thing we should do is store which profiling options<br>
>>>>>>>> are<br>
enabled, in both formats. When we specify profile-instr-use it'll<br>
> be<br>
>>>>>>>> less error prone if we can detect whether or not this includes<br>
> indirect<br>
>>>>>>>> call profiling without checking other options, and it's<br>
>>>>>>>> probably<br>
a<br>
> good<br>
>>>>>>>> idea for "llvm-profdata merge" to disallow merging profiles<br>
>>>>>>>> that<br>
> are<br>
>>>>>>>> gathering different sets of data. A bitfield seems suitable for<br>
> this.<br>
>>>>>>> For value profiling, allowing profile-gen and profile-use passes<br>
> using<br>
>>>>>>> different options is a useful feature. Consider the following<br>
scenarios:<br>
>>>>>>> 1) collect the same profile data once with all kinds of value<br>
> profile<br>
>>>>>>> data collected. The exact same profile data can be used in<br>
> performance<br>
>>>>>>> experiments with different kinds of value profiling<br>
> enabled/disabled<br>
>>>>>> This use case is pretty easy to accomodate for with the model<br>
>>>>>> where<br>
> the<br>
>>>>>> profile stores its type. We could autodetect the type to decide<br>
>>>>>> how<br>
> to<br>
>>>>>> interpret the profile, and if more specific profile flags are set<br>
> simply<br>
>>>>>> ignore some of the data. The thing that the file format storing<br>
>>>>>> the<br>
> type<br>
>>>>>> gives us is that it's harder to misinterpret the data by<br>
>>>>>> supplying<br>
> the<br>
>>>>>> mismatching flags between reading and writing.<br>
>>>>> Yes, this is achievable with some extra effort -- for instance by<br>
collecting the profile sites of all kinds regardless of the flags.<br>
After profiling matching, simply drop selected sites according to the<br>
>>>>> flags.<br>
>>>>> This adds complexity of the code. With value kinds encoded in<br>
profile<br>
>>>>> data, the handling is much simpler -- each value profiler only<br>
>>>>> needs<br>
to deal with its own kind.<br>
>>>>>>> 2) work around profile-use/value transformation bug selectively<br>
for<br>
>>>>>>> some file without the need to change anything in<br>
instrumentation<br>
> pass<br>
>>>>>> I'm not sure I understand what you mean here.<br>
>>>>> Similar to the above, but for correctness.<br>
>>>>>>> Besides, with the latest patch, the value_kind is not recorded<br>
>>>>>>> in<br>
> each<br>
>>>>>>> profile value thus the overhead is minimized.<br>
>>>>>> The overhead is low, sure, but the code complexity of dealing<br>
>>>>>> with<br>
> the<br>
>>>>>> multiple kinds in this way is quite real.<br>
>>>>> I actually believe having value-kind can help reduce code<br>
>>>>> complexity<br>
instead of the other way around. What is the extra complexity?<br>
thanks,<br>
>>>>> David<br>
>>>>>>Since I'm not convinced we're<br>
>>>>>> getting real benefit from storing the kind, I don't believe this<br>
trade-off is worth it.<br>
>>>>>>> thanks,<br>
>>>>>>> David<br>
<br>
<br>
<br>
<br>
<br>
</blockquote></div>