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

2014-10-27 Thread Thomas Preud'homme 
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Sunday, October 26, 2014 4:40 PM
 I tried to modify check_effective_target_bswap
 and added:
 +   } else {
 +   if { [istarget arm*-*-*]
 + [check_no_compiler_messages_nocache arm_v6_or_later
 object {
 +#if __ARM_ARCH  6
 +#error not armv6 or later
 +#endif
 +int i;
 +} ] } {
 +   set et_bswap_saved 1
 +   }
 since the rev* instructions appeared in v6.
 
 Regarding the testsuite, it moves the tests to UNSUPPORTED vs a mix of
 PASS/FAIL/XFAIL

[SNIP PASS/FAIL/XFAIL changes]

 
 The PASS seems not very informative, so it may not be a problem to
 loose these few PASS/XFAIL.

Agreed. A FAIL would only mean that the test was badly written. Only
the dump is relevant to tell whether the bswap pass did its job or not.

 
 We can also explicitly skip optimize-bswaphi-1 when ARM_ARCH  6.
 
 Not sure what's preferred?

I prefer changing the effective target as it could be reused for some other 
tests
eventually. It also reflects better the reason why the test is disabled: no 
16-bit
bswap.

Best regards,

Thomas

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

2014-10-26 Thread Christophe Lyon 
On 22 October 2014 10:56, Thomas Preud'homme thomas.preudho...@arm.com wrote:
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Tuesday, October 21, 2014 10:03 PM
  +typedef int SItype __attribute__ ((mode (SI)));
 What's the purpose of this? It seems unused.

 Sigh. Bad copy/paste from optimize-bswapsi-1.c

 I'll add it to my patch for pr63259.


 I believe this should be:
 checks that unknown byte markers are set correctly in case of cast

 Indeed, there is a 's' missing for markers.


  +
  +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*-*-* } } } */

 This line fails when forcing the compiler to target -march=armv5t for
 instance. I suspect this is because the check_effective_target_bswap
 test is too permissive.

 Yep, it's likely to be the case. Feel to add a version check in it.

I tried to modify check_effective_target_bswap
and added:
+   } else {
+   if { [istarget arm*-*-*]
+ [check_no_compiler_messages_nocache arm_v6_or_later object {
+#if __ARM_ARCH  6
+#error not armv6 or later
+#endif
+int i;
+} ] } {
+   set et_bswap_saved 1
+   }
since the rev* instructions appeared in v6.

Regarding the testsuite, it moves the tests to UNSUPPORTED vs a mix of
PASS/FAIL/XFAIL
 UNSUPPORTED: gcc.dg/optimize-bswaphi-1.c
 UNSUPPORTED: gcc.dg/optimize-bswapsi-1.c
 UNSUPPORTED: gcc.dg/optimize-bswapsi-2.c
---
 PASS: gcc.dg/optimize-bswaphi-1.c (test for excess errors)
 FAIL: gcc.dg/optimize-bswaphi-1.c scan-tree-dump-times bswap 16 bit bswap 
 implementation found at 1
 XFAIL: gcc.dg/optimize-bswaphi-1.c scan-tree-dump-times bswap 16 bit bswap 
 implementation found at 4
 PASS: gcc.dg/optimize-bswaphi-1.c scan-tree-dump-times bswap 16 bit load in 
 target endianness found at 3
 PASS: gcc.dg/optimize-bswapsi-1.c (test for excess errors)
 PASS: gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit bswap 
 implementation found at 4
 PASS: gcc.dg/optimize-bswapsi-2.c (test for excess errors)
 XFAIL: gcc.dg/optimize-bswapsi-2.c scan-tree-dump-times bswap 32 bit bswap 
 implementation found at 3
 PASS: gcc.dg/optimize-bswapsi-2.c scan-tree-dump-times bswap 32 bit load in 
 target endianness found at 3

The PASS seems not very informative, so it may not be a problem to
loose these few PASS/XFAIL.

We can also explicitly skip optimize-bswaphi-1 when ARM_ARCH  6.

Not sure what's preferred?

Christophe.

 Thanks for the review.

 Best regards,

 Thomas

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

2014-10-22 Thread Thomas Preud'homme 
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Tuesday, October 21, 2014 10:03 PM
  +typedef int SItype __attribute__ ((mode (SI)));
 What's the purpose of this? It seems unused.

Sigh. Bad copy/paste from optimize-bswapsi-1.c

I'll add it to my patch for pr63259.


 I believe this should be:
 checks that unknown byte markers are set correctly in case of cast

Indeed, there is a 's' missing for markers.

 
  +
  +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*-*-* } } } */
 
 This line fails when forcing the compiler to target -march=armv5t for
 instance. I suspect this is because the check_effective_target_bswap
 test is too permissive.

