diff mbox

RFA: Fix rtl-optimization/57425

Message ID 20130615190823.wzbakvee68w0kcwo-nzlynne@webmail.spamcop.net
State New
Headers show

Commit Message

Joern Rennecke June 15, 2013, 11:08 p.m. UTC 
Bootstrapped/regtested on i686-pc-linux-gnu.
2013-06-15  Joern Rennecke <joern.rennecke@embecosm.com>

gcc:
	PR rtl-optimization/57425
	* alias.c (write_dependence_p): Add new parameters mem_size and
	canon_mem_addr.  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
	* gcc.dg/torture/pr57425-1.c, gcc.dg/torture/pr57425-2.c: New files.
	* gcc.dg/torture/pr57425-3.c: Likewise.

Comments

Eric Botcazou June 16, 2013, 10:34 a.m. UTC | #1 
> Bootstrapped/regtested on i686-pc-linux-gnu.

For the record, and as you diagnosed, the change proposed in
  http://gcc.gnu.org/ml/gcc-patches/2012-06/msg00367.html
means that we must now be very careful with memory dependency checking in the 
various RTL optimization passes.  Another example is PR rtl-opt/57569.

The patch is OK on principle but I think that we should use the same interface 
for write_dependence_p as for true_dependence_1, i.e. add a mem_mode parameter 
instead of a mem_size and add both mem_addr and mem_canonicalized (and since 
it doesn't seem that we need x_addr for now, let's set it aside).  Btw I agree 
that the interface is probably not optimal, but it has been there for a while.

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?

Thanks for working on this.
Joern Rennecke June 16, 2013, 12:11 p.m. UTC | #2 
Quoting Eric Botcazou <ebotcazou@adacore.com>:

> The patch is OK on principle but I think that we should use the same  
>  interface
> for write_dependence_p as for true_dependence_1, i.e. add a mem_mode  
>  parameter
> instead of a mem_size and add both mem_addr and mem_canonicalized (and since
> it doesn't seem that we need x_addr for now, let's set it aside).    
> Btw I agree
> that the interface is probably not optimal, but it has been there   
> for a while.


Well, I see some merit in the way true_dependence_1 does it with regards to
the opportunities that this allows function cloning / specialization, it
just thought that the tiny speed benefit it could bring to write_dependence_p
didn't justify the extra code and churn to have this - or an enum parameter
to distinguish the three cases.  Using DEP_ANTI / DEP_OUTPUT from sched-int.h
would mean unwanted dependencies, so it'd be a new ad-hoc enum... .

OK, so if we go with consistency with a bool mem_canonicalized, I suppose
I should also make writep bool, i.e.:

static int
write_dependence_p (const_rtx mem, unsigned mem_size, rtx canon_mem_addr,
                     const_rtx x, bool mem_canonicalized, bool writep)
diff mbox

Patch

Index: alias.c
===================================================================
--- alias.c	(revision 200126)
+++ alias.c	(working copy)
@@ -156,7 +156,7 @@  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, unsigned, rtx, const_rtx, int);
 
 static void memory_modified_1 (rtx, const_rtx, void *);
 
@@ -2553,10 +2553,12 @@  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 nonzero, a write to) MEM.
+   If CANON_MEM_ADDR is nonzero, it is the canonicalized address of MEM.  */
 
 static int
-write_dependence_p (const_rtx mem, const_rtx x, int writep)
+write_dependence_p (const_rtx mem, unsigned mem_size, rtx canon_mem_addr,
+		    const_rtx x, int writep)
 {
   rtx x_addr, mem_addr;
   rtx base;
@@ -2612,9 +2614,14 @@  write_dependence_p (const_rtx mem, const
     return 0;
 
   x_addr = canon_rtx (x_addr);
-  mem_addr = canon_rtx (mem_addr);
+  if (canon_mem_addr)
+    mem_addr = canon_mem_addr;
+  else
+    mem_addr = canon_rtx (mem_addr);
+  if (!mem_size)
+    mem_size = SIZE_FOR_MODE (mem);
 
-  if ((ret = memrefs_conflict_p (SIZE_FOR_MODE (mem), mem_addr,
+  if ((ret = memrefs_conflict_p (mem_size, mem_addr,
 				 SIZE_FOR_MODE (x), x_addr, 0)) != -1)
     return ret;
 
@@ -2629,7 +2636,19 @@  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, 0, NULL_RTX, x, /*writep=*/0);
+}
+
+/* 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, GET_MODE_SIZE (mem_mode), mem_addr,
+			     x, /*writep=*/0);
 }
 
 /* Output dependence: X is written after store in MEM takes place.  */
@@ -2637,7 +2656,7 @@  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, 0, NULL_RTX, x, /*writep=*/1);
 }
 
 
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;
+}