[PATCH] PR43280: Wrong code generation in bswap pass

[Andreas Krebbel]

Hi,

the attached patch fixes the problem in PR43280.

The problem was that a symbolic number generated for 32 bit looks the
same as a generated 64 bit number after shifting it 32 bits to the
right. In both cases it would be 0x01020304.

So the algorithm was not able to distingiush the following sequences:

i)  D.3142_37 = D.3001_8 >> 32;
    a_38 = (uint32_t) D.3142_37;

ii) b_39 = (uint32_t) D.3001_8;

In both cases it would choose D.3001_8 as the source of a 32 bit
bswap which is wrong for i).

The patch reverts the order when generating the symbolic number. So
that the highest order byte always has the highest number. The shift
then results in 0x08070605 which never matches a 32 bit number which
has been generated from scratch.

Bootstrapped and regtested on x86_64 and s390x.

Ok?

Bye,

-Andreas-


2010-03-11  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	PR tree-optimization/43280
	* tree-ssa-math-opts.c (find_bswap_1): Symbolic number generation
	modified.
	Calculation of size moved out of the if branch.
	(find_bswap): Compare number generation modified.


2010-03-11  Andreas Krebbel  <Andreas.Krebbel@de.ibm.com>

	* gcc.dg/optimize-bswapdi-1.c: OpenSSL bswap variant added.
	* gcc.dg/pr43280.c: New testcase.


Index: gcc/tree-ssa-math-opts.c
===================================================================
--- gcc/tree-ssa-math-opts.c.orig	2010-01-26 12:55:30.000000000 +0100
+++ gcc/tree-ssa-math-opts.c	2010-03-11 09:30:25.000000000 +0100
@@ -940,15 +940,18 @@ find_bswap_1 (gimple stmt, struct symbol
 	{
 	  /* Set up the symbolic number N by setting each byte to a
 	     value between 1 and the byte size of rhs1.  The highest
-	     order byte is set to 1 and the lowest order byte to
-	     n.size.  */
+	     order byte is set to n->size and the lowest order
+	     byte to 1.  */
 	  n->size = TYPE_PRECISION (TREE_TYPE (rhs1));
 	  if (n->size % BITS_PER_UNIT != 0)
 	    return NULL_TREE;
 	  n->size /= BITS_PER_UNIT;
 	  n->n = (sizeof (HOST_WIDEST_INT) < 8 ? 0 :
-		  (unsigned HOST_WIDEST_INT)0x01020304 << 32 | 0x05060708);
-	  n->n >>= (sizeof (HOST_WIDEST_INT) - n->size) * BITS_PER_UNIT;
+		  (unsigned HOST_WIDEST_INT)0x08070605 << 32 | 0x04030201);
+
+	  if (n->size < (int)sizeof (HOST_WIDEST_INT))
+	    n->n &= ((unsigned HOST_WIDEST_INT)1 <<
+		     (n->size * BITS_PER_UNIT)) - 1;
 
 	  source_expr1 = rhs1;
 	}
@@ -988,9 +991,9 @@ find_bswap_1 (gimple stmt, struct symbol
 	      {
 		/* If STMT casts to a smaller type mask out the bits not
 		   belonging to the target type.  */
-		n->size = type_size / BITS_PER_UNIT;
 		n->n &= ((unsigned HOST_WIDEST_INT)1 << type_size) - 1;
 	      }
+	    n->size = type_size / BITS_PER_UNIT;
 	  }
 	  break;
 	default:
@@ -1051,11 +1054,11 @@ static tree
 find_bswap (gimple stmt)
 {
 /* The number which the find_bswap result should match in order to
-   have a full byte swap.  The insignificant bytes are masked out
-   before using it.  */
+   have a full byte swap.  The number is shifted to the left according
+   to the size of the symbolic number before using it.  */
   unsigned HOST_WIDEST_INT cmp =
     sizeof (HOST_WIDEST_INT) < 8 ? 0 :
-    (unsigned HOST_WIDEST_INT)0x08070605 << 32 | 0x04030201;
+    (unsigned HOST_WIDEST_INT)0x01020304 << 32 | 0x05060708;
 
   struct symbolic_number n;
   tree source_expr;
@@ -1079,7 +1082,7 @@ find_bswap (gimple stmt)
 	((unsigned HOST_WIDEST_INT)1 << (n.size * BITS_PER_UNIT)) - 1;
 
       n.n &= mask;
-      cmp &= mask;
+      cmp >>= (sizeof (HOST_WIDEST_INT) - n.size) * BITS_PER_UNIT;
     }
 
   /* A complete byte swap should make the symbolic number to start
Index: gcc/testsuite/gcc.dg/optimize-bswapdi-1.c
===================================================================
--- gcc/testsuite/gcc.dg/optimize-bswapdi-1.c.orig	2010-02-12 17:58:36.000000000 +0100
+++ gcc/testsuite/gcc.dg/optimize-bswapdi-1.c	2010-03-10 15:48:30.000000000 +0100
@@ -43,6 +43,19 @@ swap64_b (DItype u)
 	  | (((u) & 0x00000000000000ffull) << 56));
 }
 
+/* The OpenSSL variant.  */
 
-/* { dg-final { scan-tree-dump-times "64 bit bswap implementation found at" 2 "bswap" } } */
+uint64_t
+swap64_c (uint64_t x)
+{
+  uint32_t a = x >> 32;
+  uint32_t b = (uint32_t) x;
+  return ((uint64_t) ((((((b)) >> (8)) | (((b)) << (32 - (8)))) & 0xff00ff00L)
+		      | (((((b)) << (8)) | (((b)) >> (32 - (8)))) & 0x00ff00ffL)) << 32)
+          | (uint64_t) ((((((a)) >> (8)) | (((a)) << (32 - (8)))) & 0xff00ff00L)
+			| (((((a)) << (8)) | (((a)) >> (32 - (8)))) & 0x00ff00ffL));
+}
+
+
+/* { dg-final { scan-tree-dump-times "64 bit bswap implementation found at" 3 "bswap" } } */
 /* { dg-final { cleanup-tree-dump "bswap" } } */
Index: gcc/testsuite/gcc.dg/pr43280.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gcc.dg/pr43280.c	2010-03-11 09:31:09.000000000 +0100
@@ -0,0 +1,30 @@
+/* { dg-do run } */
+/* { dg-require-effective-target stdint_types } */
+/* { dg-options "-O2" } */
+
+#include <stdint.h>
+
+extern void abort (void);
+
+uint64_t __attribute__((noinline))
+byteswap64(uint64_t x)
+{
+  uint32_t a = x >> 32;
+  uint32_t b = (uint32_t) x;
+  return ((uint64_t) ((((((b)) >> (8)) | (((b)) << (32 - (8)))) & 0xff00ff00L)
+		      | (((((b)) << (8)) | (((b)) >> (32 - (8)))) & 0x00ff00ffL)) << 32)
+          | (uint64_t) ((((((a)) >> (8)) | (((a)) << (32 - (8)))) & 0xff00ff00L)
+			| (((((a)) << (8)) | (((a)) >> (32 - (8)))) & 0x00ff00ffL));
+}
+
+int
+main ()
+{
+  uint64_t in = (uint64_t)0x01020304 << 32 | 0x05060708;
+  uint64_t cmp = (uint64_t)0x08070605 << 32 | 0x04030201;
+
+  if (cmp != byteswap64 (in))
+    abort ();
+
+  return 0;
+}