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

[Richard Biener <rguenther@suse.de>]

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)

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)