Re: [PATCH, PR85859][tail-merge] Factor out gimple_may_have_side_effects_p and use in stmt_local_def

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Jun 20, 2018 at 11:32 PM Martin Sebor <msebor@gmail.com> wrote:
>
> On 06/20/2018 03:14 PM, Tom de Vries wrote:
> > Hi,
> >
> > Consider the test-case from the patch.  When compiled with "-O2 -fno-dce
> > -fno-isolate-erroneous-paths-dereference -fno-tree-dce -fno-tree-vrp" and
> > run, we get:
> > ...
> > $ ./a.out
> > Floating point exception
> > ...
> >
> > The problem is introduced by -ftree-tail-merge (enabled by -O2), so it
> > executes fine when compiled with -fno-tree-tail-merge.
> >
> > Tail-merge merges two blocks it considers equal:
> > ...
> > find_duplicates: <bb 3> duplicate of <bb 4>
> > Removing basic block 4
> >
> >   <bb 3>
> >   _6 = foo (0);
> >   iftmp.2_10 = (long int) _6;
> >   goto <bb 5>; [100.00%]
> >
> >   <bb 4>
> >   iftmp.2_11 = (long int) &c;
> > ...
> > while the blocks in fact aren't equal from the point of view of side effects.
> > Executing bb3 causes the 'Floating point exception', while executing bb4
> > doesn't.
> >
> > This patch fixes the problem by factoring out a new function
> > gimple_may_have_side_effects_p from bb_no_side_effects_p, and reusing that
> > function in the side-effect test in stmt_local_def in tail-merge.

I think tail-merging and ifcombine need two different kinds of
no-side-effectness
so please do not factor it out.  In fact the name is too general and confusing
given we already have gimple_has_side_effects (which also means only
it _could_ have side-effects, not it must have...).

So simply add the call handling (and comment) to stmt_local_def (the gimple_vdef
check is redundant with the gimple_vuse one btw).

OK with that change.

Thanks,
Richard.

