On Feb 15, 2012, Richard Sandiford <rdsandif...@googlemail.com> wrote:
> I'm fine with putting it in and seeing what breaks. But I'd really
> prefer if we knew in theory. :-)
Ok, I dove into the problem without a testcase, and I managed to trigger
it on other platforms after tweaking get_addr a bit so as use loc lists
form canonical values, and to avoid returning other VALUEs if other
alternatives exist.
> Like I say, my understanding before this patch series went in was that
> cselib values weren't supposed to be cyclic. Now that they are, what
> should consumers like memrefs_conflict_p do to avoid getting stuck?
I'm now testing the following heuristic: only use an expr instead of a
value if the expr doesn't reference any value whose uid is greater than
that of the value. This worked for libgcc so far; regstrapping now.
Here's the revised patch that addresses Jakub's and your comments, that
regstrapped on x86_64-linux-gnu and i686-linux-gnu, followed by the
patch I'm testing now on both platforms.
for gcc/ChangeLog
from Alexandre Oliva <aol...@redhat.com>
PR debug/52001
* cselib.c (preserve_only_constants): Rename to...
(preserve_constants_and_equivs): ... this. Split out...
(invariant_or_equiv_p): ... this. Preserve plus expressions
of other preserved expressions too.
(cselib_reset_table): Adjust.
* var-tracking.c (reverse_op): Use canonical value to build
reverse operation.
Index: gcc/cselib.c
===================================================================
--- gcc/cselib.c.orig 2012-02-12 06:13:40.676385499 -0200
+++ gcc/cselib.c 2012-02-15 00:40:46.000000000 -0200
@@ -383,22 +383,29 @@ cselib_clear_table (void)
cselib_reset_table (1);
}
-/* Remove from hash table all VALUEs except constants
- and function invariants. */
+/* Return TRUE if V is a constant, a function invariant or a VALUE
+ equivalence; FALSE otherwise. */
-static int
-preserve_only_constants (void **x, void *info ATTRIBUTE_UNUSED)
+static bool
+invariant_or_equiv_p (cselib_val *v)
{
- cselib_val *v = (cselib_val *)*x;
struct elt_loc_list *l;
+ if (v == cfa_base_preserved_val)
+ return true;
+
+ /* Keep VALUE equivalences around. */
+ for (l = v->locs; l; l = l->next)
+ if (GET_CODE (l->loc) == VALUE)
+ return true;
+
if (v->locs != NULL
&& v->locs->next == NULL)
{
if (CONSTANT_P (v->locs->loc)
&& (GET_CODE (v->locs->loc) != CONST
|| !references_value_p (v->locs->loc, 0)))
- return 1;
+ return true;
/* Although a debug expr may be bound to different expressions,
we can preserve it as if it was constant, to get unification
and proper merging within var-tracking. */
@@ -406,24 +413,29 @@ preserve_only_constants (void **x, void
|| GET_CODE (v->locs->loc) == DEBUG_IMPLICIT_PTR
|| GET_CODE (v->locs->loc) == ENTRY_VALUE
|| GET_CODE (v->locs->loc) == DEBUG_PARAMETER_REF)
- return 1;
- if (cfa_base_preserved_val)
- {
- if (v == cfa_base_preserved_val)
- return 1;
- if (GET_CODE (v->locs->loc) == PLUS
- && CONST_INT_P (XEXP (v->locs->loc, 1))
- && XEXP (v->locs->loc, 0) == cfa_base_preserved_val->val_rtx)
- return 1;
- }
+ return true;
+
+ /* (plus (value V) (const_int C)) is invariant iff V is invariant. */
+ if (GET_CODE (v->locs->loc) == PLUS
+ && CONST_INT_P (XEXP (v->locs->loc, 1))
+ && GET_CODE (XEXP (v->locs->loc, 0)) == VALUE
+ && invariant_or_equiv_p (CSELIB_VAL_PTR (XEXP (v->locs->loc, 0))))
+ return true;
}
- /* Keep VALUE equivalences around. */
- for (l = v->locs; l; l = l->next)
- if (GET_CODE (l->loc) == VALUE)
- return 1;
+ return false;
+}
+
+/* Remove from hash table all VALUEs except constants, function
+ invariants and VALUE equivalences. */
+
+static int
+preserve_constants_and_equivs (void **x, void *info ATTRIBUTE_UNUSED)
+{
+ cselib_val *v = (cselib_val *)*x;
- htab_clear_slot (cselib_hash_table, x);
+ if (!invariant_or_equiv_p (v))
+ htab_clear_slot (cselib_hash_table, x);
return 1;
}
@@ -463,7 +475,7 @@ cselib_reset_table (unsigned int num)
}
if (cselib_preserve_constants)
- htab_traverse (cselib_hash_table, preserve_only_constants, NULL);
+ htab_traverse (cselib_hash_table, preserve_constants_and_equivs, NULL);
else
htab_empty (cselib_hash_table);
Index: gcc/var-tracking.c
===================================================================
--- gcc/var-tracking.c.orig 2012-02-12 06:13:38.633412886 -0200
+++ gcc/var-tracking.c 2012-02-14 23:56:52.000000000 -0200
@@ -5334,6 +5334,10 @@ reverse_op (rtx val, const_rtx expr, rtx
if (!v || !cselib_preserved_value_p (v))
return;
+ /* Use canonical V to avoid creating multiple redundant expressions
+ for different VALUES equivalent to V. */
+ v = canonical_cselib_val (v);
+
/* Adding a reverse op isn't useful if V already has an always valid
location. Ignore ENTRY_VALUE, while it is always constant, we should
prefer non-ENTRY_VALUE locations whenever possible. */
for gcc/ChangeLog
from Alexandre Oliva <aol...@redhat.com>
PR debug/52001
* alias.c (refs_newer_value_cb, refs_newer_value_p): New.
(get_addr): Walk canonical value's locs. Avoid returning VALUEs
and locs that reference values newer than the non-canonical value
at hand. Return the canonical value as a worst case.
(memrefs_conflict_p): Walk canonical value's locs.
Index: gcc/alias.c
===================================================================
--- gcc/alias.c.orig 2012-02-17 01:14:43.734347934 -0200
+++ gcc/alias.c 2012-02-17 01:47:33.506632170 -0200
@@ -1773,6 +1773,29 @@ base_alias_check (rtx x, rtx y, enum mac
return 1;
}
+/* Callback for for_each_rtx, that returns 1 upon encountering a VALUE
+ whose UID is greater than the int uid that D points to. */
+
+static int
+refs_newer_value_cb (rtx *x, void *d)
+{
+ if (GET_CODE (*x) == VALUE && CSELIB_VAL_PTR (*x)->uid > *(int *)d)
+ return 1;
+
+ return 0;
+}
+
+/* Return TRUE if EXPR refers to a VALUE whose uid is greater than
+ that of V. */
+
+static bool
+refs_newer_value_p (rtx expr, rtx v)
+{
+ int minuid = CSELIB_VAL_PTR (v)->uid;
+
+ return for_each_rtx (&expr, refs_newer_value_cb, &minuid);
+}
+
/* Convert the address X into something we can use. This is done by returning
it unchanged unless it is a value; in the latter case we call cselib to get
a more useful rtx. */
@@ -1788,14 +1811,20 @@ get_addr (rtx x)
v = CSELIB_VAL_PTR (x);
if (v)
{
+ v = canonical_cselib_val (v);
for (l = v->locs; l; l = l->next)
if (CONSTANT_P (l->loc))
return l->loc;
for (l = v->locs; l; l = l->next)
- if (!REG_P (l->loc) && !MEM_P (l->loc))
+ if (!REG_P (l->loc) && !MEM_P (l->loc) && GET_CODE (l->loc) != VALUE
+ && !refs_newer_value_p (l->loc, x))
+ return l->loc;
+ for (l = v->locs; l; l = l->next)
+ if (REG_P (l->loc) || (GET_CODE (l->loc) != VALUE
+ && !refs_newer_value_p (l->loc, x)))
return l->loc;
- if (v->locs)
- return v->locs->loc;
+ /* Return the canonical value. */
+ return v->val_rtx;
}
return x;
}
@@ -1873,7 +1902,8 @@ memrefs_conflict_p (int xsize, rtx x, in
{
struct elt_loc_list *l = NULL;
if (CSELIB_VAL_PTR (x))
- for (l = CSELIB_VAL_PTR (x)->locs; l; l = l->next)
+ for (l = canonical_cselib_val (CSELIB_VAL_PTR (x))->locs;
+ l; l = l->next)
if (REG_P (l->loc) && rtx_equal_for_memref_p (l->loc, y))
break;
if (l)
@@ -1891,7 +1921,8 @@ memrefs_conflict_p (int xsize, rtx x, in
{
struct elt_loc_list *l = NULL;
if (CSELIB_VAL_PTR (y))
- for (l = CSELIB_VAL_PTR (y)->locs; l; l = l->next)
+ for (l = canonical_cselib_val (CSELIB_VAL_PTR (y))->locs;
+ l; l = l->next)
if (REG_P (l->loc) && rtx_equal_for_memref_p (l->loc, x))
break;
if (l)
--
Alexandre Oliva, freedom fighter http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/ FSF Latin America board member
Free Software Evangelist Red Hat Brazil Compiler Engineer
