* RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)
@ 2011-06-13 18:57 Jakub Jelinek
2011-06-14 9:10 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2011-06-13 18:57 UTC (permalink / raw)
To: Richard Guenther, Bernd Schmidt; +Cc: gcc-patches
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.
--- 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;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)
2011-06-13 18:57 RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390) Jakub Jelinek
@ 2011-06-14 9:10 ` Richard Guenther
2011-06-14 9:40 ` Bernd Schmidt
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2011-06-14 9:10 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: Bernd Schmidt, gcc-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)
2011-06-14 9:10 ` Richard Guenther
@ 2011-06-14 9:40 ` Bernd Schmidt
2011-06-14 10:03 ` Richard Guenther
0 siblings, 1 reply; 5+ messages in thread
From: Bernd Schmidt @ 2011-06-14 9:40 UTC (permalink / raw)
To: Richard Guenther; +Cc: Jakub Jelinek, gcc-patches
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)
2011-06-14 9:40 ` Bernd Schmidt
@ 2011-06-14 10:03 ` Richard Guenther
2011-06-14 15:32 ` Jakub Jelinek
0 siblings, 1 reply; 5+ messages in thread
From: Richard Guenther @ 2011-06-14 10:03 UTC (permalink / raw)
To: Bernd Schmidt; +Cc: Jakub Jelinek, gcc-patches
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.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390)
2011-06-14 10:03 ` Richard Guenther
@ 2011-06-14 15:32 ` Jakub Jelinek
0 siblings, 0 replies; 5+ messages in thread
From: Jakub Jelinek @ 2011-06-14 15:32 UTC (permalink / raw)
To: Richard Guenther; +Cc: Bernd Schmidt, gcc-patches
On Tue, Jun 14, 2011 at 11:49:08AM +0200, Richard Guenther wrote:
> So I'd say we revert your patch for now and if somebody feels like
> implementing the above ...
Ok, here is what I've bootstrapped/regtested on x86_64-linux and i686-linux
and committed to trunk and 4.6 branch:
2011-06-14 Jakub Jelinek <jakub@redhat.com>
PR rtl-optimization/49390
Revert:
2010-06-29 Bernd Schmidt <bernds@codesourcery.com>
* cse.c (exp_equiv_p): For MEMs, if for_gcse, only compare
MEM_ALIAS_SET.
* gcc.c-torture/execute/pr49390.c: New test.
--- gcc/cse.c.jj (revision 161534)
+++ gcc/cse.c (revision 161533)
@@ -2669,16 +2669,26 @@
case MEM:
if (for_gcse)
{
- /* Can't merge two expressions in different alias sets, since we
- can decide that the expression is transparent in a block when
- it isn't, due to it being set with the different alias set. */
- if (MEM_ALIAS_SET (x) != MEM_ALIAS_SET (y))
- return 0;
-
/* A volatile mem should not be considered equivalent to any
other. */
if (MEM_VOLATILE_P (x) || MEM_VOLATILE_P (y))
return 0;
+
+ /* Can't merge two expressions in different alias sets, since we
+ can decide that the expression is transparent in a block when
+ it isn't, due to it being set with the different alias set.
+
+ Also, can't merge two expressions with different MEM_ATTRS.
+ They could e.g. be two different entities allocated into the
+ same space on the stack (see e.g. PR25130). In that case, the
+ MEM addresses can be the same, even though the two MEMs are
+ absolutely not equivalent.
+
+ But because really all MEM attributes should be the same for
+ equivalent MEMs, we just use the invariant that MEMs that have
+ the same attributes share the same mem_attrs data structure. */
+ if (MEM_ATTRS (x) != MEM_ATTRS (y))
+ return 0;
}
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;
+}
Jakub
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-06-14 15:26 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-13 18:57 RFC: Fix GCSE exp_equiv_p on MEMs with different MEM_ATTRS (PR rtl-optimization/49390) Jakub Jelinek
2011-06-14 9:10 ` Richard Guenther
2011-06-14 9:40 ` Bernd Schmidt
2011-06-14 10:03 ` Richard Guenther
2011-06-14 15:32 ` Jakub Jelinek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).