Patchwork Fix up sbitmap bitmap_{and,ior,xor} (PR target/55562)

login
register
mail settings
Submitter Jakub Jelinek
Date Dec. 18, 2012, 4:47 p.m.
Message ID <20121218164724.GS2315@tucnak.redhat.com>
Download mbox | patch
Permalink /patch/207155/
State New
Headers show

Comments

Jakub Jelinek - Dec. 18, 2012, 4:47 p.m.
Hi!

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?

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.

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.


	Jakub
Lawrence Crowl - Dec. 18, 2012, 7:10 p.m.
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.  */
Richard Henderson - Dec. 18, 2012, 9:35 p.m.
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~

Patch

--- 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.  */