Yep, it's likely to be the case. Feel to add a version check in it.

Thanks for the review.

Best regards,

Thomas

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

2014-10-21 Thread Thomas Preud'homme 
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.preudho...@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.preudho...@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.guent...@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.preudho...@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.preudho...@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.preudho...@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?

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

2014-10-21 Thread Richard Biener 
On Tue, Oct 21, 2014 at 11:28 AM, Thomas Preud'homme
thomas.preudho...@arm.com wrote:
 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.

Ok.

Thanks,
Richard.

 ChangeLog are as follows:

 *** gcc/ChangeLog ***

 2014-09-26  Thomas Preud'homme  thomas.preudho...@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.preudho...@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.guent...@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.preudho...@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.preudho...@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.preudho...@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?

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

2014-10-21 Thread Christophe Lyon 
Hi Thomas,

Some minor comments:

On 21 October 2014 11:28, Thomas Preud'homme thomas.preudho...@arm.com wrote:
 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.preudho...@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.preudho...@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)));
What's the purpose of this? It seems unused.

 +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.  */
I believe this should be:
checks that unknown byte markers are set correctly in case 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*-*-* } } } */

This line fails when forcing the compiler to target -march=armv5t for
instance. I suspect this is because the check_effective_target_bswap
test is too permissive.

Christophe.

 +/* { 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.guent...@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.preudho...@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.preudho...@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.preudho...@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

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

2014-09-26 Thread Christophe Lyon 
On 26 September 2014 04:25, Thomas Preud'homme
thomas.preudho...@arm.com wrote:
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Thursday, September 25, 2014 10:08 PM


 While attempting to try this, I noticed that more precisely the test
 is currently UNSUPPORTED on aarch64_be,
 which is because check_effective_target_bswap only accepts istarget
 aarch64-*-*.

 Ah yes, of course.


 I didn't try yet to change it into istarget aarch64*-*-*.

 It should probably be added no matter the result anyway, since this target 
 has bswap instructions.


Fixing check_effective_target_bswap to accept aarch64*-*-* makes the
test pass, so we should submit that patch.

I tried the other change you suggested, but it seem that
scan-tree-dump-times only matched 3 times. I did this in a bit of a
hurry though, so I may have done something wrong.
I'm not sure when I have time to look at that again, so I prefer to
give this little feedback now :-)

Christophe.

 Best regards,

 Thomas

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

2014-09-25 Thread Thomas Preud'homme 
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Thursday, September 25, 2014 10:08 PM
 

 While attempting to try this, I noticed that more precisely the test
 is currently UNSUPPORTED on aarch64_be,
 which is because check_effective_target_bswap only accepts istarget
 aarch64-*-*.

Ah yes, of course.

 
 I didn't try yet to change it into istarget aarch64*-*-*.

It should probably be added no matter the result anyway, since this target has 
bswap instructions.

Best regards,

Thomas

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

2014-09-25 Thread Thomas Preud'homme 
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Thursday, September 25, 2014 4:28 AM

 
 Hi Thomas,

Hi Christophe,

 
 Although I could notice the improvement:
 Pass disappears   [PASS = ]:
   gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit
 bswap
 implementation found at 3
 New pass  [ = PASS]:
   gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit
 bswap
 implementation found at 4
 
 for arm-*, armeb-* and aarch64-* targets, there is no change for
 aarch64_be: is this expected?

No, but neither is this:

@@ -1905,11 +1913,10 @@ find_bswap_or_nop_1 (gimple stmt, struct 
symbolic_number *n, int limit)
 
/* Sign extension: result is dependent on the value.  */
old_type_size = TYPE_PRECISION (n-type) / BITS_PER_UNIT;
-   if (!TYPE_UNSIGNED (n-type)
-type_size  old_type_size
-n-n  ((uint64_t) 0xff  ((old_type_size - 1)
-  * BITS_PER_MARKER)))
- return NULL;
+   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);
 
