<div dir="ltr">Yes, that's what I meant.</div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 12, 2017 at 4:14 PM, Xin Tong <span dir="ltr"><<a href="mailto:trent.xin.tong@gmail.com" target="_blank">trent.xin.tong@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Thu, Jan 12, 2017 at 4:09 PM, Xin Tong <<a href="mailto:trent.xin.tong@gmail.com">trent.xin.tong@gmail.com</a>> wrote:<br>
> On Thu, Jan 12, 2017 at 3:37 PM, Michael Kuperstein <<a href="mailto:mkuper@google.com">mkuper@google.com</a>> wrote:<br>
>><br>
>><br>
>> On Thu, Jan 12, 2017 at 2:55 PM, Xin Tong <<a href="mailto:trent.xin.tong@gmail.com">trent.xin.tong@gmail.com</a>> wrote:<br>
>>><br>
>>> On Thu, Jan 12, 2017 at 2:40 PM, Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>> wrote:<br>
>>> > Are you suggesting changing it to:<br>
>>> ><br>
>>> > rotate_update_probability(<wbr>float orig_prob) {<br>
>>> >   if (orig_prob >= 0 && orig_prob < 0.5) {<br>
>>> >     branch_prob_inside = 0;<br>
>>> >     branch_prob_outside = orig_prob;<br>
>>> >   } else {<br>
>>> >     branch_prob_inside = 2 - 1/orig_prob;<br>
>>> >     branch_prob_outside = 1;<br>
>>> >   }<br>
>>> > }<br>
>>><br>
>>> This would not work for the case which the orig_prob is bigger than<br>
>>> 0.5. branch_prob_outside tells me when the guard block is hit, it will<br>
>>> always go to the exit.<br>
>>> ><br>
>>> > int estimate_trip_count(float latch_probability) {<br>
>>> >    if (latch_probability == 0)<br>
>>> >      return 0;<br>
>>> >    else<br>
>>> >      return 1/(1-latch_probability)<br>
>>> > }<br>
>>> ><br>
>>> > My gut feeling tells me this is a little dangerous. Micheal, what do you<br>
>>> > think?<br>
>>> ><br>
>>> > Dehao<br>
>>><br>
>>> My general feeling is.<br>
>>><br>
>>> 1. We do not have all the information to do a very precise<br>
>>> distribution of the weight, and we will unlikely to be able to get<br>
>>> such information in short term (we need value profiling at least I<br>
>>> think or some other more clever approach). we made a decision to use<br>
>>> average trip count for loop peeling, assuming the loop tripcounts are<br>
>>> not polarized. Loop peeling will probably not be as effective if it<br>
>>> is.  So we make the same assumption when we rotate the loops and<br>
>>> update the !prof metadata.<br>
>><br>
>><br>
>> I agree.<br>
>><br>
>>><br>
>>> 2. We also consider the possibility of this assumption falling out, in<br>
>>> the case of loop peel, we basically peeled for some gains when the<br>
>>> iteration has a high trip count and some loss  (increase code size)<br>
>>> when the iterations has low trip count. I feel this is acceptable.  In<br>
>>> case of metadata update in loop rotation, if loop does have a very<br>
>>> polarized distribution. i.e. cases where a few iterations takes all<br>
>>> the trip counts and rest have close to zero. We basically<br>
>>> underestimate the weight of the branch from guard block to exit block,<br>
>>> as there should be more iterations of the loop that exits before even<br>
>>> one iteration is executed. This will propagate into the CFG and result<br>
>>> in us underestimating the average tripcount of the loops when it<br>
>>> actually executes (i.e. we have fewer iterations responsible for all<br>
>>> these body weight). This is OK, we become less aggressive in peeling,<br>
>>> we left some performance opportunities on the table, but only if the<br>
>>> loop distribution is polarized. And we may do a slightly worse job in<br>
>>> block placement.<br>
>><br>
>><br>
>> That's not exactly right. Underestimating can make us more aggressive, not<br>
>> less.<br>
>> Because we use a hard threshold for when to peel, underestimating the trip<br>
>> count<br>
>> can cause us to be more aggressive. I'm not sure this is a big problem,<br>
>> because<br>
>> if the threshold is chosen correctly, choosing to peel or not to peel when<br>
>> we're very<br>
>> close to the threshold should be a wash.<br>
>> Admittedly, that's a pretty big if. :-) But I don't think it's a huge<br>
>> problem.<br>
><br>
> If the actual tripcount of the loop is 10 and we estimated to be 5, we<br>
> will peel it 5 iterations.<br>
> Its only when the<br>
> <a href="https://github.com/llvm-mirror/llvm/blob/master/lib/Transforms/Utils/LoopUnrollPeel.cpp#L95" rel="noreferrer" target="_blank">https://github.com/llvm-<wbr>mirror/llvm/blob/master/lib/<wbr>Transforms/Utils/<wbr>LoopUnrollPeel.cpp#L95</a><br>
> is false,  we do not peel, then if the underestimation breaks this<br>
> condition, the real number (bigger) will sure not cause the loop to be<br>
> peeled.<br>
><br>
> i.e. we will never peel more than this threshold. so I feel we are<br>
> leaving opportunities on the table, instead of being too<br>
> aggressive.<br>
><br>
> -Xin<br>
<br>
</div></div>Ok, there is also the possiblity that we underestimate and peel a loop<br>
which we would not peel given the right tripcount.  I think thats what<br>
you meant.<br>
<div class="HOEnZb"><div class="h5">-Xin<br>
>><br>
>> Dehao, why do you think this is dangerous? It's certainly somewhat ugly,<br>
>> but that's a separate issue.<br>
>><br>
>>><br>
>>> 3. In case the loop distribution is close to each other, the patch<br>
>>> handles it and distributes the weight in a reasonable way I feel.<br>
>>><br>
>>> -Xin<br>
>>> ><br>
>>> > On Thu, Jan 12, 2017 at 2:33 PM, Xin Tong <<a href="mailto:trent.xin.tong@gmail.com">trent.xin.tong@gmail.com</a>><br>
>>> > wrote:<br>
>>> >><br>
>>> >> On Thu, Jan 12, 2017 at 2:19 PM, Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>><br>
>>> >> wrote:<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > On Thu, Jan 12, 2017 at 2:14 PM, Xin Tong <<a href="mailto:trent.xin.tong@gmail.com">trent.xin.tong@gmail.com</a>><br>
>>> >> > wrote:<br>
>>> >> >><br>
>>> >> >> On Thu, Jan 12, 2017 at 2:10 PM, Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>><br>
>>> >> >> wrote:<br>
>>> >> >> ><br>
>>> >> >> ><br>
>>> >> >> > On Thu, Jan 12, 2017 at 2:02 PM, Xin Tong<br>
>>> >> >> > <<a href="mailto:trent.xin.tong@gmail.com">trent.xin.tong@gmail.com</a>><br>
>>> >> >> > wrote:<br>
>>> >> >> >><br>
>>> >> >> >> On Thu, Jan 12, 2017 at 1:34 PM, Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>><br>
>>> >> >> >> wrote:<br>
>>> >> >> >> > If we assume that the trip count will always be the same, then<br>
>>> >> >> >> > if<br>
>>> >> >> >> > back<br>
>>> >> >> >> > edge<br>
>>> >> >> >> > taken probability falls in the range of (0, 0.5), then it<br>
>>> >> >> >> > already<br>
>>> >> >> >> > broke<br>
>>> >> >> >> > this<br>
>>> >> >> >> > assumption. So we do not have enough info to infer both branch<br>
>>> >> >> >> > probabilities. So probably we should simply set the inner<br>
>>> >> >> >> > probability<br>
>>> >> >> >> > as<br>
>>> >> >> >> > "0". And when we estimate loop trip count, we will always<br>
>>> >> >> >> > return 0<br>
>>> >> >> >> > if<br>
>>> >> >> >> > the<br>
>>> >> >> >> > latch probability is 0. Something like:<br>
>>> >> >> >> ><br>
>>> >> >> >> > rotate_update_probability(<wbr>float orig_prob) {<br>
>>> >> >> >> >   branch_prob_outside = orig_prob;<br>
>>> >> >> >> >   if (orig_prob >= 0 && orig_prob < 0.5) {<br>
>>> >> >> >> >     branch_prob_inside = 0;<br>
>>> >> >> >> >   } else {<br>
>>> >> >> >> >     branch_prob_inside = 2 - 1/orig_prob;<br>
>>> >> >> >> >   }<br>
>>> >> >> >> > }<br>
>>> >> >> >> ><br>
>>> >> >> >> > int estimate_trip_count(float latch_probability) {<br>
>>> >> >> >> >    if (latch_probability == 0)<br>
>>> >> >> >> >      return 0;<br>
>>> >> >> >> >    else<br>
>>> >> >> >> >      return 1/(1-latch_probability)<br>
>>> >> >> >> > }<br>
>>> >> >> >><br>
>>> >> >> >> We assume the trip count will be close to each other, they do not<br>
>>> >> >> >> have<br>
>>> >> >> >> to be identical =). And that assumption has implications, I<br>
>>> >> >> >> addressed<br>
>>> >> >> >> the implications when we have polarized loop tripcounts in an<br>
>>> >> >> >> earlier<br>
>>> >> >> >> response. I feel we need to make assumptions and also reason<br>
>>> >> >> >> about<br>
>>> >> >> >> the<br>
>>> >> >> >> fallouts if that assumption turns out to be not true as Michael<br>
>>> >> >> >> said.<br>
>>> >> >> ><br>
>>> >> >> ><br>
>>> >> >> > Yes, the above code is trying to address one case where the<br>
>>> >> >> > assumption<br>
>>> >> >> > is<br>
>>> >> >> > untrue.<br>
>>> >> >> ><br>
>>> >> >> >><br>
>>> >> >> >><br>
>>> >> >> >> Back to your example, I am not sure i understand it fully, is<br>
>>> >> >> >> orig_prob the probability of branching outside before rotation ?<br>
>>> >> >> ><br>
>>> >> >> ><br>
>>> >> >> > Yes, orig_prob is the original probability of the branch before<br>
>>> >> >> > loop<br>
>>> >> >> > rotation.<br>
>>> >> >><br>
>>> >> >> If orig_prob == 0.5, the branch_prob_inside is 0.<br>
>>> >> >> (branch_prob_inside<br>
>>> >> >> is the probability that the loop body is branched to before rotation<br>
>>> >> >> ?).<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > Sorry about the confusion. Let me use my original example:<br>
>>> >> ><br>
>>> >> > Before loop rotation<br>
>>> >> ><br>
>>> >> > L1:<br>
>>> >> > if (cond) { // "orig_prob"<br>
>>> >> >   stmt;<br>
>>> >> >   goto L1;<br>
>>> >> > }<br>
>>> >> ><br>
>>> >> > After loop rotation:<br>
>>> >> ><br>
>>> >> > if (cond) { // "branch_prob_outside"<br>
>>> >> > L1:<br>
>>> >> >   stmt;<br>
>>> >> >   if (cond) { // "branch_prob_inside" // and also "latch_probability"<br>
>>> >> >     goto L1;<br>
>>> >> >   }<br>
>>> >> > }<br>
>>> >> ><br>
>>> >> > Note that all probability are for when "cond" is true.<br>
>>> >> ><br>
>>> >> > Thanks,<br>
>>> >> > Dehao<br>
>>> >><br>
>>> >> Thanks for the code. One thing I notice is that branch_prob_outside<br>
>>> >> probably should not be orig_prob. orig_prob before rotation is the (#<br>
>>> >> of times the exit is taken)/(# of times the header branch is hit<br>
>>> >> through forward and back edges). So if the loop has a cmp i32 %index,<br>
>>> >> 127 and recurrence of +1 (index = index + 1), i.e. the loop has a trip<br>
>>> >> count of 128, then branch_prob_outside = orig_prob = 1/128.  However,<br>
>>> >> branch_prob_outside should really be 0 as the loop will always<br>
>>> >> execute, i..e branch_prob_outside in the guard block will never be<br>
>>> >> taken.<br>
>>> >> ><br>
>>> >> ><br>
>>> >> >><br>
>>> >> >> Thanks,<br>
>>> >> >> -Xin<br>
>>> >> >> ><br>
>>> >> >> >><br>
>>> >> >> >><br>
>>> >> >> >> Thanks,<br>
>>> >> >> >> -Xin<br>
>>> >> >> >><br>
>>> >> >> >> ><br>
>>> >> >> >> ><br>
>>> >> >> >> > On Thu, Jan 12, 2017 at 12:59 PM, Michael Kuperstein via<br>
>>> >> >> >> > Phabricator<br>
>>> >> >> >> > <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
>>> >> >> >> >><br>
>>> >> >> >> >> mkuper added inline comments.<br>
>>> >> >> >> >><br>
>>> >> >> >> >><br>
>>> >> >> >> >> ================<br>
>>> >> >> >> >> Comment at: lib/Transforms/Scalar/<wbr>LoopRotation.cpp:476<br>
>>> >> >> >> >> +  // data.<br>
>>> >> >> >> >> +  if (!<wbr>updateLoopEstimatedBranchWeigh<wbr>t(L, GBI, BI /*<br>
>>> >> >> >> >> OrigHeader<br>
>>> >> >> >> >> BR<br>
>>> >> >> >> >> */)) {<br>
>>> >> >> >> >> +    BI->setMetadata(LLVMContext::<wbr>MD_prof, nullptr);<br>
>>> >> >> >> >> ----------------<br>
>>> >> >> >> >> Also, regardless of the rest of the discussion - I don't think<br>
>>> >> >> >> >> we<br>
>>> >> >> >> >> should<br>
>>> >> >> >> >> drop the metadata on the floor if we fail.<br>
>>> >> >> >> >><br>
>>> >> >> >> >> I don't think "No data is better than imprecise data" is right<br>
>>> >> >> >> >> in<br>
>>> >> >> >> >> the<br>
>>> >> >> >> >> general case, but that's arguable. Specifically here, though,<br>
>>> >> >> >> >> we're<br>
>>> >> >> >> >> imprecise even if updateLoopEstimatedBranchWeigh<wbr>t() succeeds,<br>
>>> >> >> >> >> because<br>
>>> >> >> >> >> of the<br>
>>> >> >> >> >> assumptions we make on the distribution.<br>
>>> >> >> >> >><br>
>>> >> >> >> >><br>
>>> >> >> >> >> <a href="https://reviews.llvm.org/D28593" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28593</a><br>
>>> >> >> >> >><br>
>>> >> >> >> >><br>
>>> >> >> >> >><br>
>>> >> >> >> ><br>
>>> >> >> ><br>
>>> >> >> ><br>
>>> >> ><br>
>>> >> ><br>
>>> ><br>
>>> ><br>
>><br>
>><br>
</div></div></blockquote></div><br></div>