[RFA,PR,tree-optimization/68599] Avoid over-zealous optimization with -funsafe-loop-optimizations

I strongly recommend reading the analysis in pr45122 since pr68599 uses 
the same testcase and just triggers the same bug in the RTL optimizers 
instead of the tree optimziers.

As noted in 45122, with -funsafe-loop-optimizations, we may exit the 
loop an iteration too early.  The loop in question is finite and the 
counter does not overflow.  Yet -funsafe-loop-optimizations munges it badly.

As is noted in c#6 and patched in c#8, when there's more than one exit 
from the loop, simply discarding the assumptions for the trip count is 
"a bit too unsafe".  Richi & Zdenek agreed that disabling the 
optimization when the loop has > 1 exit was the preferred approach. 
Alex's patch did just that, but only for the tree optimizers.

This patch does essentially the same thing for the RTL loop optimizer. 
If the candidate loop has > 1 exit, then we don't allow 
-funsafe-loop-optimizations to drop the assumptions/infinite notes for 
the RTL loop.

This required ensuring that LOOPS_HAVE_RECORDED_EXITS when initializing 
the loop optimizer.

Bootstrapped and regression tested on x86_64-linux-gnu and 
powerpc64-linux-gnu.  For the latter, pr45122.c flips to a pass.  Given 
this is covered by the pr45122 testcase, I didn't add a new one.

OK for the trunk?

Jeff

On Wed, Dec 2, 2015 at 5:27 PM, Jeff Law <law@redhat.com> wrote:
>
>
> I strongly recommend reading the analysis in pr45122 since pr68599 uses the
> same testcase and just triggers the same bug in the RTL optimizers instead
> of the tree optimziers.
>
> As noted in 45122, with -funsafe-loop-optimizations, we may exit the loop an
> iteration too early.  The loop in question is finite and the counter does
> not overflow.  Yet -funsafe-loop-optimizations munges it badly.
>
> As is noted in c#6 and patched in c#8, when there's more than one exit from
> the loop, simply discarding the assumptions for the trip count is "a bit too
> unsafe".  Richi & Zdenek agreed that disabling the optimization when the
> loop has > 1 exit was the preferred approach. Alex's patch did just that,
> but only for the tree optimizers.
>
> This patch does essentially the same thing for the RTL loop optimizer. If
> the candidate loop has > 1 exit, then we don't allow
> -funsafe-loop-optimizations to drop the assumptions/infinite notes for the
> RTL loop.
>
> This required ensuring that LOOPS_HAVE_RECORDED_EXITS when initializing the
> loop optimizer.
>
> Bootstrapped and regression tested on x86_64-linux-gnu and
> powerpc64-linux-gnu.  For the latter, pr45122.c flips to a pass.  Given this
> is covered by the pr45122 testcase, I didn't add a new one.
>
> OK for the trunk?

Ok.

Note that I believe we should dump -funsafe-loop-optimizations in
favor of a per-loop
#pragma now that we can properly track such.  Globally it's known to miscompile
SPEC at least.

Thanks,
Richard.

> Jeff
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 9a78b7a..ed677ec 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,12 @@
> +2015-12-02  Jeff Law  <law@redhat.com>
> +
> +       PR tree-optimization/68599
> +       * loop-init.c (rtl_loop_init): Set LOOPS_HAVE_RECORDED_EXITS
> +       in call to loop_optimizer_init.
> +       * loop-iv.c (get_simple_loop_desc): Only allow unsafe loop
> +       optimization to drop the assumptions/infinite notations if
> +       the loop has a single exit.
> +
>  2015-12-01  Andreas Tobler  <andreast@gcc.gnu.org>
>
>         * config/rs6000/freebsd64.h (ELFv2_ABI_CHECK): Add new macro.
> diff --git a/gcc/loop-init.c b/gcc/loop-init.c
> index e32c94a..120316d 100644
> --- a/gcc/loop-init.c
> +++ b/gcc/loop-init.c
> @@ -395,7 +395,7 @@ rtl_loop_init (void)
>        dump_flow_info (dump_file, dump_flags);
>      }
>
> -  loop_optimizer_init (LOOPS_NORMAL);
> +  loop_optimizer_init (LOOPS_NORMAL | LOOPS_HAVE_RECORDED_EXITS);
>    return 0;
>  }
>
> diff --git a/gcc/loop-iv.c b/gcc/loop-iv.c
> index c7d5164..dfa3ca3 100644
> --- a/gcc/loop-iv.c
> +++ b/gcc/loop-iv.c
> @@ -3054,7 +3054,7 @@ get_simple_loop_desc (struct loop *loop)
>             }
>         }
>
> -      if (flag_unsafe_loop_optimizations)
> +      if (flag_unsafe_loop_optimizations && single_exit (loop))
>         {
>           desc->assumptions = NULL_RTX;
>           desc->infinite = NULL_RTX;
>

On 12/03/2015 02:36 AM, Richard Biener wrote:
> On Wed, Dec 2, 2015 at 5:27 PM, Jeff Law <law@redhat.com> wrote:
>>
>>
>> I strongly recommend reading the analysis in pr45122 since pr68599 uses the
>> same testcase and just triggers the same bug in the RTL optimizers instead
>> of the tree optimziers.
>>
>> As noted in 45122, with -funsafe-loop-optimizations, we may exit the loop an
>> iteration too early.  The loop in question is finite and the counter does
>> not overflow.  Yet -funsafe-loop-optimizations munges it badly.
>>
>> As is noted in c#6 and patched in c#8, when there's more than one exit from
>> the loop, simply discarding the assumptions for the trip count is "a bit too
>> unsafe".  Richi & Zdenek agreed that disabling the optimization when the
>> loop has > 1 exit was the preferred approach. Alex's patch did just that,
>> but only for the tree optimizers.
>>
>> This patch does essentially the same thing for the RTL loop optimizer. If
>> the candidate loop has > 1 exit, then we don't allow
>> -funsafe-loop-optimizations to drop the assumptions/infinite notes for the
>> RTL loop.
>>
>> This required ensuring that LOOPS_HAVE_RECORDED_EXITS when initializing the
>> loop optimizer.
>>
>> Bootstrapped and regression tested on x86_64-linux-gnu and
>> powerpc64-linux-gnu.  For the latter, pr45122.c flips to a pass.  Given this
>> is covered by the pr45122 testcase, I didn't add a new one.
>>
>> OK for the trunk?
>
> Ok.
>
> Note that I believe we should dump -funsafe-loop-optimizations in
> favor of a per-loop
> #pragma now that we can properly track such.  Globally it's known to miscompile
> SPEC at least.
Yea, I saw that on IRC and almost went down that path.  Certainly 
wouldn't get any argument from me if we were to remove that option. 
Sounds like Bin might want to do that and he'll have my full support.

Jeff