Message ID | 20121218164724.GS2315@tucnak.redhat.com |
---|---|
State | New |
Headers | show |
On 12/18/12, Jakub Jelinek <jakub@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 <jakub@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. */
On 12/18/2012 08:47 AM, Jakub Jelinek wrote: > 2012-12-18 Jakub Jelinek <jakub@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. Ok. r~
--- 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. */