> Would gimple.h be a better place to declare the function, to
> make it easier to use (i.e., without searching for the header
> to include when using it for the first time)?
>
> Martin
>
> PS With one exception, AFAICS, the functions called by
> gimple_may_have_side_effects_p are all defined in gimple.c, but
> gimple_uses_undefined_value_p is declared in gimple-ssa.h and
> defined in tree-ssa.c.  I don't know enough about how functions
> are divvied up among these files but it seems to me that the
> easiest scheme to understand and follow would be to declare all
> extern gimple_xxx functions in gimple.h, no matter where they
> are defined.
>
> >
> > The patch inhibits tail-merge in pr81192.c, because
> > gimple_may_have_side_effects_p tests for gimple_uses_undefined_value_p, which
> > triggers for this particular test-case.
> >
> > Bootstrapped and reg-tested on x86_64.
> >
> > OK for trunk?
> >
> > Thanks,
> > - Tom
> >
> > [tail-merge] Factor out gimple_may_have_side_effects_p and use in stmt_local_def
> >
> > 2018-06-20  Tom de Vries  <tdevries@suse.de>
> >
> >       PR tree-optimization/85859
> >       * tree-ssa-ifcombine.c (gimple_may_have_side_effects_p): Factor out of
> >       ...
> >       (bb_no_side_effects_p): ... here.
> >       * tree-ssa-ifcombine.h: New file.
> >       (gimple_may_have_side_effects_p): Declare.
> >       * tree-ssa-tail-merge.c (stmt_local_def): Use
> >       gimple_may_have_side_effects_p.
> >
> >       * gcc.dg/pr85859.c: New test.
> >       * gcc.dg/pr81192.c: Update.
> >
> > ---
> >  gcc/testsuite/gcc.dg/pr81192.c |  2 +-
> >  gcc/testsuite/gcc.dg/pr85859.c | 19 +++++++++++++++++++
> >  gcc/tree-ssa-ifcombine.c       | 34 +++++++++++++++++++++++-----------
> >  gcc/tree-ssa-ifcombine.h       | 24 ++++++++++++++++++++++++
> >  gcc/tree-ssa-tail-merge.c      |  5 ++---
> >  5 files changed, 69 insertions(+), 15 deletions(-)
> >
> > diff --git a/gcc/testsuite/gcc.dg/pr81192.c b/gcc/testsuite/gcc.dg/pr81192.c
> > index 0049f371b3d..9db641ba91d 100644
> > --- a/gcc/testsuite/gcc.dg/pr81192.c
> > +++ b/gcc/testsuite/gcc.dg/pr81192.c
> > @@ -24,4 +24,4 @@ fn2 (void)
> >        ;
> >  }
> >
> > -/* { dg-final { scan-tree-dump-times "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" 1 "pre" } } */
> > +/* { dg-final { scan-tree-dump-not "(?n)find_duplicates: <bb .*> duplicate of <bb .*>" "pre" } } */
> > diff --git a/gcc/testsuite/gcc.dg/pr85859.c b/gcc/testsuite/gcc.dg/pr85859.c
> > new file mode 100644
> > index 00000000000..96eb9671137
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr85859.c
> > @@ -0,0 +1,19 @@
> > +/* { dg-do run } */
> > +/* { dg-options "-ftree-tail-merge -Wno-div-by-zero -O2 -fno-dce -fno-isolate-erroneous-paths-dereference -fno-tree-dce -fno-tree-vrp" } */
> > +
> > +int b, c, d, e;
> > +
> > +__attribute__ ((noinline, noclone))
> > +int foo (short f)
> > +{
> > +  f %= 0;
> > +  return f;
> > +}
> > +
> > +int
> > +main (void)
> > +{
> > +  b = (unsigned char) __builtin_parity (d);
> > +  e ? foo (0) : (long) &c;
> > +  return 0;
> > +}
> > diff --git a/gcc/tree-ssa-ifcombine.c b/gcc/tree-ssa-ifcombine.c
> > index b63c600c47b..8ea51a793f9 100644
> > --- a/gcc/tree-ssa-ifcombine.c
> > +++ b/gcc/tree-ssa-ifcombine.c
> > @@ -40,6 +40,7 @@ along with GCC; see the file COPYING3.  If not see
> >  #include "gimplify-me.h"
> >  #include "tree-cfg.h"
> >  #include "tree-ssa.h"
> > +#include "tree-ssa-ifcombine.h"
> >
> >  #ifndef LOGICAL_OP_NON_SHORT_CIRCUIT
> >  #define LOGICAL_OP_NON_SHORT_CIRCUIT \
> > @@ -108,6 +109,27 @@ recognize_if_then_else (basic_block cond_bb,
> >    return true;
> >  }
> >
> > +/* Return true if gimple STMT may have side effect.  */
> > +
> > +bool
> > +gimple_may_have_side_effects_p (gimple *stmt)
> > +{
> > +  if (gimple_has_side_effects (stmt)
> > +      || gimple_uses_undefined_value_p (stmt)
> > +      || gimple_could_trap_p (stmt)
> > +      || gimple_vuse (stmt)
> > +      /* const calls don't match any of the above, yet they could
> > +      still have some side-effects - they could contain
> > +      gimple_could_trap_p statements, like floating point
> > +      exceptions or integer division by zero.  See PR70586.
> > +      FIXME: perhaps gimple_has_side_effects or gimple_could_trap_p
> > +      should handle this.  */
> > +      || is_gimple_call (stmt))
> > +    return true;
> > +
> > +  return false;
> > +}
> > +
> >  /* Verify if the basic block BB does not have side-effects.  Return
> >     true in this case, else false.  */
> >
> > @@ -123,17 +145,7 @@ bb_no_side_effects_p (basic_block bb)
> >        if (is_gimple_debug (stmt))
> >       continue;
> >
> > -      if (gimple_has_side_effects (stmt)
> > -       || gimple_uses_undefined_value_p (stmt)
> > -       || gimple_could_trap_p (stmt)
> > -       || gimple_vuse (stmt)
> > -       /* const calls don't match any of the above, yet they could
> > -          still have some side-effects - they could contain
> > -          gimple_could_trap_p statements, like floating point
> > -          exceptions or integer division by zero.  See PR70586.
> > -          FIXME: perhaps gimple_has_side_effects or gimple_could_trap_p
> > -          should handle this.  */
> > -       || is_gimple_call (stmt))
> > +      if (gimple_may_have_side_effects_p (stmt))
> >       return false;
> >      }
> >
> > diff --git a/gcc/tree-ssa-ifcombine.h b/gcc/tree-ssa-ifcombine.h
> > new file mode 100644
> > index 00000000000..8c62ff450eb
> > --- /dev/null
> > +++ b/gcc/tree-ssa-ifcombine.h
> > @@ -0,0 +1,24 @@
> > +/* Copyright (C) 2018 Free Software Foundation, Inc.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > +under the terms of the GNU General Public License as published by the
> > +Free Software Foundation; either version 3, or (at your option) any
> > +later version.
> > +
> > +GCC is distributed in the hope that it will be useful, but WITHOUT
> > +ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> > +FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
> > +for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3.  If not see
> > +<http://www.gnu.org/licenses/>.  */
> > +
> > +#ifndef TREE_SSA_IFCOMBINE_H
> > +#define TREE_SSA_IFCOMBINE_H_
> > +
> > +extern bool gimple_may_have_side_effects_p (gimple *);
> > +
> > +#endif
> > diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
> > index f482ce197cd..ec8219b993b 100644
> > --- a/gcc/tree-ssa-tail-merge.c
> > +++ b/gcc/tree-ssa-tail-merge.c
> > @@ -206,6 +206,7 @@ along with GCC; see the file COPYING3.  If not see
> >  #include "cfgloop.h"
> >  #include "tree-eh.h"
> >  #include "tree-cfgcleanup.h"
> > +#include "tree-ssa-ifcombine.h"
> >
> >  const int ignore_edge_flags = EDGE_DFS_BACK | EDGE_EXECUTABLE;
> >
> > @@ -299,9 +300,7 @@ stmt_local_def (gimple *stmt)
> >    def_operand_p def_p;
> >
> >    if (gimple_vdef (stmt) != NULL_TREE
> > -      || gimple_has_side_effects (stmt)
> > -      || gimple_could_trap_p_1 (stmt, false, false)
> > -      || gimple_vuse (stmt) != NULL_TREE)
> > +      || gimple_may_have_side_effects_p (stmt))
> >      return false;
> >
> >    def_p = SINGLE_SSA_DEF_OPERAND (stmt, SSA_OP_DEF);
> >
>