On Tue, 4 Jul 2017, Jakub Jelinek wrote:
> Hi!
>
> The compare_assert_loc added recently to sort assert exprs that could
> operand_equal_p the expressions/values in there unfortunately broke
> -fcompare-debug. The problem is that DECL_UIDs don't have to be the same
> between -g and -g0, and thus what iterative_hash_expr returns might not be
> the same. For the removal of duplicate assert exprs or moving assert expr
> to the dominator if present on all edges this doesn't matter, because
> all we care about there are the adjacent vector entries and code generation
> is not affected by the traversal order, but when we actually
> process_assert_insertions_for afterwards, the order matters a lot for
> code generation (different SSA_NAME_VERSION on the ASSERT_EXPR lhs will
> mean different order of release_ssa_name afterwards and might result
> in different SSA_NAME versions created later on in other passes and that
> in many cases affects code generation.
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk? No testcase, as the reduced testcase doesn't reproduce it (at least
> for me) and the original is way too large).
>
> 2017-07-04 Jakub Jelinek <ja...@redhat.com>
>
> PR debug/81278
> * tree-vrp.c (compare_assert_loc): Only test if a->e is NULL,
> !a->e == !b->e has been verified already. Use e == NULL or
> e != NULL instead of e or ! e tests.
> (compare_assert_loc_stable): New function.
> (process_assert_insertions): Sort using compare_assert_loc_stable
> before calling process_assert_insertions_for in a loop. Use break
> instead of continue once seen NULL pointer.
>
> --- gcc/tree-vrp.c.jj 2017-07-03 19:03:23.817824263 +0200
> +++ gcc/tree-vrp.c 2017-07-04 10:30:36.403204757 +0200
> @@ -6400,13 +6400,13 @@ compare_assert_loc (const void *pa, cons
> {
> assert_locus * const a = *(assert_locus * const *)pa;
> assert_locus * const b = *(assert_locus * const *)pb;
> - if (! a->e && b->e)
> + if (a->e == NULL && b->e != NULL)
> return 1;
> - else if (a->e && ! b->e)
> + else if (a->e != NULL && b->e == NULL)
> return -1;
>
> /* Sort after destination index. */
> - if (! a->e && ! b->e)
> + if (a->e == NULL)
> ;
> else if (a->e->dest->index > b->e->dest->index)
> return 1;
so this will now ICE if b->e is NULL, did you forget the && b->e == NULL
above?
> @@ -6423,12 +6423,53 @@ compare_assert_loc (const void *pa, cons
> hashval_t ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val,
> 0));
> hashval_t hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val,
> 0));
> if (ha == hb)
> - return (a->e && b->e
> + return (a->e != NULL
> ? a->e->src->index - b->e->src->index
> : a->bb->index - b->bb->index);
> return ha - hb;
> }
Likewise.
> +/* Qsort helper for sorting assert locations. Like the above, but
> + don't use expression hashes for the sorting to make the sorting
> + stable for -fcompare-debug. Some assert_locus pointers could
> + be NULL, sort those last. */
> +
> +static int
> +compare_assert_loc_stable (const void *pa, const void *pb)
> +{
> + assert_locus * const a = *(assert_locus * const *)pa;
> + assert_locus * const b = *(assert_locus * const *)pb;
> +
> + if (a == NULL)
> + return b != NULL;
> + else if (b == NULL)
> + return -1;
> +
> + if (a->e == NULL && b->e != NULL)
> + return 1;
> + else if (a->e != NULL && b->e == NULL)
> + return -1;
> +
> + /* Sort after destination index. */
> + if (a->e == NULL)
See above. The changelog says it has been verified already but
I can't find where. A comment in the code is warranted IMHO.
> + ;
> + else if (a->e->dest->index > b->e->dest->index)
> + return 1;
> + else if (a->e->dest->index < b->e->dest->index)
> + return -1;
> +
> + /* Sort after comp_code. */
> + if (a->comp_code > b->comp_code)
> + return 1;
> + else if (a->comp_code < b->comp_code)
> + return -1;
> +
> + /* Break the tie using source/bb index. */
> + return (a->e != NULL
> + ? a->e->src->index - b->e->src->index
> + : a->bb->index - b->bb->index);
> +}
> +
> /* Process all the insertions registered for every name N_i registered
> in NEED_ASSERT_FOR. The list of assertions to be inserted are
> found in ASSERTS_FOR[i]. */
> @@ -6506,11 +6547,12 @@ process_assert_insertions (void)
> }
> }
>
> + asserts.qsort (compare_assert_loc_stable);
please add a comment here why we re-sort.
> for (unsigned j = 0; j < asserts.length (); ++j)
> {
> loc = asserts[j];
> if (! loc)
> - continue;
> + break;
> update_edges_p |= process_assert_insertions_for (ssa_name (i), loc);
> num_asserts++;
> free (loc);
Otherwise looks ok to me. I wonder if we should merge the two
sorting functions and change behavior with a global var or a
template parameter instead (to reduce source duplication). Does
vec.qsort (function_template<true>);
work?
Thanks,
Richard.
