diff mbox

RFA: Fix rtl-optimization/57425

Message ID 20130616092626.oidnkv1n8gkg808w-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

Joern Rennecke June 16, 2013, 1:26 p.m. UTC 
Quoting Eric Botcazou <ebotcazou@adacore.com>:

> Could you also check that your patch also fixes PR opt/57569 and, if so, add
> the reference to the ChangeLog as well as the testcase?

Attached is what I'm currently testing. bootstrap on i686-pc-linux-gnu
finished, now regtesting.
On x86_64-pc-linux-gnu, bootstrap is still in progress & I also still have
to verify if the pr57569.c testcase FAILs/PASSes for unpatched/patched
svn trunk.
2013-06-15  Joern Rennecke <joern.rennecke@embecosm.com>

gcc:
	PR rtl-optimization/57425
	PR rtl-optimization/57569
	* alias.c (write_dependence_p): Add new parameters mem_size,
	canon_mem_addr and mem_canonicalized.  Change type of writep to bool.
	Changed all callers.
	(canon_anti_dependence): New function.
	* cse.c (check_dependence): Use canon_anti_dependence.
	* cselib.c (cselib_invalidate_mem): Likewise.
	* rtl.h (canon_anti_dependence): Declare.
gcc/testsuite:
	PR rtl-optimization/57425
	PR rtl-optimization/57569
	* gcc.dg/torture/pr57425-1.c, gcc.dg/torture/pr57425-2.c: New files.
	* gcc.dg/torture/pr57425-3.c, gcc.dg/torture/pr57569.c: Likewise.

Comments

Michael Matz June 18, 2013, 1:01 p.m. UTC | #1 
On Sun, 16 Jun 2013, Joern Rennecke wrote:

> Quoting Eric Botcazou <ebotcazou@adacore.com>:
> 
> > Could you also check that your patch also fixes PR opt/57569 and, if so, add
> > the reference to the ChangeLog as well as the testcase?
> 
> Attached is what I'm currently testing. bootstrap on i686-pc-linux-gnu
> finished, now regtesting.
> On x86_64-pc-linux-gnu, bootstrap is still in progress & I also still have
> to verify if the pr57569.c testcase FAILs/PASSes for unpatched/patched
> svn trunk.

Careful.  The patch uses the wrong order of arguments in the call
to the new function:

>  /* Returns nonzero if a write to X might alias a previous read from            
> -   (or, if WRITEP is nonzero, a write to) MEM.  */                             
> +   (or, if WRITEP is true, a write to) MEM.                                    
> +   If MEM_CANONCALIZED is nonzero, CANON_MEM_ADDR is the canonicalized         
> +   address of MEM, and MEM_MODE the mode for that access.  */                  
>                                                                                 
>  static int                                                                     
> -write_dependence_p (const_rtx mem, const_rtx x, int writep)                    
> +write_dependence_p (const_rtx mem, enum machine_mode mem_mode,                 
> +                   rtx canon_mem_addr, const_rtx x,                            
> +                   bool mem_canonicalized, bool writep)

So, first argument is the thing in question (the one that might be 
clobbered), the second (or in the new interface the fourth) is the write 
that might clobber it ...

