Re: [PATCH 4/19]middle-end: Fix scale_loop_frequencies segfault on multiple-exits

[Jan Hubicka <hubicka@ucw.cz>]

> On Wed, 28 Jun 2023, Tamar Christina wrote:
> 
> > Hi All,
> > 
> > There's an existing bug in loop frequency scaling where the if statement checks
> > to see if there's a single exit, and records an dump file note but then
> > continues.
> > 
> > It then tries to access the null pointer, which of course fails.
> > 
> > For multiple loop exists it's not really clear how to scale the exit
> > probablities as it's really unknown which exit is most probably.
> > 
> > For that reason I ignore the exit edges during scaling but still adjust the
> > loop body.
> > 
> > Bootstrapped Regtested on aarch64-none-linux-gnu and no issues.
> > 
> > Ok for master?
> 
> I can't really make sense of
> 
>       /* If latch exists, change its count, since we changed
>          probability of exit.  Theoretically we should update everything 
> from
>          source of exit edge to latch, but for vectorizer this is enough.  
> */
>       if (loop->latch && loop->latch != e->src)
>         loop->latch->count += count_delta;
> 
> since with simple latches the latch itself is an empty forwarder and
> e->src is the block with the conditional eventually exiting the block.
> That means this condition is always true.
> 
> So I think for exits the idea is to "remove" them by redirecting
> them "count-wise" back into the loop.  So turn
> 
>    if (cond) --(exit-count)-- exit
>      |
>      | in-loop-count
>      |
>    latch
> 
> into
> 
>    [cond-blk-count]
>    if (cond) -- (zero count) -- exit
>      |
>      | in-loop-cound + exit-count (== cond-blk-count)
>      |
>    latch (now with cond-blk-count)

This is oposite situation.  You have loop predicted to iterate 10 times,
but you found it actually iterates at most twice.  So you want to
 1) scale down profile of every BB in the loop
    so header is 2*sum_of_counts_to_from_entry_edges
    instead of 10*
 2) reduce probability of loopback and instead increase probability of
    exit.

The code attemts to get right only case where loop has one exit 
and instead of
  if (cond) -- (original-wrong-exit-probability) -- exit
it does
  if (cond) -- (exit-probability=1/#iterations) -- exit
Now it should adjust in-loop-count for every path from source of exit to
latch edge.  It just assumes that there is one basic block that is latch
and does it there.

I was just looking into using this for profile update when loop-ch or
complete unrolling proves that loop is iterating fewer times then
profile.  I can cleanup the funtion - it was originall written for the
old reperesentation of probabilities and cound and I did not do very
good job on updating it to new code.

Honza
> 
> and the comment correctly suggests all blocks following from here
> would need similar adjustment (and on in-loop branches the delta would be
> distributed according to probabilities).
> 
> Given the code is quite imperfect I would suggest to change the
> updating of the latch block count to read
> 
>   profile_count count_delta = profile_count::zero ();
>   if (loop->latch
>       && single_pred_p (loop->latch)
>       && loop_exits_from_bb_p (single_pred (loop->latch)))
>     {
>       count_delta = single_pred (loop->latch)->count - loop->latch->count;
>       loop->latch->count = single_pred (loop->latch)->count;
>     }
> 
>    scale_loop_frequencies (loop, p);
> 
>   if (count_delta != 0)
>     loop->latch->count -= count_delta;
> 
> which should exactly preserve the exit-before-latch behavior independent
> on the number of exits of the loop.
> 
> Please leave Honza a chance to comment here.
> 
> Thanks,
> Richard.
> 
> 
> > Thanks,
> > Tamar
> > 
> > gcc/ChangeLog:
> > 
> > 	* cfgloopmanip.cc (scale_loop_frequencies): Fix typo.
> > 	(scale_loop_profile): Don't access null pointer.
> > 
> > --- inline copy of patch -- 
> > diff --git a/gcc/cfgloopmanip.cc b/gcc/cfgloopmanip.cc
> > index 6e09dcbb0b1864bc64ffd570a4b923f50c3819b5..b10ef3d2be82902ccd74e52a4318217b2db13bcb 100644
> > --- a/gcc/cfgloopmanip.cc
> > +++ b/gcc/cfgloopmanip.cc
> > @@ -501,7 +501,7 @@ scale_loop_frequencies (class loop *loop, profile_probability p)
> >  /* Scale profile in LOOP by P.
> >     If ITERATION_BOUND is non-zero, scale even further if loop is predicted
> >     to iterate too many times.
> > -   Before caling this function, preheader block profile should be already
> > +   Before calling this function, preheader block profile should be already
> >     scaled to final count.  This is necessary because loop iterations are
> >     determined by comparing header edge count to latch ege count and thus
> >     they need to be scaled synchronously.  */
> > @@ -597,14 +597,14 @@ scale_loop_profile (class loop *loop, profile_probability p,
> >        /* If latch exists, change its count, since we changed
> >  	 probability of exit.  Theoretically we should update everything from
> >  	 source of exit edge to latch, but for vectorizer this is enough.  */
> > -      if (loop->latch && loop->latch != e->src)
> > +      if (e && loop->latch && loop->latch != e->src)
> >  	loop->latch->count += count_delta;
> >  
> >        /* Scale the probabilities.  */
> >        scale_loop_frequencies (loop, p);
> >  
> >        /* Change latch's count back.  */
> > -      if (loop->latch && loop->latch != e->src)
> > +      if (e && loop->latch && loop->latch != e->src)
> >  	loop->latch->count -= count_delta;
> >  
> >        if (dump_file && (dump_flags & TDF_DETAILS))
> > 
> > 
> > 
> > 
> > 
> 
> -- 
> Richard Biener <rguenther@suse.de>
> SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
> Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
> HRB 36809 (AG Nuernberg)