if (type_size  64 / BITS_PER_MARKER)
  {

type_size - 1 - I gives a number of marker bytes to shift. I forgot to multiply 
by the number of bits in a marker. Can you do the change locally and tell me if 
the test now succeed for aarch64_be?

Best regards,

Thomas

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

2014-09-25 Thread Christophe Lyon 
On 25 September 2014 08:39, Thomas Preud'homme
thomas.preudho...@arm.com wrote:
 From: Christophe Lyon [mailto:christophe.l...@linaro.org]
 Sent: Thursday, September 25, 2014 4:28 AM


 Hi Thomas,

 Hi Christophe,


 Although I could notice the improvement:
 Pass disappears   [PASS = ]:
   gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit
 bswap
 implementation found at 3
 New pass  [ = PASS]:
   gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit
 bswap
 implementation found at 4

 for arm-*, armeb-* and aarch64-* targets, there is no change for
 aarch64_be: is this expected?

 No, but neither is this:

 @@ -1905,11 +1913,10 @@ find_bswap_or_nop_1 (gimple stmt, struct 
 symbolic_number *n, int limit)

 /* Sign extension: result is dependent on the value.  */
 old_type_size = TYPE_PRECISION (n-type) / BITS_PER_UNIT;
 -   if (!TYPE_UNSIGNED (n-type)
 -type_size  old_type_size
 -n-n  ((uint64_t) 0xff  ((old_type_size - 1)
 -  * BITS_PER_MARKER)))
 - return NULL;
 +   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);

 if (type_size  64 / BITS_PER_MARKER)
   {

 type_size - 1 - I gives a number of marker bytes to shift. I forgot to 
 multiply by the number of bits in a marker. Can you do the change locally and 
 tell me if the test now succeed for aarch64_be?


While attempting to try this, I noticed that more precisely the test
is currently UNSUPPORTED on aarch64_be,
which is because check_effective_target_bswap only accepts istarget aarch64-*-*.

I didn't try yet to change it into istarget aarch64*-*-*.


 Best regards,

 Thomas

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

2014-09-24 Thread Richard Biener 
On Tue, Sep 16, 2014 at 12:24 PM, Thomas Preud'homme
thomas.preudho...@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.preudho...@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.preudho...@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?

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

2014-09-24 Thread Christophe Lyon 
Hi Thomas,


On 24 September 2014 10:01, Richard Biener richard.guent...@gmail.com wrote:
 On Tue, Sep 16, 2014 at 12:24 PM, Thomas Preud'homme
 thomas.preudho...@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.


Although I could notice the improvement:
Pass disappears   [PASS = ]:
  gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit bswap
implementation found at 3
New pass  [ = PASS]:
  gcc.dg/optimize-bswapsi-1.c scan-tree-dump-times bswap 32 bit bswap
implementation found at 4

for arm-*, armeb-* and aarch64-* targets, there is no change for
aarch64_be: is this expected?

Christophe.


 *** gcc/ChangeLog ***

 2014-09-15  Thomas Preud'homme  thomas.preudho...@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.preudho...@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?

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

2014-09-23 Thread Thomas Preud'homme 
Ping?

 -Original Message-
 From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-
 ow...@gcc.gnu.org] On Behalf Of Thomas Preud'homme
 Sent: Tuesday, September 16, 2014 6:25 PM
 To: gcc-patches@gcc.gnu.org
 Subject: [PATCH] Fix PR63266: Keep track of impact of sign extension in
 bswap
 
 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.
 
 *** gcc/ChangeLog ***
 
 2014-09-15  Thomas Preud'homme  thomas.preudho...@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.preudho...@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?