RE: [PATCH] Fix PR63266: Keep track of impact of sign extension in bswap

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

Hi Richard,

I realized thanks to Christophe Lyon that a shift was not right: the shift count
is a number of bytes instead of a number of bits.

This extra patch fixes the problem.

ChangeLog are as follows:

*** gcc/ChangeLog ***

2014-09-26  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * tree-ssa-math-opts.c (find_bswap_or_nop_1): Fix creation of
        MARKER_BYTE_UNKNOWN markers when handling casts.

*** gcc/testsuite/ChangeLog ***

2014-10-08  Thomas Preud'homme  <thomas.preudhomme@arm.com>

        * gcc.dg/optimize-bswaphi-1.c: New bswap pass test.

diff --git a/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c b/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
index 3e51f04..18aba28 100644
--- a/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
+++ b/gcc/testsuite/gcc.dg/optimize-bswaphi-1.c
@@ -42,6 +42,20 @@ uint32_t read_be16_3 (unsigned char *data)
   return *(data + 1) | (*data << 8);
 }
 
+typedef int SItype __attribute__ ((mode (SI)));
+typedef int HItype __attribute__ ((mode (HI)));
+
+/* Test that detection of significant sign extension works correctly. This
+   checks that unknown byte marker are set correctly in cast of cast.  */
+
+HItype
+swap16 (HItype in)
+{
+  return (HItype) (((in >> 0) & 0xFF) << 8)
+		| (((in >> 8) & 0xFF) << 0);
+}
+
 /* { dg-final { scan-tree-dump-times "16 bit load in target endianness found at" 3 "bswap" } } */
-/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at" 3 "bswap" { xfail alpha*-*-* arm*-*-* } } } */
+/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at" 1 "bswap" { target alpha*-*-* arm*-*-* } } } */
+/* { dg-final { scan-tree-dump-times "16 bit bswap implementation found at" 4 "bswap" { xfail alpha*-*-* arm*-*-* } } } */
 /* { dg-final { cleanup-tree-dump "bswap" } } */
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 3c6e935..2ef2333 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1916,7 +1916,8 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 	    if (!TYPE_UNSIGNED (n->type) && type_size > old_type_size
 		&& HEAD_MARKER (n->n, old_type_size))
 	      for (i = 0; i < type_size - old_type_size; i++)
-		n->n |= MARKER_BYTE_UNKNOWN << (type_size - 1 - i);
+		n->n |= MARKER_BYTE_UNKNOWN
+			<< ((type_size - 1 - i) * BITS_PER_MARKER);
 
 	    if (type_size < 64 / BITS_PER_MARKER)
 	      {

regression testsuite run without regression on x86_64-linux-gnu and bswap tests all pass on arm-none-eabi target

Is it ok for trunk?

Best regards,

Thomas

> -----Original Message-----
> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Wednesday, September 24, 2014 4:01 PM
> To: Thomas Preud'homme
> Cc: GCC Patches
> Subject: Re: [PATCH] Fix PR63266: Keep track of impact of sign extension
> in bswap
> 
> On Tue, Sep 16, 2014 at 12:24 PM, Thomas Preud'homme
> <thomas.preudhomme@arm.com> wrote:
> > Hi all,
> >
> > The fix for PR61306 disabled bswap when a sign extension is detected.
> However this led to a test case regression (and potential performance
> regression) in case where a sign extension happens but its effect is
> canceled by other bit manipulation. This patch aims to fix that by having a
> special marker to track bytes whose value is unpredictable due to sign
> extension. If the final result of a bit manipulation doesn't contain any
> such marker then the bswap optimization can proceed.
> 
> Nice and simple idea.
> 
> Ok.
> 
> Thanks,
> Richard.
> 
> > *** gcc/ChangeLog ***
> >
> > 2014-09-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> >
> >         PR tree-optimization/63266
> >         * tree-ssa-math-opts.c (struct symbolic_number): Add comment
> about
> >         marker for unknown byte value.
> >         (MARKER_MASK): New macro.
> >         (MARKER_BYTE_UNKNOWN): New macro.
> >         (HEAD_MARKER): New macro.
> >         (do_shift_rotate): Mark bytes with unknown values due to sign
> >         extension when doing an arithmetic right shift. Replace hardcoded
> >         mask for marker by new MARKER_MASK macro.
> >         (find_bswap_or_nop_1): Likewise and adjust ORing of two
> symbolic
> >         numbers accordingly.
> >
> > *** gcc/testsuite/ChangeLog ***
> >
> > 2014-09-15  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> >
> >         PR tree-optimization/63266
> >         * gcc.dg/optimize-bswapsi-1.c (swap32_d): New bswap pass test.
> >
> >
> > Testing:
> >
> > * Built an arm-none-eabi-gcc cross-compiler and used it to run the
> testsuite on QEMU emulating Cortex-M3 without any regression
> > * Bootstrapped on x86_64-linux-gnu target and testsuite was run
> without regression
> >
> >
> > Ok for trunk?