| Message ID | 20170704113247.GI2123@tucnak |
|---|---|
| State | New |
| Headers | show |
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.
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
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.
--- 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);