On Wed, Jun 20, 2018 at 11:32 PM Martin Sebor <mse...@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 <tdevr...@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); > > >