diff mbox

RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)

Message ID 20110613184028.GW17079@tyan-ft48-01.lab.bos.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek June 13, 2011, 6:40 p.m. UTC 
Hi!

As the testcase shows, the
http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02945.html
patch looks wrong, MEM_ATTRS matters quite a lot for the
alias oracle, so ignoring it leads to miscompilations.

Instead of just reverting the patch, this patch attempts to add
some exceptions, notably when MEM_ATTRS are indirect with MEM_REF
containing an SSA_NAME as base, and the SSA_NAMEs have the same
var (maybe that check is unnecessary) and both SSA_NAMEs have the
same points-to info, we can consider them interchangeable.
But when they have different points-to info or if other details
are different, we need to play safe and return false from exp_equiv_p.

Bootstrapped/regtested on x86_64-linux and i686-linux.

Should the tests in rtx_mem_attrs_equiv_p be split into more
functions (e.g. have a points-to equiv inline function, and
perhaps most of the remaining body in tree-ssa-alias.c instead
of alias.c (like just do the ao_ref_from_mem/MEM_ATTRS/MEM_ALIGN
checks in there and keep the rest in the tree files)?

2011-06-13  Jakub Jelinek  <jakub@redhat.com>

	PR rtl-optimization/49390
	* alias.c (rtx_mem_attrs_equiv_p): New function.
	* alias.h (rtx_mem_attrs_equiv_p): New prototype.
	* tree-ssa-alias.c (ao_ref_base_alias_set): No longer static.
	* tree-ssa-alias.h (ao_ref_base_alias_set): New prototype.
	* cse.c (exp_equiv_p) <case MEM>: If MEM_ATTRS are different,
	call rtx_mem_attrs_equiv_p to see if MEM_ATTRS are interchangeable.

	* gcc.c-torture/execute/pr49390.c: New test.


	Jakub

Comments

Richard Biener June 14, 2011, 8:43 a.m. UTC | #1 
On Mon, 13 Jun 2011, Jakub Jelinek wrote:

> Hi!
> 
> As the testcase shows, the
> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02945.html
> patch looks wrong, MEM_ATTRS matters quite a lot for the
> alias oracle, so ignoring it leads to miscompilations.
> 
> Instead of just reverting the patch, this patch attempts to add
> some exceptions, notably when MEM_ATTRS are indirect with MEM_REF
> containing an SSA_NAME as base, and the SSA_NAMEs have the same
> var (maybe that check is unnecessary) and both SSA_NAMEs have the
> same points-to info, we can consider them interchangeable.

Hum, I think this is bogus.  When the SSA names are not exactly
the same we miss the must-alias check which prevents TBAA from
being applied.

So if you really want equivalency for the alias oracle then
you have to preserve whether the SSA names are exactly the
same or not.

The patch that reverted the MEM_ATTR comparison didn't come
with a single testcase (ugh, I realize I approved it though ;)).

What I suspect is that we are not good with sharing MEM_ATTRS
with MEM_EXPRs, esp. using operand_equal_p for comparing MEM_EXPRs
does not do a structural comparison of the trees (that was ok
as long as we didn't have INDIRECT_REFs as bases for MEM_EXPRs
but NULLed them).  Maybe it was already fixed with my patch
to treat the base operand of MEM_REFs specially via 
OEP_CONSTANT_ADDRESS_OF?

So, please consider reverting Bernds patch instead.

Bernd, do you have any testcases?

Thanks,
Richard.
Bernd Schmidt June 14, 2011, 9:37 a.m. UTC | #2 
On 06/14/2011 10:43 AM, Richard Guenther wrote:
> The patch that reverted the MEM_ATTR comparison didn't come
> with a single testcase (ugh, I realize I approved it though ;)).

> Bernd, do you have any testcases?

It was a missed-optimization problem, but I think it only showed up with
a modified ARM backend, and it was a set of changes I threw away in the
end since I found a better fix. So, from that angle no objections if
it's reverted.

Judging from the variable names the testcase was 253.perlbmk/op.c, but I
can't make the problem reappear at the moment - quite possibly because
I'm not fully remembering what I had changed in arm.c.


Bernd
Richard Biener June 14, 2011, 9:49 a.m. UTC | #3 
On Tue, 14 Jun 2011, Bernd Schmidt wrote:

> On 06/14/2011 10:43 AM, Richard Guenther wrote:
> > The patch that reverted the MEM_ATTR comparison didn't come
> > with a single testcase (ugh, I realize I approved it though ;)).
> 
> > Bernd, do you have any testcases?
> 
> It was a missed-optimization problem, but I think it only showed up with
> a modified ARM backend, and it was a set of changes I threw away in the
> end since I found a better fix. So, from that angle no objections if
> it's reverted.
> 
> Judging from the variable names the testcase was 253.perlbmk/op.c, but I
> can't make the problem reappear at the moment - quite possibly because
> I'm not fully remembering what I had changed in arm.c.

