* [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
@ 2017-07-04 11:32 Jakub Jelinek
2017-07-04 11:46 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2017-07-04 11:32 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
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 <jakub@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;
@@ -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;
}
+/* 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)
+ ;
+ 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);
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);
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 11:32 [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278) Jakub Jelinek
@ 2017-07-04 11:46 ` Richard Biener
2017-07-04 11:50 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2017-07-04 11:46 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
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 <jakub@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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 11:46 ` Richard Biener
@ 2017-07-04 11:50 ` Jakub Jelinek
2017-07-04 12:00 ` Richard Biener
0 siblings, 1 reply; 7+ messages in thread
From: Jakub Jelinek @ 2017-07-04 11:50 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Tue, Jul 04, 2017 at 01:46:25PM +0200, Richard Biener wrote:
> > 2017-07-04 Jakub Jelinek <jakub@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?
That was intentional. If a->e != NULL, then we know that b->e != NULL,
because we have
else if (a->e != NULL && b->e == NULL)
return -1;
earlier. Similarly, if a->e == NULL, then we know that b-> == NULL, because
we have:
if (a->e == NULL && b->e != NULL)
return 1;
earlier.
> > @@ -6506,11 +6547,12 @@ process_assert_insertions (void)
> > }
> > }
> >
> > + asserts.qsort (compare_assert_loc_stable);
>
> please add a comment here why we re-sort.
Ok, will do.
> > 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?
Let me try that.
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 11:50 ` Jakub Jelinek
@ 2017-07-04 12:00 ` Richard Biener
2017-07-04 12:42 ` Jakub Jelinek
0 siblings, 1 reply; 7+ messages in thread
From: Richard Biener @ 2017-07-04 12:00 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On Tue, 4 Jul 2017, Jakub Jelinek wrote:
> On Tue, Jul 04, 2017 at 01:46:25PM +0200, Richard Biener wrote:
> > > 2017-07-04 Jakub Jelinek <jakub@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?
>
> That was intentional. If a->e != NULL, then we know that b->e != NULL,
> because we have
> else if (a->e != NULL && b->e == NULL)
> return -1;
> earlier. Similarly, if a->e == NULL, then we know that b-> == NULL, because
> we have:
> if (a->e == NULL && b->e != NULL)
> return 1;
> earlier.
Ah, ok. Twisty ;) I suppose jump threading will have eliminated
the extra test.
> > > @@ -6506,11 +6547,12 @@ process_assert_insertions (void)
> > > }
> > > }
> > >
> > > + asserts.qsort (compare_assert_loc_stable);
> >
> > please add a comment here why we re-sort.
>
> Ok, will do.
>
> > > 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?
>
> Let me try that.
Thanks,
Richard.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 12:00 ` Richard Biener
@ 2017-07-04 12:42 ` Jakub Jelinek
2017-07-04 14:06 ` Richard Biener
2017-07-04 15:07 ` Jeff Law
0 siblings, 2 replies; 7+ messages in thread
From: Jakub Jelinek @ 2017-07-04 12:42 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
On Tue, Jul 04, 2017 at 02:00:13PM +0200, Richard Biener wrote:
> > That was intentional. If a->e != NULL, then we know that b->e != NULL,
> > because we have
> > else if (a->e != NULL && b->e == NULL)
> > return -1;
> > earlier. Similarly, if a->e == NULL, then we know that b-> == NULL, because
> > we have:
> > if (a->e == NULL && b->e != NULL)
> > return 1;
> > earlier.
>
> Ah, ok. Twisty ;) I suppose jump threading will have eliminated
> the extra test.
In the first case maybe, I doubt it would do that after the
iterative_hash_expr calls which are likely not pure.
> > > 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?
> >
> > Let me try that.
Seems to work, so like this if it passes bootstrap/regtest?
2017-07-04 Jakub Jelinek <jakub@redhat.com>
PR debug/81278
* tree-vrp.c (compare_assert_loc): Turn into a function template
with stable template parameter. 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. If stable is true, don't use
iterative_hash_expr, on the other side allow a or b or both NULL
and sort the NULLs last.
(process_assert_insertions): Sort using compare_assert_loc<false>
instead of compare_assert_loc, later sort using
compare_assert_loc<true> 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-04 10:43:48.627706528 +0200
+++ gcc/tree-vrp.c 2017-07-04 14:37:06.823101453 +0200
@@ -6393,20 +6393,37 @@ process_assert_insertions_for (tree name
gcc_unreachable ();
}
-/* Qsort helper for sorting assert locations. */
+/* Qsort helper for sorting assert locations. If stable is true, don't
+ use iterative_hash_expr because it can be unstable for -fcompare-debug,
+ on the other side some pointers might be NULL. */
+template <bool stable>
static int
compare_assert_loc (const void *pa, const void *pb)
{
assert_locus * const a = *(assert_locus * const *)pa;
assert_locus * const b = *(assert_locus * const *)pb;
- if (! a->e && b->e)
+
+ /* If stable, some asserts might be optimized away already, sort
+ them last. */
+ if (stable)
+ {
+ 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 && ! b->e)
+ else if (a->e != NULL && b->e == NULL)
return -1;
+ /* After the above checks, we know that (a->e == NULL) == (b->e == NULL),
+ no need to test both a->e and b->e. */
+
/* 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;
@@ -6419,11 +6436,27 @@ compare_assert_loc (const void *pa, cons
else if (a->comp_code < b->comp_code)
return -1;
+ hashval_t ha, hb;
+
+ /* E.g. if a->val is ADDR_EXPR of a VAR_DECL, iterative_hash_expr
+ uses DECL_UID of the VAR_DECL, so sorting might differ between
+ -g and -g0. When doing the removal of redundant assert exprs
+ and commonization to successors, this does not matter, but for
+ the final sort needs to be stable. */
+ if (stable)
+ {
+ ha = 0;
+ hb = 0;
+ }
+ else
+ {
+ ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val, 0));
+ hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val, 0));
+ }
+
/* Break the tie using hashing and source/bb index. */
- 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;
@@ -6452,7 +6485,7 @@ process_assert_insertions (void)
auto_vec<assert_locus *, 16> asserts;
for (; loc; loc = loc->next)
asserts.safe_push (loc);
- asserts.qsort (compare_assert_loc);
+ asserts.qsort (compare_assert_loc<false>);
/* Push down common asserts to successors and remove redundant ones. */
unsigned ecnt = 0;
@@ -6506,11 +6539,14 @@ process_assert_insertions (void)
}
}
+ /* The asserts vector sorting above might be unstable for
+ -fcompare-debug, sort again to ensure a stable sort. */
+ asserts.qsort (compare_assert_loc<true>);
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);
Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 12:42 ` Jakub Jelinek
@ 2017-07-04 14:06 ` Richard Biener
2017-07-04 15:07 ` Jeff Law
1 sibling, 0 replies; 7+ messages in thread
From: Richard Biener @ 2017-07-04 14:06 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On July 4, 2017 2:41:52 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>On Tue, Jul 04, 2017 at 02:00:13PM +0200, Richard Biener wrote:
>> > That was intentional. If a->e != NULL, then we know that b->e !=
>NULL,
>> > because we have
>> > else if (a->e != NULL && b->e == NULL)
>> > return -1;
>> > earlier. Similarly, if a->e == NULL, then we know that b-> ==
>NULL, because
>> > we have:
>> > if (a->e == NULL && b->e != NULL)
>> > return 1;
>> > earlier.
>>
>> Ah, ok. Twisty ;) I suppose jump threading will have eliminated
>> the extra test.
>
>In the first case maybe, I doubt it would do that after the
>iterative_hash_expr calls which are likely not pure.
>
>> > > 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?
>> >
>> > Let me try that.
>
>Seems to work, so like this if it passes bootstrap/regtest?
OK.
Richard.
>2017-07-04 Jakub Jelinek <jakub@redhat.com>
>
> PR debug/81278
> * tree-vrp.c (compare_assert_loc): Turn into a function template
> with stable template parameter. 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. If stable is true, don't use
> iterative_hash_expr, on the other side allow a or b or both NULL
> and sort the NULLs last.
> (process_assert_insertions): Sort using compare_assert_loc<false>
> instead of compare_assert_loc, later sort using
> compare_assert_loc<true> 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-04 10:43:48.627706528 +0200
>+++ gcc/tree-vrp.c 2017-07-04 14:37:06.823101453 +0200
>@@ -6393,20 +6393,37 @@ process_assert_insertions_for (tree name
> gcc_unreachable ();
> }
>
>-/* Qsort helper for sorting assert locations. */
>+/* Qsort helper for sorting assert locations. If stable is true,
>don't
>+ use iterative_hash_expr because it can be unstable for
>-fcompare-debug,
>+ on the other side some pointers might be NULL. */
>
>+template <bool stable>
> static int
> compare_assert_loc (const void *pa, const void *pb)
> {
> assert_locus * const a = *(assert_locus * const *)pa;
> assert_locus * const b = *(assert_locus * const *)pb;
>- if (! a->e && b->e)
>+
>+ /* If stable, some asserts might be optimized away already, sort
>+ them last. */
>+ if (stable)
>+ {
>+ 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 && ! b->e)
>+ else if (a->e != NULL && b->e == NULL)
> return -1;
>
>+ /* After the above checks, we know that (a->e == NULL) == (b->e ==
>NULL),
>+ no need to test both a->e and b->e. */
>+
> /* 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;
>@@ -6419,11 +6436,27 @@ compare_assert_loc (const void *pa, cons
> else if (a->comp_code < b->comp_code)
> return -1;
>
>+ hashval_t ha, hb;
>+
>+ /* E.g. if a->val is ADDR_EXPR of a VAR_DECL, iterative_hash_expr
>+ uses DECL_UID of the VAR_DECL, so sorting might differ between
>+ -g and -g0. When doing the removal of redundant assert exprs
>+ and commonization to successors, this does not matter, but for
>+ the final sort needs to be stable. */
>+ if (stable)
>+ {
>+ ha = 0;
>+ hb = 0;
>+ }
>+ else
>+ {
>+ ha = iterative_hash_expr (a->expr, iterative_hash_expr (a->val,
>0));
>+ hb = iterative_hash_expr (b->expr, iterative_hash_expr (b->val,
>0));
>+ }
>+
> /* Break the tie using hashing and source/bb index. */
>- 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;
>@@ -6452,7 +6485,7 @@ process_assert_insertions (void)
> auto_vec<assert_locus *, 16> asserts;
> for (; loc; loc = loc->next)
> asserts.safe_push (loc);
>- asserts.qsort (compare_assert_loc);
>+ asserts.qsort (compare_assert_loc<false>);
>
>/* Push down common asserts to successors and remove redundant ones.
>*/
> unsigned ecnt = 0;
>@@ -6506,11 +6539,14 @@ process_assert_insertions (void)
> }
> }
>
>+ /* The asserts vector sorting above might be unstable for
>+ -fcompare-debug, sort again to ensure a stable sort. */
>+ asserts.qsort (compare_assert_loc<true>);
> 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);
>
>
> Jakub
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278)
2017-07-04 12:42 ` Jakub Jelinek
2017-07-04 14:06 ` Richard Biener
@ 2017-07-04 15:07 ` Jeff Law
1 sibling, 0 replies; 7+ messages in thread
From: Jeff Law @ 2017-07-04 15:07 UTC (permalink / raw)
To: Jakub Jelinek, Richard Biener; +Cc: gcc-patches
On 07/04/2017 06:41 AM, Jakub Jelinek wrote:
> On Tue, Jul 04, 2017 at 02:00:13PM +0200, Richard Biener wrote:
>>> That was intentional. If a->e != NULL, then we know that b->e != NULL,
>>> because we have
>>> else if (a->e != NULL && b->e == NULL)
>>> return -1;
>>> earlier. Similarly, if a->e == NULL, then we know that b-> == NULL, because
>>> we have:
>>> if (a->e == NULL && b->e != NULL)
>>> return 1;
>>> earlier.
>>
>> Ah, ok. Twisty ;) I suppose jump threading will have eliminated
>> the extra test.
>
> In the first case maybe, I doubt it would do that after the
> iterative_hash_expr calls which are likely not pure.
Correct. It'll have to assume that the memory objects changed.
Jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-07-04 15:07 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-04 11:32 [PATCH] Fix -fcompare-debug issues caused by recent VRP assert expr sorting changes (PR debug/81278) Jakub Jelinek
2017-07-04 11:46 ` Richard Biener
2017-07-04 11:50 ` Jakub Jelinek
2017-07-04 12:00 ` Richard Biener
2017-07-04 12:42 ` Jakub Jelinek
2017-07-04 14:06 ` Richard Biener
2017-07-04 15:07 ` Jeff Law
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).