Patchwork Fix incorrect byte swap detection (PR tree-optimization/60454)

login
register
mail settings
Submitter Thomas Preud'homme
Date March 11, 2014, 10:48 a.m.
Message ID <000401cf3d17$76a460a0$63ed21e0$@arm.com>
Download mbox | patch
Permalink /patch/329031/
State New
Headers show

Comments

Thomas Preud'homme - March 11, 2014, 10:48 a.m.
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> 
> The ChangeLog entry is wrong, the file is new, so you should just say:
> 	* gcc.c-torture/execute/pr60454.c: New test.

Done.

> 
> But more importantly:
> 
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
> > @@ -0,0 +1,25 @@
> > +#include <stdint.h>
> 
> I'm not sure if all targets even provide stdint.h header, and if all targets
> have uint32_t type.  For most targets gcc now provides stdint.h (or uses
> glibc stdint.h).
> So perhaps instead of including stdint.h just do:
> #ifndef __UINT32_TYPE__
> int
> main ()
> {
>   return 0;
> }
> #else
> typedef __UINT32_TYPE__ uint32_t;
> 
> ...
> #endif
> ?

I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right.

> > +
> > +#define __fake_const_swab32(x) ((uint32_t)(			      \
> > +        (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) |            \
> > +        (((uint32_t)(x) & (uint32_t)0x0000ff00UL) <<  8) |            \
> > +        (((uint32_t)(x) & (uint32_t)0x000000ffUL) <<  8) |            \
> > +        (((uint32_t)(x) & (uint32_t)0x0000ff00UL)      ) |            \
> > +        (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24)))
> > +
> > +/* Previous version of bswap optimization would detect byte swap when
> none
> > +   happen. This test aims at catching such wrong detection to avoid
> > +   regressions.  */
> > +
> > +uint32_t
> > +fake_swap32 (uint32_t in) __attribute__ ((noinline, noclone))
> > +{
> 
> This must not have been tested, this is a syntax error.
> It should be
> __attribute__ (noinline, noclone)) uint32_t
> fake_swap32 (uint32_t in)
> {

My bad, I did catched this error but changed it only in a copy of the sources but produced the patch from git. I moved the attribute before the function name but after the return value though and it did compile and run. I used your version when redoing the patch.

> ...
> 
> (the attribute can be after the arguments only for prototypes).

That's why it always sesms to be different than in my memories. I'll try to remember that (or always use prototypes in such case).

> 
> > +  return __fake_const_swab32 (in);
> > +}
> > +
> > +int main(void)
> > +{
> > +  if (fake_swap32 (0x12345678) != 0x78567E12)
> > +    abort ();
> 
> Use __builtin_abort (); here instead?  You aren't including stdlib.h
> for abort.

Again, I must have copied the extern void abort() definition I've seen elsewhere but made the change only on a local copy of the sources. My apologizes.

> 
> After fixing up the testcase, you don't need to bootstrap/regtest it again,
> make check-gcc RUNTESTFLAGS=execute.exp=pr60454.c
> should be enough.

Right, but I made another bootstrap for another patch just after so I need to do it again.

> 
> The rest looks good to me.

Thanks.

See below for the new version as well as in attachment.
Jakub Jelinek - March 11, 2014, 10:59 a.m.
On Tue, Mar 11, 2014 at 06:48:37PM +0800, Thomas Preud'homme wrote:
> I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right.

In theory you could have __CHAR_BIT__ different from 8 and what you care
about is that uint32_t has exactly 32 bits, so the check would need to be
  if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
    return 0;

> +  if (fake_swap32 (0x12345678) != 0x78567E12)
> +    __builtin_abort ();

Also, for int16 targets where __UINT32_TYPE__ is supposedly unsigned long,
I think you would need to use:

  if (fake_swap32 (0x12345678UL) != 0x78567E12UL)
    __builtin_abort ();

(the C standard guarantees that unsigned long is at least 32-bit and
unsigned int at least 16-bit).

Ok with those changes.

Do you have write access, or will somebody from your coworkers commit it for
you?  Are you covered by ARM GCC Copyright assignment?

	Jakub
Michael Eager - March 11, 2014, 11:43 a.m.
On 03/11/14 03:48, Thomas Preud'homme wrote:

> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 748805e..b6d7d93 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,8 @@
> +2014-03-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> +
> +	PR tree-optimization/60454
> +	* tree-ssa-math-opts.c (find_bswap_1): Fix bswap detection.
> +
>   2014-02-23  David Holsgrove <david.holsgrove@xilinx.com>
>
>   	* config/microblaze/microblaze.md: Correct ashrsi_reg / lshrsi_reg names

Don't include the ChangeLog entry as part of your patch.
It isn't likely to apply.
Ye Joey - March 12, 2014, 3:43 a.m.
4.8 also has this bug. OK to backport?

On Tue, Mar 11, 2014 at 6:59 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Tue, Mar 11, 2014 at 06:48:37PM +0800, Thomas Preud'homme wrote:
>> I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right.
>
> In theory you could have __CHAR_BIT__ different from 8 and what you care
> about is that uint32_t has exactly 32 bits, so the check would need to be
>   if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
>     return 0;
>
>> +  if (fake_swap32 (0x12345678) != 0x78567E12)
>> +    __builtin_abort ();
>
> Also, for int16 targets where __UINT32_TYPE__ is supposedly unsigned long,
> I think you would need to use:
>
>   if (fake_swap32 (0x12345678UL) != 0x78567E12UL)
>     __builtin_abort ();
>
> (the C standard guarantees that unsigned long is at least 32-bit and
> unsigned int at least 16-bit).
>
> Ok with those changes.
>
> Do you have write access, or will somebody from your coworkers commit it for
> you?  Are you covered by ARM GCC Copyright assignment?
>
>         Jakub
Jakub Jelinek - March 12, 2014, 7:08 a.m.
On Wed, Mar 12, 2014 at 11:43:00AM +0800, Joey Ye wrote:
> 4.8 also has this bug. OK to backport?

Yes.

	Jakub

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 748805e..b6d7d93 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-03-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>
+
+	PR tree-optimization/60454
+	* tree-ssa-math-opts.c (find_bswap_1): Fix bswap detection.
+
 2014-02-23  David Holsgrove <david.holsgrove@xilinx.com>
 
 	* config/microblaze/microblaze.md: Correct ashrsi_reg / lshrsi_reg names
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f3c0c85..04ce403 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-03-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
+
+	PR tree-optimization/60454
+	* gcc.c-torture/execute/pr60454.c: New test.
+
 2014-02-23  David Holsgrove <david.holsgrove@xilinx.com>
 
 	* gcc/testsuite/gcc.target/microblaze/others/mem_reload.c: New test.
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr60454.c b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
new file mode 100644
index 0000000..83f2987
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
@@ -0,0 +1,31 @@ 
+#ifdef __UINT32_TYPE__
+typedef __UINT32_TYPE__ uint32_t;
+#else
+typedef unsigned uint32_t;
+#endif
+
+#define __fake_const_swab32(x) ((uint32_t)(			      \
+        (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) |            \
+        (((uint32_t)(x) & (uint32_t)0x0000ff00UL) <<  8) |            \
+        (((uint32_t)(x) & (uint32_t)0x000000ffUL) <<  8) |            \
+        (((uint32_t)(x) & (uint32_t)0x0000ff00UL)      ) |            \
+        (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24)))
+
+/* Previous version of bswap optimization would detect byte swap when none
+   happen. This test aims at catching such wrong detection to avoid
+   regressions.  */
+
+__attribute__ ((noinline, noclone)) uint32_t
+fake_swap32 (uint32_t in)
+{
+  return __fake_const_swab32 (in);
+}
+
+int main(void)
+{
+  if (sizeof (uint32_t) != 4)
+    return 0;
+  if (fake_swap32 (0x12345678) != 0x78567E12)
+    __builtin_abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 8e372ed..9ff857c 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1801,7 +1801,9 @@  find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
 
   if (rhs_class == GIMPLE_BINARY_RHS)
     {
+      int i;
       struct symbolic_number n1, n2;
+      unsigned HOST_WIDEST_INT mask;
       tree source_expr2;
 
       if (code != BIT_IOR_EXPR)
@@ -1827,6 +1829,15 @@  find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
 	    return NULL_TREE;
 
 	  n->size = n1.size;
+	  for (i = 0, mask = 0xff; i < n->size; i++, mask <<= BITS_PER_UNIT)
+	    {
+	      unsigned HOST_WIDEST_INT masked1, masked2;
+
+	      masked1 = n1.n & mask;
+	      masked2 = n2.n & mask;
+	      if (masked1 && masked2 && masked1 != masked2)
+		return NULL_TREE;
+	    }
 	  n->n = n1.n | n2.n;
 
 	  if (!verify_symbolic_number_p (n, stmt))