On 12/18/12, Jakub Jelinek <ja...@redhat.com> wrote:
> The bitmap unification changes apparently broke at least modulo
> scheduling, which used sbitmap_a_and_b_cg functions, which
> were replaced by bitmap_and. But, sbitmap_a_and_b_cg asserted
> dst->popcount is NULL and returned whether dst sbitmap changed,
> but bitmap_and returns always false if dst->popcount is NULL
> (and only if it is non-NULL returns if the bitmap changed).
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and
> i686-linux, ok for trunk?
The changes look okay to me. (But I don't approve trunk.)
> Alternatively we could add back separate functions that would
> return whether dest changed for speed reasons (similarly to
> the old *_cg ones), and return void from the normal ones again.
> I bet most of the users don't check the return value of these
> functions and thus it is useless computation.
The discussion at the time was pretty conclusive to not have
separate functions.
>
> 2012-12-18 Jakub Jelinek <ja...@redhat.com>
>
> PR target/55562
> * sbitmap.c (bitmap_and, bitmap_xor, bitmap_ior): Return whether
> dst sbitmap changed even if it doesn't have popcount.
>
> --- gcc/sbitmap.c.jj 2012-11-02 09:01:54.000000000 +0100
> +++ gcc/sbitmap.c 2012-12-18 14:29:13.695299294 +0100
> @@ -434,28 +434,26 @@ bitmap_and (sbitmap dst, const_sbitmap a
> const_sbitmap_ptr bp = b->elms;
> bool has_popcount = dst->popcount != NULL;
> unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
> for (i = 0; i < n; i++)
> {
> const SBITMAP_ELT_TYPE tmp = *ap++ & *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
> if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
> *dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
> if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Set DST to be (A xor B)).
> @@ -470,28 +468,26 @@ bitmap_xor (sbitmap dst, const_sbitmap a
> const_sbitmap_ptr bp = b->elms;
> bool has_popcount = dst->popcount != NULL;
> unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
> for (i = 0; i < n; i++)
> {
> const SBITMAP_ELT_TYPE tmp = *ap++ ^ *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
> if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
> *dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
> if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Set DST to be (A or B)).
> @@ -506,28 +502,26 @@ bitmap_ior (sbitmap dst, const_sbitmap a
> const_sbitmap_ptr bp = b->elms;
> bool has_popcount = dst->popcount != NULL;
> unsigned char *popcountp = dst->popcount;
> - bool anychange = false;
> + SBITMAP_ELT_TYPE changed = 0;
>
> for (i = 0; i < n; i++)
> {
> const SBITMAP_ELT_TYPE tmp = *ap++ | *bp++;
> + SBITMAP_ELT_TYPE wordchanged = *dstp ^ tmp;
> if (has_popcount)
> {
> - bool wordchanged = (*dstp ^ tmp) != 0;
> if (wordchanged)
> - {
> - *popcountp = do_popcount (tmp);
> - anychange = true;
> - }
> + *popcountp = do_popcount (tmp);
> popcountp++;
> }
> *dstp++ = tmp;
> + changed |= wordchanged;
> }
> #ifdef BITMAP_DEBUGGING
> if (has_popcount)
> sbitmap_verify_popcount (dst);
> #endif
> - return anychange;
> + return changed != 0;
> }
>
> /* Return nonzero if A is a subset of B. */
--
Lawrence Crowl
