On January 9, 2016 5:08:51 AM GMT+01:00, Alexandre Oliva <aol...@redhat.com>
wrote:
>Here are two patches related with PR69123, an infinite dataflow loop in
>VTA.
>
>The first non-comment hunk in var-tracking.c:drop_overlapping_mem_locs
>is what fixes the problem, but the other changes in the first patch fix
>similar problems that might cause other such oscillations.
>
>The second patch adds some more information to detailed vartrack dumps,
>avoiding short-circuiting of dataflow set compares and dumping added
>and
>removed locations for variables present in both sets.
>
>The patches are largely independent, but they were successfully
>regstrapped together on x86_64-linux-gnu and i686-linux-gnu. They were
>only compile-tested separately.
>
>Ok to install?
OK.
Thanks,
Richard.
>
>[PR69123] fix handling of MEMs in VTA to avoid dataflow oscillation
>
>From: Alexandre Oliva <aol...@redhat.com>
>
>The problem arises because we used to drop overwritten MEMs from loc
>lists of VALUEs, but not of other onepart variables, and it just so
>happens that, by doing so, block 6 in the testcase has no D#5 in its
>output in the first pass, because the MEM holding its (previous) value
>was correctly dropped from value 88:88, but gains it in the second
>pass because D#5 has the MEM location incoming directly in its loc
>list, rather than indirectly in a VALUE.
>
>This incorrect binding enables other blocks to believe they have a
>tentative binding for D#5 in some cycles, but others, still operating
>on the early conclusion, believe there isn't, and they oscillate from
>that.
>
>Since we check for escaping MEMs in clobbers, we won't lose anything
>relevant by dropping call-clobbered or overwritten MEMs in all onepart
>variables, and this ensures the loc intersection operation in onepart
>vars won't let a MEM through that wasn't present in earlier
>iterations.
>
>for gcc/ChangeLog
>
> PR bootstrap/69123
> * var-tracking.c (drop_overlapping_mem_locs): Operate on all
> onepart vars. Fix typo in comment. Fix reversed condition in
> unshare test.
> (dataflow_set_remove_mem_locs): Operate on all onepart vars.
>
>for gcc/testsuite/ChangeLog
>
> PR bootstrap/69123
> * gcc.dg/pr69123.c: New.
>---
>gcc/testsuite/gcc.dg/pr69123.c | 95
>++++++++++++++++++++++++++++++++++++++++
> gcc/var-tracking.c | 12 +++--
> 2 files changed, 101 insertions(+), 6 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/pr69123.c
>
>diff --git a/gcc/testsuite/gcc.dg/pr69123.c
>b/gcc/testsuite/gcc.dg/pr69123.c
>new file mode 100644
>index 0000000..0546e20
>--- /dev/null
>+++ b/gcc/testsuite/gcc.dg/pr69123.c
>@@ -0,0 +1,95 @@
>+/* { dg-do compile } */
>+/* { dg-options "-O3 -g" } */
>+
>+/* This was reduced from gcc/tree-vect-slp.c by H.J.Lu. */
>+
>+struct xxx_def;
>+typedef xxx_def *xxx;
>+
>+union rtxxx
>+{
>+ const char *rt_str;
>+ xxx rt_xxx;
>+};
>+
>+struct xxx_def {
>+ union u {
>+ rtxxx fld[1];
>+ } u;
>+};
>+
>+extern xxx bar (void);
>+extern int foo1 (xxx);
>+
>+static inline xxx
>+foo2 (xxx arg0, xxx arg1)
>+{
>+ xxx rt;
>+ rt = bar ();
>+ (((rt)->u.fld[0]).rt_xxx) = arg0;
>+ (((rt)->u.fld[1]).rt_xxx) = arg1;
>+ return rt;
>+}
>+
>+static inline xxx
>+foo4 (const char *arg0 )
>+{
>+ xxx rt;
>+ rt = bar ();
>+ (((rt)->u.fld[0]).rt_str) = arg0;
>+ (((rt)->u.fld[1]).rt_xxx) = (xxx) 0;
>+ return rt;
>+}
>+
>+extern xxx foo5 (long);
>+
>+struct address_cost_data
>+{
>+ unsigned costs[2][2][2][2];
>+};
>+
>+void
>+get_address_cost (address_cost_data *data)
>+{
>+ unsigned acost;
>+ long i;
>+ long rat, off = 0;
>+ unsigned sym_p, var_p, off_p, rat_p;
>+ xxx addr, base;
>+ xxx reg0, reg1;
>+
>+ reg1 = bar ();
>+ addr = foo2 (reg1, (xxx) 0);
>+ rat = 1;
>+ acost = 0;
>+ reg0 = bar ();
>+ reg1 = bar ();
>+
>+ for (i = 0; i < 16; i++)
>+ {
>+ sym_p = i & 1;
>+ var_p = (i >> 1) & 1;
>+ off_p = (i >> 2) & 1;
>+ rat_p = (i >> 3) & 1;
>+
>+ addr = reg0;
>+ if (rat_p)
>+ addr = foo2 (addr, foo5 (rat)) ;
>+
>+ if (var_p)
>+ addr = foo2 (addr, reg1);
>+
>+ if (sym_p)
>+ base = foo4 ("");
>+ else if (off_p)
>+ base = foo5 (off);
>+ else
>+ base = (xxx) 0;
>+
>+ if (base)
>+ addr = foo2 (addr, base);
>+
>+ acost = foo1 (addr);
>+ data->costs[sym_p][var_p][off_p][rat_p] = acost;
>+ }
>+}
>diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
>index 634ebe0..a8931f3 100644
>--- a/gcc/var-tracking.c
>+++ b/gcc/var-tracking.c
>@@ -2224,7 +2224,7 @@ struct overlapping_mems
> };
>
> /* Remove all MEMs that overlap with COMS->LOC from the location list
>- of a hash table entry for a value. COMS->ADDR must be a
>+ of a hash table entry for a onepart variable. COMS->ADDR must be a
> canonicalized form of COMS->LOC's address, and COMS->LOC must be
> canonicalized itself. */
>
>@@ -2235,7 +2235,7 @@ drop_overlapping_mem_locs (variable **slot,
>overlapping_mems *coms)
> rtx mloc = coms->loc, addr = coms->addr;
> variable *var = *slot;
>
>- if (var->onepart == ONEPART_VALUE)
>+ if (var->onepart != NOT_ONEPART)
> {
> location_chain *loc, **locp;
> bool changed = false;
>@@ -4682,11 +4682,11 @@ dataflow_set_preserve_mem_locs (variable
>**slot, dataflow_set *set)
> {
> for (loc = var->var_part[0].loc_chain; loc; loc = loc->next)
> {
>- /* We want to remove dying MEMs that doesn't refer to DECL. */
>+ /* We want to remove dying MEMs that don't refer to DECL. */
> if (GET_CODE (loc->loc) == MEM
> && (MEM_EXPR (loc->loc) != decl
> || INT_MEM_OFFSET (loc->loc) != 0)
>- && !mem_dies_at_call (loc->loc))
>+ && mem_dies_at_call (loc->loc))
> break;
> /* We want to move here MEMs that do refer to DECL. */
> else if (GET_CODE (loc->loc) == VALUE
>@@ -4769,14 +4769,14 @@ dataflow_set_preserve_mem_locs (variable
>**slot, dataflow_set *set)
> }
>
> /* Remove all MEMs from the location list of a hash table entry for a
>- value. */
>+ onepart variable. */
>
> int
> dataflow_set_remove_mem_locs (variable **slot, dataflow_set *set)
> {
> variable *var = *slot;
>
>- if (var->onepart == ONEPART_VALUE)
>+ if (var->onepart != NOT_ONEPART)
> {
> location_chain *loc, **locp;
> bool changed = false;
>
>
>[PR69123] make dataflow_set_different details more verbose
>
>From: Alexandre Oliva <aol...@redhat.com>
>
>for gcc/ChangeLog
>
> PR bootstrap/69123
> * var-tracking.c (dump_onepart_variable_differences): New.
> (dataflow_set_different): If a detailed dump is requested,
> delay early returns and dump differences between onepart
> variables present before and after, and added variables.
>---
>gcc/var-tracking.c | 113
>+++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 103 insertions(+), 10 deletions(-)
>
>diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c
>index a5cca2b..634ebe0 100644
>--- a/gcc/var-tracking.c
>+++ b/gcc/var-tracking.c
>@@ -4921,6 +4921,63 @@ onepart_variable_different_p (variable *var1,
>variable *var2)
> return lc1 != lc2;
> }
>
>+/* Return true if one-part variables VAR1 and VAR2 are different.
>+ They must be in canonical order. */
>+
>+static void
>+dump_onepart_variable_differences (variable *var1, variable *var2)
>+{
>+ location_chain *lc1, *lc2;
>+
>+ gcc_assert (var1 != var2);
>+ gcc_assert (dump_file);
>+ gcc_assert (dv_as_opaque (var1->dv) == dv_as_opaque (var2->dv));
>+ gcc_assert (var1->n_var_parts == 1
>+ && var2->n_var_parts == 1);
>+
>+ lc1 = var1->var_part[0].loc_chain;
>+ lc2 = var2->var_part[0].loc_chain;
>+
>+ gcc_assert (lc1 && lc2);
>+
>+ while (lc1 && lc2)
>+ {
>+ switch (loc_cmp (lc1->loc, lc2->loc))
>+ {
>+ case -1:
>+ fprintf (dump_file, "removed: ");
>+ print_rtl_single (dump_file, lc1->loc);
>+ lc1 = lc1->next;
>+ continue;
>+ case 0:
>+ break;
>+ case 1:
>+ fprintf (dump_file, "added: ");
>+ print_rtl_single (dump_file, lc2->loc);
>+ lc2 = lc2->next;
>+ continue;
>+ default:
>+ gcc_unreachable ();
>+ }
>+ lc1 = lc1->next;
>+ lc2 = lc2->next;
>+ }
>+
>+ while (lc1)
>+ {
>+ fprintf (dump_file, "removed: ");
>+ print_rtl_single (dump_file, lc1->loc);
>+ lc1 = lc1->next;
>+ }
>+
>+ while (lc2)
>+ {
>+ fprintf (dump_file, "added: ");
>+ print_rtl_single (dump_file, lc2->loc);
>+ lc2 = lc2->next;
>+ }
>+}
>+
> /* Return true if variables VAR1 and VAR2 are different. */
>
> static bool
>@@ -4964,19 +5021,32 @@ dataflow_set_different (dataflow_set *old_set,
>dataflow_set *new_set)
> {
> variable_iterator_type hi;
> variable *var1;
>+ bool diffound = false;
>+ bool details = (dump_file && (dump_flags & TDF_DETAILS));
>+
>+#define RETRUE \
>+ do \
>+ { \
>+ if (!details) \
>+ return true; \
>+ else \
>+ diffound = true; \
>+ } \
>+ while (0)
>
> if (old_set->vars == new_set->vars)
> return false;
>
> if (shared_hash_htab (old_set->vars)->elements ()
> != shared_hash_htab (new_set->vars)->elements ())
>- return true;
>+ RETRUE;
>
> FOR_EACH_HASH_TABLE_ELEMENT (*shared_hash_htab (old_set->vars),
> var1, variable, hi)
> {
> variable_table_type *htab = shared_hash_htab (new_set->vars);
>variable *var2 = htab->find_with_hash (var1->dv, dv_htab_hash
>(var1->dv));
>+
> if (!var2)
> {
> if (dump_file && (dump_flags & TDF_DETAILS))
>@@ -4984,26 +5054,49 @@ dataflow_set_different (dataflow_set *old_set,
>dataflow_set *new_set)
> fprintf (dump_file, "dataflow difference found: removal of:\n");
> dump_var (var1);
> }
>- return true;
>+ RETRUE;
> }
>-
>- if (variable_different_p (var1, var2))
>+ else if (variable_different_p (var1, var2))
> {
>- if (dump_file && (dump_flags & TDF_DETAILS))
>+ if (details)
> {
> fprintf (dump_file, "dataflow difference found: "
> "old and new follow:\n");
> dump_var (var1);
>+ if (dv_onepart_p (var1->dv))
>+ dump_onepart_variable_differences (var1, var2);
> dump_var (var2);
> }
>- return true;
>+ RETRUE;
> }
> }
>
>- /* No need to traverse the second hashtab, if both have the same
>number
>- of elements and the second one had all entries found in the first
>one,
>- then it can't have any extra entries. */
>- return false;
>+ /* There's no need to traverse the second hashtab unless we want to
>+ print the details. If both have the same number of elements and
>+ the second one had all entries found in the first one, then the
>+ second can't have any extra entries. */
>+ if (!details)
>+ return diffound;
>+
>+ FOR_EACH_HASH_TABLE_ELEMENT (*shared_hash_htab (new_set->vars),
>+ var1, variable, hi)
>+ {
>+ variable_table_type *htab = shared_hash_htab (old_set->vars);
>+ variable *var2 = htab->find_with_hash (var1->dv, dv_htab_hash
>(var1->dv));
>+ if (!var2)
>+ {
>+ if (details)
>+ {
>+ fprintf (dump_file, "dataflow difference found: addition
>of:\n");
>+ dump_var (var1);
>+ }
>+ RETRUE;
>+ }
>+ }
>+
>+#undef RETRUE
>+
>+ return diffound;
> }
>
> /* Free the contents of dataflow set SET. */
