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] handle bzero/bcopy in DSE and aliasing (PR 80933, 80934)

On 06/08/2017 01:51 AM, Richard Biener wrote:

On Thu, Jun 8, 2017 at 4:33 AM, Martin Sebor <msebor@gmail.com> wrote:

On 06/07/2017 02:12 PM, Martin Sebor wrote:


On 06/07/2017 02:01 PM, Marc Glisse wrote:


On Wed, 7 Jun 2017, Bernhard Reutner-Fischer wrote:


On 7 June 2017 16:46:53 CEST, Martin Sebor <msebor@gmail.com> wrote:


On 06/07/2017 02:23 AM, Richard Biener wrote:


On Wed, Jun 7, 2017 at 5:26 AM, Martin Sebor <msebor@gmail.com>


wrote:


Note I'd be _much_ more sympathetic to simply canonicalizing all of
bzero and bcopy
to memset / memmove and be done with all the above complexity.




Attached is an updated patch along these lines.  Please let me
know if it matches your expectations.



I think you attached the wrong patch.



Yes I did, sorry.  The correct one is attached.



Under POSIX.1-2008 "optimizing" bzero or bcmp is IMO plain wrong.

It's like optimizing foo() to a random built-in but maybe that's just
me. If your libc provides a define to a standard function for these
under a compat knob then fine but otherwise you should fix that.
*shrug*. Joseph?



The patch optimizes __builtin_bzero, which should be ok. The question
(independent from this patch) is then under what conditions bzero should
be detected as a builtin.



Yes.  The problem is that unlike for C and C++, GCC doesn't have
a mechanism to select the target version of POSIX.  I think it
should.

But there is a subtle problem with the patch that needs fixing.
Bcopy should not be transformed to memcpy but rather memmove.
I'll fix that before committing.



Attached is an updated patch with this fix.  I also added a cast
from bcopy and bzero to void to detect accidental uses of the
return value.  Tested on x86_64-linux.


Please do not add foldings to builtins.c but instead add them to gimple-fold.c.


Sure.  Attached is an adjusted patch.



+  /* Call memset and return the result cast to void to detect its use
+     (bzero returns void).  */
+  tree call = build_call_expr_loc (loc, fn, 3, dst, integer_zero_node, len);
+  return fold_convert (void_type_node, call);

???  How can the result be used if the original call result was not?


The cast ensured GCC would continue to warn on code like:

  void f (void *d, unsigned n)
  {
    return bzero (d, n);
  }

Without the cast (as in the first patch) the above was silently
accepted.

This isn't necessary when the folding is done in gimple-fold.c.

Martin

PR tree-optimization/80934 - bzero should be assumed not to escape pointer argument
PR tree-optimization/80933 - redundant bzero/bcopy calls not eliminated

gcc/ChangeLog:

	PR tree-optimization/80933
	PR tree-optimization/80934
	* builtins.c (fold_builtin_3): Do not handle bcmp here.
	* gimple-fold.c (gimple_fold_builtin_bcmp): New function.
	(gimple_fold_builtin_bcopy, gimple_fold_builtin_bzero): Likewise.
	(gimple_fold_builtin): Call them.

gcc/testsuite/ChangeLog:

	PR tree-optimization/80933
	PR tree-optimization/80934
	* gcc.dg/fold-bcopy.c: New test.
	* gcc.dg/tree-ssa/ssa-dse-30.c: Likewise..
	* gcc.dg/tree-ssa/alias-36.c: Likewise.
	* gcc/testsuite/gcc.dg/pr79214.c: Adjust.
	* gcc.dg/tree-prof/val-prof-7.c: Likewise.
	* gcc.dg/Wsizeof-pointer-memaccess1.c: Likewise.
	* gcc.dg/builtins-nonnull.c: Likewise.

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 30462ad..ce657bf 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -9034,7 +9034,6 @@ fold_builtin_3 (location_t loc, tree fndecl,
 	return do_mpfr_remquo (arg0, arg1, arg2);
     break;
 
-    case BUILT_IN_BCMP:
     case BUILT_IN_MEMCMP:
       return fold_builtin_memcmp (loc, arg0, arg1, arg2);;
 
diff --git a/gcc/gimple-fold.c b/gcc/gimple-fold.c
index d12f9d0..159a7e6 100644
--- a/gcc/gimple-fold.c
+++ b/gcc/gimple-fold.c
@@ -1076,6 +1076,88 @@ done:
   return true;
 }
 
