[PATCH] PR43280: Wrong code generation in bswap pass
Richard Guenther
richard.guenther@gmail.com
Thu Mar 11 13:20:00 GMT 2010
On Thu, Mar 11, 2010 at 1:31 PM, Andreas Krebbel
<krebbel@linux.vnet.ibm.com> wrote:
> 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.
Modify symbolic number generation.
etc.
Ok with that change.
Thanks,
Richard.
>
>
> 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;
> +}
>
More information about the Gcc-patches
mailing list