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

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

[Thomas Preud'homme]

[-- Attachment #1: Type: text/plain, Size: 1513 bytes --]

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

[-- Attachment #2: pr63266.1.1.diff --]
[-- Type: application/octet-stream, Size: 5706 bytes --]

diff --git a/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c b/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
index ebfca60..580e6e0 100644
--- a/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
+++ b/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
@@ -49,5 +49,20 @@ swap32_c (SItype u)
 	  | (((u) & 0x000000ff) << 24));
 }
 
-/* { dg-final { scan-tree-dump-times "32 bit bswap implementation found at" 3 "bswap" } } */
+/* This variant comes from gcc.target/sh/pr53568-1.c.  It requires to track
+   which bytes have an unpredictable value (eg. due to sign extension) to
+   make sure that the final expression have only well defined byte values.  */
+
+SItype
+swap32_d (SItype in)
+{
+  /* 1x swap.w
+     2x swap.b  */
+  return (((in >> 0) & 0xFF) << 24)
+	 | (((in >> 8) & 0xFF) << 16)
+	 | (((in >> 16) & 0xFF) << 8)
+	 | (((in >> 24) & 0xFF) << 0);
+}
+
+/* { dg-final { scan-tree-dump-times "32 bit bswap implementation found at" 4 "bswap" } } */
 /* { dg-final { cleanup-tree-dump "bswap" } } */
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index c1c2c82..3c6e935 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1603,6 +1603,7 @@ make_pass_cse_sincos (gcc::context *ctxt)
    consisting of octet sized markers:
 
    0    - target byte has the value 0
+   FF   - target byte has an unknown value (eg. due to sign extension)
    1..size - marker value is the target byte index minus one.
 
    To detect permutations on memory sources (arrays and structures), a symbolic
@@ -1629,6 +1630,10 @@ struct symbolic_number {
 };
 
 #define BITS_PER_MARKER 8
+#define MARKER_MASK ((1 << BITS_PER_MARKER) - 1)
+#define MARKER_BYTE_UNKNOWN MARKER_MASK
+#define HEAD_MARKER(n, size) \
+  ((n) & ((uint64_t) MARKER_MASK << (((size) - 1) * BITS_PER_MARKER)))
 
 /* The number which the find_bswap_or_nop_1 result should match in
    order to have a nop.  The number is masked according to the size of
@@ -1651,7 +1656,8 @@ do_shift_rotate (enum tree_code code,
 		 struct symbolic_number *n,
 		 int count)
 {
-  int size = TYPE_PRECISION (n->type) / BITS_PER_UNIT;
+  int i, size = TYPE_PRECISION (n->type) / BITS_PER_UNIT;
+  unsigned head_marker;
 
   if (count % BITS_PER_UNIT != 0)
     return false;
@@ -1668,11 +1674,13 @@ do_shift_rotate (enum tree_code code,
       n->n <<= count;
       break;
     case RSHIFT_EXPR:
-      /* Arithmetic shift of signed type: result is dependent on the value.  */
-      if (!TYPE_UNSIGNED (n->type)
-	  && (n->n & ((uint64_t) 0xff << ((size - 1) * BITS_PER_MARKER))))
-	return false;
+      head_marker = HEAD_MARKER (n->n, size);
       n->n >>= count;
+      /* Arithmetic shift of signed type: result is dependent on the value.  */
+      if (!TYPE_UNSIGNED (n->type) && head_marker)
+	for (i = 0; i < count / BITS_PER_MARKER; i++)
+	  n->n |= (uint64_t) MARKER_BYTE_UNKNOWN
+		  << ((size - 1 - i) * BITS_PER_MARKER);
       break;
     case LROTATE_EXPR:
       n->n = (n->n << count) | (n->n >> ((size * BITS_PER_MARKER) - count));
@@ -1878,7 +1886,7 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 	      if ((val & tmp) != 0 && (val & tmp) != tmp)
 		return NULL;
 	      else if (val & tmp)
-		mask |= (uint64_t) 0xff << (i * BITS_PER_MARKER);
+		mask |= (uint64_t) MARKER_MASK << (i * BITS_PER_MARKER);
 
 	    n->n &= mask;
 	  }
