RE: [PATCH] Fix incorrect byte swap detection (PR tree-optimization/60454)

["Thomas Preud'homme" <thomas dot preudhomme at arm dot com>]

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

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))