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 <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?
>
> 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.