@@ -1892,7 +1900,7 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 	  break;
 	CASE_CONVERT:
 	  {
-	    int type_size, old_type_size;
+	    int i, type_size, old_type_size;
 	    tree type;
 
 	    type = gimple_expr_type (stmt);
@@ -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)
 	      {
@@ -1968,7 +1975,7 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 	  if (gimple_assign_rhs1 (source_stmt1)
 	      != gimple_assign_rhs1 (source_stmt2))
 	    {
-	      int64_t inc, mask;
+	      int64_t inc;
 	      HOST_WIDE_INT off_sub;
 	      struct symbolic_number *n_ptr;
 
@@ -2002,15 +2009,15 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 		 bigger weight according to target endianness.  */
 	      inc = BYTES_BIG_ENDIAN ? off_sub + n2.range - n1.range : off_sub;
 	      size = TYPE_PRECISION (n1.type) / BITS_PER_UNIT;
-	      mask = 0xff;
 	      if (BYTES_BIG_ENDIAN)
 		n_ptr = &n1;
 	      else
 		n_ptr = &n2;
-	      for (i = 0; i < size; i++, inc <<= BITS_PER_MARKER,
-					 mask <<= BITS_PER_MARKER)
+	      for (i = 0; i < size; i++, inc <<= BITS_PER_MARKER)
 		{
-		  if (n_ptr->n & mask)
+		  unsigned marker =
+		    (n_ptr->n >> (i * BITS_PER_MARKER)) & MARKER_MASK;
+		  if (marker && marker != MARKER_BYTE_UNKNOWN)
 		    n_ptr->n += inc;
 		}
 	    }
@@ -2028,7 +2035,8 @@ find_bswap_or_nop_1 (gimple stmt, struct symbolic_number *n, int limit)
 	  n->bytepos = n1.bytepos;
 	  n->type = n1.type;
 	  size = TYPE_PRECISION (n->type) / BITS_PER_UNIT;
-	  for (i = 0, mask = 0xff; i < size; i++, mask <<= BITS_PER_MARKER)
+	  for (i = 0, mask = MARKER_MASK; i < size;
+	       i++, mask <<= BITS_PER_MARKER)
 	    {
 	      uint64_t masked1, masked2;
 

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

[Thomas Preud'homme]

Ping?

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@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.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?

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

[Richard Biener]

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?

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

[Christophe Lyon]

Hi Thomas,


On 24 September 2014 10:01, Richard Biener <richard.guenther@gmail.com> wrote:
> 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.
>

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

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

[Thomas Preud'homme]

> From: Christophe Lyon [mailto:christophe.lyon@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

[Christophe Lyon]

On 25 September 2014 08:39, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
>> From: Christophe Lyon [mailto:christophe.lyon@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

[Thomas Preud'homme]

> From: Christophe Lyon [mailto:christophe.lyon@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

[Christophe Lyon]

On 26 September 2014 04:25, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
>> From: Christophe Lyon [mailto:christophe.lyon@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

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

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

[Richard Biener]

On Tue, Oct 21, 2014 at 11:28 AM, Thomas Preud'homme
<thomas.preudhomme@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.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?
>
>
>
>

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

[Christophe Lyon]

Hi Thomas,

Some minor comments:

On 21 October 2014 11:28, Thomas Preud'homme <thomas.preudhomme@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.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)));
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.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?
>
>
>
>

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

[Thomas Preud'homme]

> From: Christophe Lyon [mailto:christophe.lyon@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

[Christophe Lyon]

On 22 October 2014 10:56, Thomas Preud'homme <thomas.preudhomme@arm.com> wrote:
>> From: Christophe Lyon [mailto:christophe.lyon@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

[Thomas Preud'homme]

> From: Christophe Lyon [mailto:christophe.lyon@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

[PATCH] Fix up sign extension in bswap

[Jakub Jelinek]

On Tue, Oct 21, 2014 at 10:28:40AM +0100, Thomas Preud'homme wrote:
> --- 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)
>  	      {

As my last bootstrap-ubsan bootstrap revealed, this is still wrong.
Here is a fix (other spots where MARKER_BYTE_UNKNOWN is shifted up
are correct).  Bootstrapped/regtested on i686-linux, ok for trunk?

Thomas, you know the code better, can you from the fix figure out
a testcase that current trunk miscompiles or doesn't optimize
because of this bug?

2014-10-28  Jakub Jelinek  <jakub@redhat.com>

	* tree-ssa-math-opts.c (find_bswap_or_nop_1): Use uint64_t
	type for the left shift in CASE_CONVERT case.

--- gcc/tree-ssa-math-opts.c.jj	2014-10-27 19:41:14.000000000 +0100
+++ gcc/tree-ssa-math-opts.c	2014-10-27 23:43:41.956495361 +0100
@@ -1926,7 +1926,7 @@ find_bswap_or_nop_1 (gimple stmt, struct
 	    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
+		n->n |= (uint64_t) MARKER_BYTE_UNKNOWN
 			<< ((type_size - 1 - i) * BITS_PER_MARKER);
 
 	    if (type_size < 64 / BITS_PER_MARKER)


	Jakub

Re: [PATCH] Fix up sign extension in bswap

[Richard Biener]

On Tue, 28 Oct 2014, Jakub Jelinek wrote:

> On Tue, Oct 21, 2014 at 10:28:40AM +0100, Thomas Preud'homme wrote:
> > --- 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)
> >  	      {
> 
> As my last bootstrap-ubsan bootstrap revealed, this is still wrong.
> Here is a fix (other spots where MARKER_BYTE_UNKNOWN is shifted up
> are correct).  Bootstrapped/regtested on i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> Thomas, you know the code better, can you from the fix figure out
> a testcase that current trunk miscompiles or doesn't optimize
> because of this bug?
> 
> 2014-10-28  Jakub Jelinek  <jakub@redhat.com>
> 
> 	* tree-ssa-math-opts.c (find_bswap_or_nop_1): Use uint64_t
> 	type for the left shift in CASE_CONVERT case.
> 
> --- gcc/tree-ssa-math-opts.c.jj	2014-10-27 19:41:14.000000000 +0100
> +++ gcc/tree-ssa-math-opts.c	2014-10-27 23:43:41.956495361 +0100
> @@ -1926,7 +1926,7 @@ find_bswap_or_nop_1 (gimple stmt, struct
>  	    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
> +		n->n |= (uint64_t) MARKER_BYTE_UNKNOWN
>  			<< ((type_size - 1 - i) * BITS_PER_MARKER);
>  
>  	    if (type_size < 64 / BITS_PER_MARKER)
> 
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend"orffer

RE: [PATCH] Fix up sign extension in bswap

[Thomas Preud'homme]

> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Tuesday, October 28, 2014 12:27 PM
> 
> Thomas, you know the code better, can you from the fix figure out
> a testcase that current trunk miscompiles or doesn't optimize
> because of this bug?

Here you are (see attachment).

Best regards,

Thomas

RE: [PATCH] Fix up sign extension in bswap

[Thomas Preud'homme]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

Bummer. Why didn't my MUA warned me on this one?

Here you are.

Best regards,

Thomas

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> Sent: Wednesday, October 29, 2014 9:33 AM
> To: 'Jakub Jelinek'; Richard Biener
> Cc: GCC Patches
> Subject: RE: [PATCH] Fix up sign extension in bswap
> 
> > From: Jakub Jelinek [mailto:jakub@redhat.com]
> > Sent: Tuesday, October 28, 2014 12:27 PM
> >
> > Thomas, you know the code better, can you from the fix figure out
> > a testcase that current trunk miscompiles or doesn't optimize
> > because of this bug?
> 
> Here you are (see attachment).
> 
> Best regards,
> 
> Thomas
> 
> 
> 
> 

[-- Attachment #2: marker_not_cast_testcase.1.0.diff --]
[-- Type: application/octet-stream, Size: 2104 bytes --]

diff --git a/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c b/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
index 580e6e0..cfde218 100644
--- a/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
+++ b/gcc/testsuite/gcc.dg/optimize-bswapsi-1.c
@@ -64,5 +64,19 @@ swap32_d (SItype in)
 	 | (((in >> 24) & 0xFF) << 0);
 }
 
-/* { dg-final { scan-tree-dump-times "32 bit bswap implementation found at" 4 "bswap" } } */
+/* This variant is adapted from swap32_d above.  It detects missing cast of
+   MARKER_BYTE_UNKNOWN to uint64_t for the CASE_CONVERT case for host
+   architecture where a left shift with too big an operand mask its high
+   bits.  */
+
+SItype
+swap32_e (SItype in)
+{
+  return (((in >> 0) & 0xFF) << 24)
+	 | (((in >> 8) & 0xFF) << 16)
+	 | (((((int64_t) in) & 0xFF0000FF0000) >> 16) << 8)
+	 | (((in >> 24) & 0xFF) << 0);
+}
+
+/* { dg-final { scan-tree-dump-times "32 bit bswap implementation found at" 5 "bswap" } } */
 /* { dg-final { cleanup-tree-dump "bswap" } } */
diff --git a/gcc/testsuite/gcc.dg/optimize-bswapsi-3.c b/gcc/testsuite/gcc.dg/optimize-bswapsi-3.c
new file mode 100644
index 0000000..79f2147
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/optimize-bswapsi-3.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target bswap32 } */
+/* { dg-require-effective-target stdint_types } */
+/* { dg-options "-O2 -fdump-tree-bswap" } */
+/* { dg-additional-options "-march=z900" { target s390-*-* } } */
+
+typedef int SItype __attribute__ ((mode (SI)));
+typedef int DItype __attribute__ ((mode (DI)));
+
+/* This variant comes from optimize-bswapsi-1.c swap32_d.  It detects a missing
+   cast of MARKER_BYTE_UNKNOWN to uint64_t for the CASE_CONVERT case for host
+   architecture where a left shift with too big an operand gives zero.  */
+
+SItype
+swap32 (SItype in)
+{
+  return (((in >> 0) & 0xFF) << 24)
+	 | (((in >> 8) & 0xFF) << 16)
+	 | (((((DItype) in) & 0xFF00FF0000llu) >> 16) << 8)
+	 | (((in >> 24) & 0xFF) << 0);
+}
+
+/* { dg-final { scan-tree-dump-not "32 bit bswap implementation found at" "bswap" } } */
+/* { dg-final { cleanup-tree-dump "bswap" } } */

Re: [PATCH] Fix up sign extension in bswap

[Jakub Jelinek]

On Wed, Oct 29, 2014 at 09:36:02AM -0000, Thomas Preud'homme wrote:
> Bummer. Why didn't my MUA warned me on this one?

I think this is ok for trunk with proper ChangeLog entry.

	Jakub

RE: [PATCH] Fix up sign extension in bswap

[Thomas Preud'homme]

> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Wednesday, October 29, 2014 9:41 AM
> 
> I think this is ok for trunk with proper ChangeLog entry.

Done with following ChangeLog entry:


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

	* gcc.dg/optimize-bswapsi-1.c (swap32_e): New bswap test.
	* gcc.dg/optimize-bswapsi-3.c: New test.

Best regards,

Thomas