Re: [PR67828] don't unswitch loops on undefined SSA values (was: Re: [PR64164] drop copyrename, integrate into expand)

[Richard Biener <richard.guenther@gmail.com>]

On Fri, Oct 9, 2015 at 7:26 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> This patch fixes a latent bug in loop unswitching exposed by the PR64164
> changes.
>
> We would move a test out of a loop that might never have been executed,
> and that accessed an uninitialized variable.  The uninitialized SSA
> name, due to uncprop, now gets coalescesd with other SSA names,
> expanding the ill effects of the undefined behavior we introduce: in
> spite of the zero initialization introduced in later rtl stages for the
> uninitialized pseudo, by then we've already expanded a PHI node that
> referenced the unitialized variable in the path coming from a path in
> which it would necessarily be zero, to a copy from the coalesced pseudo,
> that gets modified between the zero-initialization and the copy, so the
> copied zero is no longer zero.  Oops.
>
> We might want to be stricter in coalesce conflict detection to avoid
> this sort of problem, and perhaps to avoid undefined values in uncprop,
> but this would all be attempting to limit the effects of undefined
> behavior, which is probably a waste of effort.  As long as we avoid
> introducing undefined behavior ourselves, we shouldn't have to do any of
> that.  So, this patch fixes loop unswitching so as to not introduce
> undefined behavior.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu.  Ok to install?

Ok.

Thanks,
Richard.

>
> [PR67828] don't unswitch on default defs of non-parms
>
> From: Alexandre Oliva <aoliva@redhat.com>
>
> for  gcc/ChangeLog
>
>         PR rtl-optimizatoin/67828
>         * tree-ssa-loop-unswitch.c: Include tree-ssa.h.
>         (tree_may_unswitch_on): Don't unswitch on expressions
>         involving undefined values.
>
> for  gcc/testsuite/ChangeLog
>
>         PR rtl-optimization/67828
>         * gcc.dg/torture/pr67828.c: New.
> ---
>  gcc/testsuite/gcc.dg/torture/pr67828.c |   43 ++++++++++++++++++++++++++++++++
>  gcc/tree-ssa-loop-unswitch.c           |    5 ++++
>  2 files changed, 48 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.dg/torture/pr67828.c
>
> diff --git a/gcc/testsuite/gcc.dg/torture/pr67828.c b/gcc/testsuite/gcc.dg/torture/pr67828.c
> new file mode 100644
> index 0000000..c7b6965
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/torture/pr67828.c
> @@ -0,0 +1,43 @@
> +/* Check that we don't misoptimize the final value of d.  We used to
> +   apply loop unswitching on if(j), introducing undefined behavior
> +   that the original code wouldn't exercise, and this undefined
> +   behavior would get later passes to misoptimize the loop.  */
> +
> +/* { dg-do run } */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +
> +int x;
> +
> +int __attribute__ ((noinline, noclone))
> +xprintf (int d) {
> +  if (d)
> +    {
> +      if (x)
> +       printf ("%d", d);
> +      abort ();
> +    }
> +}
> +
> +int a, b;
> +short c;
> +
> +int
> +main ()
> +{
> +  int j, d = 1;
> +  for (; c >= 0; c++)
> +    {
> +      a = d;
> +      d = 0;
> +      if (b)
> +       {
> +         xprintf (0);
> +         if (j)
> +           xprintf (0);
> +       }
> +    }
> +  xprintf (d);
> +  exit (0);
> +}
> diff --git a/gcc/tree-ssa-loop-unswitch.c b/gcc/tree-ssa-loop-unswitch.c
> index 4328d6a..d6faa37 100644
> --- a/gcc/tree-ssa-loop-unswitch.c
> +++ b/gcc/tree-ssa-loop-unswitch.c
> @@ -32,6 +32,7 @@ along with GCC; see the file COPYING3.  If not see
>  #include "internal-fn.h"
>  #include "gimplify.h"
>  #include "tree-cfg.h"
> +#include "tree-ssa.h"
>  #include "tree-ssa-loop-niter.h"
>  #include "tree-ssa-loop.h"
>  #include "tree-into-ssa.h"
> @@ -139,6 +140,10 @@ tree_may_unswitch_on (basic_block bb, struct loop *loop)
>    /* Condition must be invariant.  */
>    FOR_EACH_SSA_TREE_OPERAND (use, stmt, iter, SSA_OP_USE)
>      {
> +      /* Unswitching on undefined values would introduce undefined
> +        behavior that the original program might never exercise.  */
> +      if (ssa_undefined_value_p (use, true))
> +       return NULL_TREE;
>        def = SSA_NAME_DEF_STMT (use);
>        def_bb = gimple_bb (def);
>        if (def_bb
>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer