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 <rguent...@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)
