This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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.


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);
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]