+/* Transform a call to built-in bcmp(a, b, len) at *GSI into one
+   to built-in memcmp (a, b, len).  */
+
+static bool
+gimple_fold_builtin_bcmp (gimple_stmt_iterator *gsi)
+{
+  tree fn = builtin_decl_implicit (BUILT_IN_MEMCMP);
+
+  if (!fn)
+    return false;
+
+  /* Transform bcmp (a, b, len) into memcmp (a, b, len).  */
+
+  gimple *stmt = gsi_stmt (*gsi);
+  tree a = gimple_call_arg (stmt, 0);
+  tree b = gimple_call_arg (stmt, 1);
+  tree len = gimple_call_arg (stmt, 2);
+
+  gimple_seq seq = NULL;
+  gimple *repl = gimple_build_call (fn, 3, a, b, len);
+  gimple_seq_add_stmt_without_update (&seq, repl);
+  gsi_replace_with_seq_vops (gsi, seq);
+  fold_stmt (gsi);
+
+  return true;
+}
+
+/* Transform a call to built-in bcopy (src, dest, len) at *GSI into one
+   to built-in memmove (dest, src, len).  */
+
+static bool
+gimple_fold_builtin_bcopy (gimple_stmt_iterator *gsi)
+{
+  tree fn = builtin_decl_implicit (BUILT_IN_MEMMOVE);
+
+  if (!fn)
+    return false;
+
+  /* bcopy has been removed from POSIX in Issue 7 but Issue 6 specifies
+     it's quivalent to memmove (not memcpy).  Transform bcopy (src, dest,
+     len) into memmove (dest, src, len).  */
+
+  gimple *stmt = gsi_stmt (*gsi);
+  tree src = gimple_call_arg (stmt, 0);
+  tree dest = gimple_call_arg (stmt, 1);
+  tree len = gimple_call_arg (stmt, 2);
+
+  gimple_seq seq = NULL;
+  gimple *repl = gimple_build_call (fn, 3, dest, src, len);
+  gimple_seq_add_stmt_without_update (&seq, repl);
+  gsi_replace_with_seq_vops (gsi, seq);
+  fold_stmt (gsi);
+
+  return true;
+}
+
+/* Transform a call to built-in bzero (dest, len) at *GSI into one
+   to built-in memset (dest, 0, len).  */
+
+static bool
+gimple_fold_builtin_bzero (gimple_stmt_iterator *gsi)
+{
+  tree fn = builtin_decl_implicit (BUILT_IN_MEMSET);
+
+  if (!fn)
+    return false;
+
+  /* Transform bzero (dest, len) into memset (dest, 0, len).  */
+
+  gimple *stmt = gsi_stmt (*gsi);
+  tree dest = gimple_call_arg (stmt, 0);
+  tree len = gimple_call_arg (stmt, 1);
+
+  gimple_seq seq = NULL;
+  gimple *repl = gimple_build_call (fn, 3, dest, integer_zero_node, len);
+  gimple_seq_add_stmt_without_update (&seq, repl);
+  gsi_replace_with_seq_vops (gsi, seq);
+  fold_stmt (gsi);
+
+  return true;
+}
+
 /* Fold function call to builtin memset or bzero at *GSI setting the
    memory of size LEN to VAL.  Return whether a simplification was made.  */
 