It's likely that due to MEM_REFs on the tree level we now detect more
cases there.  Btw, if we'd re-arrange the code to use NULL MEM_ATTRS
for the canonical MEM whenever we see two non-equivalent MEM_ATTRS
it should work again (no need to compare MEM_ALIAS_SET either then).
Not sure where to do that check and MEM_ATTRS adjustment though
(probably at hashtable lookup time).

So I'd say we revert your patch for now and if somebody feels like
implementing the above ...

Richard.
diff mbox

Patch

--- gcc/alias.c.jj	2011-05-24 23:34:28.000000000 +0200
+++ gcc/alias.c	2011-06-13 17:17:17.000000000 +0200
@@ -374,6 +374,79 @@  rtx_refs_may_alias_p (const_rtx x, const
 			     && MEM_ALIAS_SET (mem) != 0);
 }
 
+/* Return true if MEM_ATTRS of X and Y are equivalent for the
+   alias oracle.  */
+
+bool
+rtx_mem_attrs_equiv_p (const_rtx x, const_rtx y)
+{
+  ao_ref ref1, ref2;
+
+  if (MEM_ATTRS (x) == MEM_ATTRS (y))
+    return true;
+
+  if (!ao_ref_from_mem (&ref1, x)
+      || !ao_ref_from_mem (&ref2, y))
+    return false;
+
+  if (MEM_ALIGN (x) != MEM_ALIGN (y))
+    return false;
+
+  if (ref1.base == NULL
+      || ref2.base == NULL
+      || ref1.ref == NULL
+      || ref2.ref == NULL
+      || MEM_ALIGN (x) != MEM_ALIGN (y)
+      || TREE_CODE (ref1.ref) != TREE_CODE (ref2.ref)
+      || TREE_CODE (ref1.base) != TREE_CODE (ref2.base)
+      || ref1.offset != ref2.offset
+      || ref1.size != ref2.size
+      || ref1.max_size != ref2.max_size
+      || ao_ref_alias_set (&ref1) != ao_ref_alias_set (&ref2)
+      || ao_ref_base_alias_set (&ref1) != ao_ref_base_alias_set (&ref2))
+    return false;
+
+  if (TREE_CODE (ref1.base) == MEM_REF
+      && TREE_CODE (TREE_OPERAND (ref1.base, 0)) == SSA_NAME
+      && TREE_CODE (TREE_OPERAND (ref2.base, 0)) == SSA_NAME)
+    {
+      tree v1 = TREE_OPERAND (ref1.base, 0);
+      tree v2 = TREE_OPERAND (ref2.base, 0);
+      struct ptr_info_def *pi1, *pi2;
+
+      if (SSA_NAME_VAR (v1) != SSA_NAME_VAR (v2)
+	  || !types_compatible_p (TREE_TYPE (ref1.base), TREE_TYPE (ref2.base))
+	  || TREE_OPERAND (ref1.base, 1) != TREE_OPERAND (ref2.base, 1))
+	return false;
+
+      pi1 = SSA_NAME_PTR_INFO (v1);
+      pi2 = SSA_NAME_PTR_INFO (v2);
+      if (pi1 == NULL || pi2 == NULL)
+	return pi1 == NULL && pi2 == NULL;
+      if (pi1->align != pi2->align
+	  || pi1->misalign != pi2->misalign
+	  || pi1->pt.anything != pi2->pt.anything
+	  || pi1->pt.nonlocal != pi2->pt.nonlocal
+	  || pi1->pt.escaped != pi2->pt.escaped
+	  || pi1->pt.ipa_escaped != pi2->pt.ipa_escaped
+	  || pi1->pt.null != pi2->pt.null
+	  || pi1->pt.vars_contains_global != pi2->pt.vars_contains_global
+	  || pi1->pt.vars_contains_restrict != pi2->pt.vars_contains_restrict)
+	return false;
+
+      if (pi1->pt.vars == NULL || pi2->pt.vars == NULL)
+	return pi1->pt.vars == NULL && pi2->pt.vars == NULL;
+
+      if (pi1->pt.vars == pi2->pt.vars
+	  || bitmap_equal_p (pi1->pt.vars, pi2->pt.vars))
+	return true;
+
+      return false;
+    }
+
+  return false;
+}
+
 /* Returns a pointer to the alias set entry for ALIAS_SET, if there is
    such an entry, or NULL otherwise.  */
 