> /* Likewise, but we already have a canonicalized MEM_ADDR for MEM.             
> +   Also, consider MEM in MEM_MODE (which might be from an enclosing            
> +   STRICT_LOW_PART / ZERO_EXTRACT).  */                                        
> +                                                                               
> +int                                                                            
> +canon_anti_dependence (const_rtx mem, enum machine_mode mem_mode,              
> +                      rtx mem_addr, const_rtx x)                               
> +{                                                                              
> +  return write_dependence_p (mem, mem_mode, mem_addr, x,  

... same here, first the read, then the potentially destroying write ...

>    if (*x && MEM_P (*x))                                                        
> -    return canon_true_dependence (d->exp, d->mode, d->addr, *x, NULL_RTX);     
> +    return canon_anti_dependence (d->exp, d->mode, d->addr, *x);               
>    else

... but here you use the same order as for true_dependence, to cite from 
the function comment:

> /* True dependence: X is read after store in MEM takes place.  */
> int
> true_dependence (const_rtx mem, enum machine_mode mem_mode, const_rtx x)

So, first the potentially clobbering write, then the read.  And indeed in 
check_dependence d->exp is the write and x the read that is potentially 
clobbered.

I.e. the arguments after your patch are exactly swapped.  This is usually 
harmless, but not always, so that should be corrected before check in.
The change in cselib.c:cselib_invalidate_mem has the same problem.

Generally I would prefer simple interfaces to the query functions, 
dependence problems are hard enough to think about without functions 
needing four arguments.  Does it really save much to not canonicalize the 
mem address for some calls?


Ciao,
Michael.
Joern Rennecke June 18, 2013, 7:53 p.m. UTC | #2 
Quoting Michael Matz <matz@suse.de>:

> So, first the potentially clobbering write, then the read.  And indeed in
> check_dependence d->exp is the write and x the read that is potentially
> clobbered.

Oops, you are right.  I got confused because what is X in cse.c:invalidate
ends up as d->exp in cse.c:check_dependence, and what is &p->canon_exp in
cse.c:invalidate ends up as X in cse.c:check_dependence.

I was a bit puzzled why we could have a STRICT_LOW_PART for MEM.  Now it
makes more sense.

I also see now that both the read expression from the hash table and the
write address are canonicalized, so if we have a canon_* interface, we
want to use both.

> Generally I would prefer simple interfaces to the query functions,
> dependence problems are hard enough to think about without functions
> needing four arguments.  Does it really save much to not canonicalize the
> mem address for some calls?

I haven't measured it myself, but I suppose it must be there for a  
good reason.
Of course a simpler interface would be nice, however...
When cse encounters a write, it goes through the entire hash table of
expressions and checks dependencies.  So for (extended?) basic blocks with
lots of expressions & writes, we got O(n^2) dependency checks.
diff mbox

Patch

Index: alias.c
===================================================================
--- alias.c	(revision 200126)
+++ alias.c	(working copy)
@@ -156,7 +156,8 @@  static int insert_subset_children (splay
 static alias_set_entry get_alias_set_entry (alias_set_type);
 static bool nonoverlapping_component_refs_p (const_rtx, const_rtx);
 static tree decl_for_component_ref (tree);
-static int write_dependence_p (const_rtx, const_rtx, int);
+static int write_dependence_p (const_rtx, enum machine_mode, rtx, const_rtx,
+			       bool, bool);
 
 static void memory_modified_1 (rtx, const_rtx, void *);
 
@@ -2553,15 +2554,22 @@  canon_true_dependence (const_rtx mem, en
 }
 
 /* Returns nonzero if a write to X might alias a previous read from
-   (or, if WRITEP is nonzero, a write to) MEM.  */
+   (or, if WRITEP is true, a write to) MEM.
+   If MEM_CANONCALIZED is nonzero, CANON_MEM_ADDR is the canonicalized
+   address of MEM, and MEM_MODE the mode for that access.  */
 
 static int
-write_dependence_p (const_rtx mem, const_rtx x, int writep)
+write_dependence_p (const_rtx mem, enum machine_mode mem_mode,
+		    rtx canon_mem_addr, const_rtx x,
+		    bool mem_canonicalized, bool writep)
 {
   rtx x_addr, mem_addr;
   rtx base;
   int ret;
 
+  gcc_checking_assert (mem_canonicalized ? (canon_mem_addr != NULL_RTX)
+		       : (canon_mem_addr == NULL_RTX && mem_mode == VOIDmode));
+
   if (MEM_VOLATILE_P (x) && MEM_VOLATILE_P (mem))
     return 1;
 
@@ -2612,9 +2620,15 @@  write_dependence_p (const_rtx mem, const
     return 0;
 
   x_addr = canon_rtx (x_addr);
-  mem_addr = canon_rtx (mem_addr);
+  if (mem_canonicalized)
+    mem_addr = canon_mem_addr;
+  else
+    {
+      mem_addr = canon_rtx (mem_addr);
+      mem_mode = GET_MODE (mem);
+    }
 
-  if ((ret = memrefs_conflict_p (SIZE_FOR_MODE (mem), mem_addr,
+  if ((ret = memrefs_conflict_p (GET_MODE_SIZE (mem_mode), mem_addr,
 				 SIZE_FOR_MODE (x), x_addr, 0)) != -1)
     return ret;
 
@@ -2629,7 +2643,20 @@  write_dependence_p (const_rtx mem, const
 int
 anti_dependence (const_rtx mem, const_rtx x)
 {
-  return write_dependence_p (mem, x, /*writep=*/0);
+  return write_dependence_p (mem, VOIDmode, NULL_RTX, x,
+			     /*mem_canonicalized=*/false, /*writep=*/false);
+}
+
+/* Likewise, but we already have a canonicalized MEM_ADDR for MEM.
+   Also, consider MEM in MEM_MODE (which might be from an enclosing
+   STRICT_LOW_PART / ZERO_EXTRACT).  */
+
+int
+canon_anti_dependence (const_rtx mem, enum machine_mode mem_mode,
+		       rtx mem_addr, const_rtx x)
+{
+  return write_dependence_p (mem, mem_mode, mem_addr, x,
+			     /*mem_canonicalized=*/true, /*writep=*/false);
 }
 
 /* Output dependence: X is written after store in MEM takes place.  */
@@ -2637,7 +2664,8 @@  anti_dependence (const_rtx mem, const_rt
 int
 output_dependence (const_rtx mem, const_rtx x)
 {
-  return write_dependence_p (mem, x, /*writep=*/1);
+  return write_dependence_p (mem, VOIDmode, NULL_RTX, x,
+			     /*mem_canonicalized=*/false, /*writep=*/true);
 }
 
 
Index: cse.c
===================================================================
--- cse.c	(revision 200126)
+++ cse.c	(working copy)
@@ -1824,7 +1824,7 @@  flush_hash_table (void)
       }
 }
 
-/* Function called for each rtx to check whether true dependence exist.  */
+/* Function called for each rtx to check whether an anti dependence exist.  */
 struct check_dependence_data
 {
   enum machine_mode mode;
@@ -1837,7 +1837,7 @@  check_dependence (rtx *x, void *data)
 {
   struct check_dependence_data *d = (struct check_dependence_data *) data;
   if (*x && MEM_P (*x))
-    return canon_true_dependence (d->exp, d->mode, d->addr, *x, NULL_RTX);
+    return canon_anti_dependence (d->exp, d->mode, d->addr, *x);
   else
     return 0;
 }
Index: cselib.c
===================================================================
--- cselib.c	(revision 200126)
+++ cselib.c	(working copy)
@@ -2263,8 +2263,8 @@  cselib_invalidate_mem (rtx mem_rtx)
 	      continue;
 	    }
 	  if (num_mems < PARAM_VALUE (PARAM_MAX_CSELIB_MEMORY_LOCATIONS)
-	      && ! canon_true_dependence (mem_rtx, GET_MODE (mem_rtx),
-					  mem_addr, x, NULL_RTX))
+	      && ! canon_anti_dependence (mem_rtx, GET_MODE (mem_rtx),
+					  mem_addr, x))
 	    {
 	      has_mem = true;
 	      num_mems++;
Index: rtl.h
===================================================================
--- rtl.h	(revision 200126)
+++ rtl.h	(working copy)
@@ -2705,6 +2705,8 @@  extern int canon_true_dependence (const_
 				  const_rtx, rtx);
 extern int read_dependence (const_rtx, const_rtx);
 extern int anti_dependence (const_rtx, const_rtx);
+extern int canon_anti_dependence (const_rtx, enum machine_mode, rtx,
+				  const_rtx);
 extern int output_dependence (const_rtx, const_rtx);
 extern int may_alias_p (const_rtx, const_rtx);
 extern void init_alias_target (void);
Index: testsuite/gcc.dg/torture/pr57425-1.c
===================================================================
--- testsuite/gcc.dg/torture/pr57425-1.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr57425-1.c	(working copy)
@@ -0,0 +1,37 @@ 
+/* { dg-do run } */
+
+extern void abort (void) __attribute__((noreturn));
+
+union setconflict
+{
+  int a[20];
+  long b[10];
+};
+
+int
+main ()
+{
+  int sum = 0;
+  {
+    union setconflict a;
+    int *c;
+    c = a.a;
+    asm ("": "=r" (c):"0" (c));
+    *c = 0;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+  {
+    union setconflict a;
+    long *c;
+    c = a.b;
+    asm ("": "=r" (c):"0" (c));
+    *c = 1;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+
+  if (sum != 1)
+    abort();
+  return 0;
+}
Index: testsuite/gcc.dg/torture/pr57425-2.c
===================================================================
--- testsuite/gcc.dg/torture/pr57425-2.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr57425-2.c	(working copy)
@@ -0,0 +1,31 @@ 
+/* { dg-do run } */
+
+extern void abort (void) __attribute__((noreturn));
+
+int
+main ()
+{
+  int sum = 0;
+  {
+    int a[20];
+    int *c;
+    c = a;
+    asm ("": "=r" (c):"0" (c));
+    *c = 0;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+  {
+    long b[10];
+    long *c;
+    c = b;
+    asm ("": "=r" (c):"0" (c));
+    *c = 1;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+
+  if (sum != 1)
+    abort();
+  return 0;
+}
Index: testsuite/gcc.dg/torture/pr57425-3.c
===================================================================
--- testsuite/gcc.dg/torture/pr57425-3.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr57425-3.c	(working copy)
@@ -0,0 +1,31 @@ 
+/* { dg-do run } */
+
+extern void abort (void) __attribute__((noreturn));
+
+int
+main ()
+{
+  int sum = 0;
+  {
+    long a[20];
+    long *c;
+    c = a;
+    asm ("": "=r" (c):"0" (c));
+    *c = 0;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+  {
+    long long b[10];
+    long long *c;
+    c = b;
+    asm ("": "=r" (c):"0" (c));
+    *c = 1;
+    asm ("": "=r" (c):"0" (c));
+    sum += *c;
+  }
+
+  if (sum != 1)
+    abort();
+  return 0;
+}
Index: testsuite/gcc.dg/torture/pr57569.c
===================================================================
--- testsuite/gcc.dg/torture/pr57569.c	(revision 0)
+++ testsuite/gcc.dg/torture/pr57569.c	(working copy)
@@ -0,0 +1,37 @@ 
+/* { dg-do run } */
+
+extern void abort (void) __attribute__((noreturn));
+
+struct S { int f0; } a; 
+
+int b, e, *d = &b, f;
+
+void 
+fn1 ()
+{
+  int **g[9][6];
+  int ***h = &g[6][3];
+  for (; e < 9; e++) {
+    f = 0;
+    for (; f < 6; f++)
+      g[e][f] = &d;
+  }
+  ***h = 0;
+}
+
+void
+fn2 ()
+{
+  fn1 ();
+  struct S c[4][10] = {};
+  a = c[3][9];
+}
+
+int
+main ()
+{
+  fn2 ();
+  if (a.f0 != 0)
+    abort ();
+  return 0;
+}