@@ -3287,16 +3369,17 @@ gimple_fold_builtin (gimple_stmt_iterator *gsi)
   enum built_in_function fcode = DECL_FUNCTION_CODE (callee);
   switch (fcode)
     {
+    case BUILT_IN_BCMP:
+      return gimple_fold_builtin_bcmp (gsi);
+    case BUILT_IN_BCOPY:
+      return gimple_fold_builtin_bcopy (gsi);
     case BUILT_IN_BZERO:
-      return gimple_fold_builtin_memset (gsi, integer_zero_node,
-					 gimple_call_arg (stmt, 1));
+      return gimple_fold_builtin_bzero (gsi);
+
     case BUILT_IN_MEMSET:
       return gimple_fold_builtin_memset (gsi,
 					 gimple_call_arg (stmt, 1),
 					 gimple_call_arg (stmt, 2));
-    case BUILT_IN_BCOPY:
-      return gimple_fold_builtin_memory_op (gsi, gimple_call_arg (stmt, 1),
-					    gimple_call_arg (stmt, 0), 3);
     case BUILT_IN_MEMCPY:
       return gimple_fold_builtin_memory_op (gsi, gimple_call_arg (stmt, 0),
 					    gimple_call_arg (stmt, 1), 0);
diff --git a/gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c b/gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c
index 7feb122..f4e8552 100644
--- a/gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c
+++ b/gcc/testsuite/gcc.dg/Wsizeof-pointer-memaccess1.c
@@ -1,6 +1,6 @@
 /* Test -Wsizeof-pointer-memaccess warnings.  */
 /* { dg-do compile } */
-/* { dg-options "-Wall -Wno-sizeof-array-argument" } */
+/* { dg-options "-Wall -Wno-sizeof-array-argument -Wno-stringop-overflow" } */
 /* { dg-require-effective-target alloca } */
 
 typedef __SIZE_TYPE__ size_t;
diff --git a/gcc/testsuite/gcc.dg/builtins-nonnull.c b/gcc/testsuite/gcc.dg/builtins-nonnull.c
index fa9eaf2..726f8e1 100644
--- a/gcc/testsuite/gcc.dg/builtins-nonnull.c
+++ b/gcc/testsuite/gcc.dg/builtins-nonnull.c
@@ -24,8 +24,9 @@ void sink (int, ...);
 
 void test_memfuncs (void *s, unsigned n)
 {
-  /* Bzero is not declared attribute nonnull.  */
-  bzero (null (), n);
+  /* Bzero is not declared attribute nonnull (maybe it should be?)
+     but it's transformed into a call to memset() which is.  */
+  bzero (null (), n);             /* { dg-warning "argument 1 null where non-null expected" } */
 
   T (memcpy (null (), s, n));     /* { dg-warning "argument 1 null where non-null expected" } */
   T (memcpy (s, null (), n));     /* { dg-warning "argument 2 null where non-null expected" } */
diff --git a/gcc/testsuite/gcc.dg/fold-bcopy.c b/gcc/testsuite/gcc.dg/fold-bcopy.c
new file mode 100644
index 0000000..ce5b317
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/fold-bcopy.c
@@ -0,0 +1,43 @@
+/* PR tree-optimization/80933 - redundant bzero/bcopy calls not eliminated
+   { dg-do compile }
+   { dg-options "-O0 -Wall -fdump-tree-gimple" } */
+
+void f0 (void *p, const void *q, unsigned n)
+{
+  /* Bcopy corresponds to memmmove, not memcpy.  */
+  __builtin_bcopy (q, p, n);
+}
+
+void f1 (void *p, const void *q, unsigned n)
+{
+  /* This call should be eliminated.  */
+  __builtin_bcopy (q, p, 0);
+}
+
+int f2 (const void *p, const void *q, unsigned n)
+{
+  return __builtin_bcmp (q, p, n);
+}
+
+int f3 (const void *p, const void *q, unsigned n)
+{
+  /* This call should be folded into 0.  */
+  return __builtin_bcmp (q, p, 0);
+}
+
+void f4 (void *p, unsigned n)
+{
+  __builtin_bzero (p, n);
+}
+
+void f5 (void *p)
+{
+  /* This call should be eliminated.  */
+  __builtin_bzero (p, 0);
+}
+
+/* Verify that calls to bcmp, bcopy, and bzero have all been removed
+   and one of each replaced with memcmp, memmove, and memset, respectively.
+   The remaining three should be eliminated.
+  { dg-final { scan-tree-dump-not "bcmp|bcopy|bzero" "gimple" } }
+  { dg-final { scan-tree-dump-times "memcmp|memmove|memset" 3 "gimple" } } */
diff --git a/gcc/testsuite/gcc.dg/pr79214.c b/gcc/testsuite/gcc.dg/pr79214.c
index 79d2a25..6cf254fb 100644
--- a/gcc/testsuite/gcc.dg/pr79214.c
+++ b/gcc/testsuite/gcc.dg/pr79214.c
@@ -22,7 +22,7 @@ size_t range (void)
 
 void test_bzero (void)
 {
-  bzero (d, range ());   /* { dg-warning ".__builtin_bzero. writing 4 or more bytes into a region of size 3 overflows the destination" } */
+  bzero (d, range ());   /* { dg-warning ".__builtin_(bzero|memset). writing 4 or more bytes into a region of size 3 overflows the destination" } */
 }
 
 void test_memcpy (void)
diff --git a/gcc/testsuite/gcc.dg/tree-prof/val-prof-7.c b/gcc/testsuite/gcc.dg/tree-prof/val-prof-7.c
index 84ec9fb..5a4e777 100644
--- a/gcc/testsuite/gcc.dg/tree-prof/val-prof-7.c
+++ b/gcc/testsuite/gcc.dg/tree-prof/val-prof-7.c
@@ -4,14 +4,10 @@
 char *buffer1;
 char *buffer2;
 
+/* Bzero is not tested because it gets transformed into memset.  */
+
 #define DEFINE_TEST(N) \
 __attribute__((noinline)) \
-void bzero_test_ ## N (int len) \
-{ \
-  __builtin_bzero (buffer1, len); \
-} \
- \
-__attribute__((noinline)) \
 void memcpy_test_ ## N (int len) \
 { \
   __builtin_memcpy (buffer1, buffer2, len); \
@@ -31,7 +27,6 @@ void memset_test_ ## N (int len) \
  \
 void test_stringops_ ## N(int len) \
 { \
-  bzero_test_ ## N (len); \
   memcpy_test_## N (len); \
   mempcpy_test_ ## N (len); \
   memset_test_ ## N (len); \
@@ -64,10 +59,6 @@ int main() {
   return 0;
 }
 
-/* { dg-final-use-not-autofdo { scan-ipa-dump "Single value 8 stringop transformation on __builtin_bzero" "profile" } } */
-/* { dg-final-use-not-autofdo { scan-ipa-dump "Single value 55 stringop transformation on __builtin_bzero" "profile" } } */
-/* { dg-final-use-not-autofdo { scan-ipa-dump-times "Single value 32 stringop transformation on __builtin_bzero" 0 "profile" } } */
-
 /* { dg-final-use-not-autofdo { scan-ipa-dump "Single value 8 stringop transformation on __builtin_memcpy" "profile" } } */
 /* { dg-final-use-not-autofdo { scan-ipa-dump "Single value 55 stringop transformation on __builtin_memcpy" "profile" } } */
 /* { dg-final-use-not-autofdo { scan-ipa-dump-times "Single value 32 stringop transformation on __builtin_memcpy" 0 "profile" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/alias-36.c b/gcc/testsuite/gcc.dg/tree-ssa/alias-36.c
new file mode 100644
index 0000000..61b601a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/alias-36.c
@@ -0,0 +1,28 @@
+/* PR tree-optimization/80934 - bzero should be assumed not to escape
+   pointer argument
+   { dg-do compile }
+   { dg-options "-O2 -fdump-tree-alias" } */
+
+void foobar (void);
+
+void f (void);
+
+void g (void)
+{
+  char d[32];
+  __builtin_memset (d, 0, sizeof d);
+  f ();
+  if (*d != 0)
+    foobar ();
+}
+
+void h (void)
+{
+  char d[32];
+  __builtin_bzero (d, sizeof d);
+  f ();
+  if (*d != 0)
+    foobar ();
+}
+
+/* { dg-final { scan-tree-dump-not "memset|foobar|bzero" "alias" } } */
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-30.c b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-30.c
new file mode 100644
index 0000000..ece8cb2
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/tree-ssa/ssa-dse-30.c
@@ -0,0 +1,31 @@
+/* PR tree-optimization/80933 - redundant bzero/bcopy calls not eliminated
+   { dg-do compile }
+   { dg-options "-O2 -fdump-tree-dse1" } */
+
+void sink (void*);
+
+void test_bcopy (const void *s)
+{
+  char d[33];
+
+  /* Bcopy is transformed into memcpy and those calls are expanded
+     inline in EVRP, before DSE runs, so this test doesn't actually
+     verify that DSE does its job.  */
+  __builtin_bcopy (s, d, sizeof d);
+  __builtin_bcopy (s, d, sizeof d);
+
+  sink (d);
+}
+
+void test_bzero (void)
+{
+  char d[33];
+
+  __builtin_bzero (d, sizeof d);
+  __builtin_bzero (d, sizeof d);
+
+  sink (d);
+}
+
+/* { dg-final { scan-tree-dump-times "builtin_memset" 1 "dse1" } } */
+/* { dg-final { scan-tree-dump-not "builtin_(bcopy|bzero|memcpy)" "dse1" } } */
Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]