--- gcc/alias.h.jj	2011-01-06 10:21:52.000000000 +0100
+++ gcc/alias.h	2011-06-13 17:05:46.000000000 +0200
@@ -43,6 +43,7 @@  extern int alias_sets_conflict_p (alias_
 extern int alias_sets_must_conflict_p (alias_set_type, alias_set_type);
 extern int objects_must_conflict_p (tree, tree);
 extern int nonoverlapping_memrefs_p (const_rtx, const_rtx, bool);
+extern bool rtx_mem_attrs_equiv_p (const_rtx, const_rtx);
 
 /* This alias set can be used to force a memory to conflict with all
    other memories, creating a barrier across which no memory reference
--- gcc/tree-ssa-alias.c.jj	2011-05-31 08:03:10.000000000 +0200
+++ gcc/tree-ssa-alias.c	2011-06-13 17:16:36.000000000 +0200
@@ -489,7 +489,7 @@  ao_ref_base (ao_ref *ref)
 
 /* Returns the base object alias set of the memory reference *REF.  */
 
-static alias_set_type
+alias_set_type
 ao_ref_base_alias_set (ao_ref *ref)
 {
   tree base_ref;
--- gcc/tree-ssa-alias.h.jj	2011-05-02 18:39:28.000000000 +0200
+++ gcc/tree-ssa-alias.h	2011-05-02 18:39:28.000000000 +0200
@@ -96,6 +96,7 @@  extern void ao_ref_init (ao_ref *, tree)
 extern void ao_ref_init_from_ptr_and_size (ao_ref *, tree, tree);
 extern tree ao_ref_base (ao_ref *);
 extern alias_set_type ao_ref_alias_set (ao_ref *);
+extern alias_set_type ao_ref_base_alias_set (ao_ref *);
 extern bool ptr_deref_may_alias_global_p (tree);
 extern bool ptr_derefs_may_alias_p (tree, tree);
 extern bool refs_may_alias_p (tree, tree);
--- gcc/cse.c.jj	2011-06-06 10:24:41.000000000 +0200
+++ gcc/cse.c	2011-06-13 17:11:18.000000000 +0200
@@ -2679,6 +2679,19 @@  exp_equiv_p (const_rtx x, const_rtx y, i
 	     other.  */
 	  if (MEM_VOLATILE_P (x) || MEM_VOLATILE_P (y))
 	    return 0;
+
+	  /* First check the address before testing MEM_ATTRS.  */
+	  if (!exp_equiv_p (XEXP (x, 0), XEXP (y, 0), validate, for_gcse))
+	    return 0;
+
+	  /* If MEM_ATTRS are different, generally we can't merge
+	     the MEMs, as alias oracle could behave differently for them.
+	     There are a few exceptions where we can detect it will behave
+	     the same though.  */
+	  if (MEM_ATTRS (x) != MEM_ATTRS (y) && !rtx_mem_attrs_equiv_p (x, y))
+	    return 0;
+
+	  return 1;
 	}
       break;
 
--- gcc/testsuite/gcc.c-torture/execute/pr49390.c.jj	2011-06-13 17:28:09.000000000 +0200
+++ gcc/testsuite/gcc.c-torture/execute/pr49390.c	2011-06-13 17:27:49.000000000 +0200
@@ -0,0 +1,88 @@ 
+/* PR rtl-optimization/49390 */
+
+struct S { unsigned int s1; unsigned int s2; };
+struct T { unsigned int t1; struct S t2; };
+struct U { unsigned short u1; unsigned short u2; };
+struct V { struct U v1; struct T v2; };
+struct S a;
+char *b;
+union { char b[64]; struct V v; } u;
+volatile int v;
+extern void abort (void);
+
+__attribute__((noinline, noclone)) void
+foo (int x, void *y, unsigned int z, unsigned int w)
+{
+  if (x != 4 || y != (void *) &u.v.v2)
+    abort ();
+  v = z + w;
+  v = 16384;
+}
+
+__attribute__((noinline, noclone)) void
+bar (struct S x)
+{
+  v = x.s1;
+  v = x.s2;
+}
+
+__attribute__((noinline, noclone)) int
+baz (struct S *x)
+{
+  v = x->s1;
+  v = x->s2;
+  v = 0;
+  return v + 1;
+}
+
+__attribute__((noinline, noclone)) void
+test (struct S *c)
+{
+  struct T *d;
+  struct S e = a;
+  unsigned int f, g;
+  if (c == 0)
+    c = &e;
+  else
+    {
+      if (c->s2 % 8192 <= 15 || (8192 - c->s2 % 8192) <= 31)
+	foo (1, 0, c->s1, c->s2);
+    }
+  if (!baz (c))
+    return;
+  g = (((struct U *) b)->u2 & 2) ? 32 : __builtin_offsetof (struct V, v2);
+  f = c->s2 % 8192;
+  if (f == 0)
+    {
+      e.s2 += g;
+      f = g;
+    }
+  else if (f < g)
+    {
+      foo (2, 0, c->s1, c->s2);
+      return;
+    }
+  if ((((struct U *) b)->u2 & 1) && f == g)
+    {
+      bar (*c);
+      foo (3, 0, c->s1, c->s2);
+      return;
+    }
+  d = (struct T *) (b + c->s2 % 8192);
+  if (d->t2.s1 >= c->s1 && (d->t2.s1 != c->s1 || d->t2.s2 >= c->s2))
+    foo (4, d, c->s1, c->s2);
+  return;
+}
+
+int
+main ()
+{
+  struct S *c = 0;
+  asm ("" : "+r" (c) : "r" (&a));
+  u.v.v2.t2.s1 = 8192;
+  b = u.b;
+  test (c);
+  if (v != 16384)
+    abort ();
+  return 0;
+}