Re: [PATCH PR77536]Generate correct profiling information for vectorized loop

["Bin.Cheng" <amker.cheng@gmail.com>]

On Mon, Feb 20, 2017 at 3:17 PM, Jan Hubicka <hubicka@ucw.cz> wrote:
>> > +  /* Without profile feedback, loops for which we do not know a better estimate
>> > +     are assumed to roll 10 times.  When we unroll such loop, it appears to
>> > +     roll too little, and it may even seem to be cold.  To avoid this, we
>> > +     ensure that the created loop appears to roll at least 5 times (but at
>> > +     most as many times as before unrolling).  */
>> > +  if (new_est_niter < 5)
>> > +    {
>> > +      if (est_niter < 5)
>> > +       new_est_niter = est_niter;
>> > +      else
>> > +       new_est_niter = 5;
>> > +    }
>> > +
>> > +  return new_est_niter;
>> > +}
>> >
>> > I see this code is pre-existing, but please extend it to test if
>> > loop->header->count is non-zero.  Even if we do not have idea about loop
>> > iteration count estimate we may end up predicting more than 10 iterations when
>> > predictors combine that way.
>> If I use expected_loop_iterations_unbounded, then do I need to handle
>> loop->header->count explicitly here?  I suppose not because it has
>> below code already:
>>
>>   /* If we have no profile at all, use AVG_LOOP_NITER.  */
>>   if (profile_status_for_fn (cfun) == PROFILE_ABSENT)
>>     expected = PARAM_VALUE (PARAM_AVG_LOOP_NITER);
>>   else if (loop->latch && (loop->latch->count || loop->header->count))
>>     {
>>       gcov_type count_in, count_latch;
>>       //...
>
> expected_loop_iterations_unbounded avoids capping the # of iterations comming
> from profile feedback (so if loop iterates 1000000 it retns 1000000 instead of
> 10000). Testing loop->header->count for non-zero tests if the loop was ever
> entered in the train run so it is easy test if you have any profile feedback
> for a given loop available.
BTW, if we use gcov_type in calculation from expected_loop_iterations_unbounded,
how should we adjust the number for calling scale_loop_frequencies
which has int type?  In extreme case, gcov_type could be out of int's
range, we have to cap the value anyway.  But yes, 10000 in
expect_loop_iterations is too small.
>
> I profile feedback is there, then making usre that new_est_niter is at least
> 5 will only make feedback less acurate.  I am not sure if we vectorize loop
> that will iterate only 1...4 times after unrolling, but I guess we could,
> and it would be desirable to peel it afterwards.
Right, I think we can discard this lower bound if profiling
information is present.
>>
>> The second question is: looks like it only takes latch->count into
>> consideration when PROFILE_ABSENT.  But according to your comments, we
>> could have nonzero count sometime?
>
> profile_status_for_fn (cfun) == PROFILE_ABSENT is set only when there is no
> profile esitmate at all - i.e. you compile with
> -fno-guess-branch-probabilities.  We don't really use that path very often
> but in that case you have no data to guess your profile from.
>
>>
>> >
>> > Perhaps testing estimated-loop_iterations would also make sense, but that
>> > could be dealt with incrementally.
>> >
>> > +static void
>> > +scale_profile_for_vect_loop (struct loop *loop, unsigned vf)
>> > +{
>> > +  unsigned freq_h = loop->header->frequency;
>> > +  unsigned freq_e = EDGE_FREQUENCY (loop_preheader_edge (loop));
>> > +  /* Reduce loop iterations by the vectorization factor.  */
>> > +  unsigned new_est_niter = niter_for_unrolled_loop (loop, vf);
>> > +
>> > +  if (freq_h != 0)
>> > +    scale_loop_frequencies (loop, freq_e * (new_est_niter + 1), freq_h);
>> > +
>> > I am always trying to avoid propagating small mistakes (i.e. frong freq_h or
>> > freq_h) into bigger mistakes (i.e. wrong profile of the whole loop) to avoid
>> > spreading mistakes across cfg.
>> >
>> > But I guess here it is sort of safe because vectorized loops are simple.
>> > You can't just scale down the existing counts/frequencies by vf, because the
>> > entry edge frequency was adjusted.
>> I am not 100% follow here, it looks the code avoids changing frequency
>> counter for preheader/exit edge, otherwise we would need to change all
>> counters dominated by them?
>
> I was just wondering what is wrong with taking the existing frequencies/counts
> the loop body has and dividing them all by the unroll factor.  This is correct
> if you ignore the versioning. With versioning I guess you want to scale by
> the unroll factor and also subtract frequencies/counts that was acocunted to
> the other versions of the loop, right?
IIUC, for (vectorized) loop header, it's frequency is calculated by:
          freq_header = freq_preheader + freq_latch
and freq_latch = (niter * freq_preheader).  Simply scaling it by VF gives:
          freq_header = (freq_preheader + freq_latch) / VF
which is wrong.  Especially if the loop is vectorized by large VF
(=16) and we normally assume niter (=10) without profiling
information, it results not only mismatch, but also
(loop->header->frequency < loop->preheader->frequency).  In fact, if
we have accurate niter information, the counter should be:
          freq_header = freq_preheader + niter * freq_preheader

>> >
>> > Also niter_for_unrolled_loop depends on sanity of the profile, so perhaps you
>> > need to compute it before you start chanigng the CFG by peeling proplogue?
>> Peeling for prologue doesn't change profiling information of
>> vect_loop, it is the skip edge from before loop to preferred epilogue
>> loop that will change profile counters.  I guess here exists a dilemma
>> that niter_for_unrolled_loop is for loop after peeling for prologue?
>
> expected_loop_iterations_unbounded calculates number of iteations by computing
> sum of frequencies of edges entering the loop and comparing it to the frequency
> of loop header.  While peeling the prologue, you split the preheader edge and
> adjust frequency of the new preheader BB of the loop to be vectorized.  I think
> that will adjust the #of iterations estimate.
It's not the case now I think.  one motivation of new vect_do_peeling
is to avoid niter checks between prologue and vector loop.  Once
prologue loop is entered or checked, the vector loop must be executed
unconditionally.  So the preheaderof vector loop has consistent
frequency counters now.  The niter check on whether vector loop should
be executed is now merged with cost check before prologue, and in the
future I think this can be further merged if loop versioning is
needed.

Thanks,
bin
>
> Honza
>>
>> Thanks,
>> bin
>> >
>> > Finally if freq_e is 0, all frequencies and counts will be probably dropped to
>> > 0.  What about determining fraction by counts if they are available?
>> >
>> > Otherwise the patch looks good and thanks a lot for working on this!
>> >
>> > Honza
>> >
>> >> >
>> >> > gcc/testsuite/ChangeLog
>> >> > 2017-02-16  Bin Cheng  <bin.cheng@arm.com>
>> >> >
>> >> >         PR tree-optimization/77536
>> >> >         * gcc.dg/vect/pr79347.c: Revise testing string.