Patchwork Fix incorrect byte swap detection (PR tree-optimization/60454)

login
register
mail settings
Submitter Thomas Preud'homme
Date March 11, 2014, 6:53 a.m.
Message ID <000001cf3cf6$a3ef8b00$ebcea100$@arm.com>
Download mbox | patch
Permalink /patch/328934/
State New
Headers show

Comments

Thomas Preud'homme - March 11, 2014, 6:53 a.m.
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> >  	  n->size = n1.size;
> > +	  for (i = 0, mask = 0xff; i < n->size; i++, mask <<= 8)
> 
> This should be mask <<= BITS_PER_UNIT for consistency.

Indeed.

> 
> And, as has been said earlier, the testcase should be a runtime testcase
> (in gcc.c-torture/execute/), probably with
> __attribute__((noinline, noclone)) on the function, where main calls the
> function with a couple of different values, verifies the result and aborts
> if it is incorrect.

Done as well. This should also solve the problem of aarch64 (and potentially others) missing as the test will be executed by all platform.

> 
> 	Jakub

Since it seems Exchange is unable to keep formatting of patches (or I am unable to configure it correctly), I put the new patch in attachment in addition to reproducing it below.
Jakub Jelinek - March 11, 2014, 9:54 a.m.
On Tue, Mar 11, 2014 at 02:53:39PM +0800, Thomas Preud'homme wrote:
> +2014-03-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> +
> +	PR tree-optimization/60454
> +	* gcc.c-torture/execute/pr60454.c (fake_swap32): Testcase to track
> +	regression of PR60454.

The ChangeLog entry is wrong, the file is new, so you should just say:
	* gcc.c-torture/execute/pr60454.c: New test.

But more importantly:

> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
> @@ -0,0 +1,25 @@
> +#include <stdint.h>

I'm not sure if all targets even provide stdint.h header, and if all targets
have uint32_t type.  For most targets gcc now provides stdint.h (or uses
glibc stdint.h).
So perhaps instead of including stdint.h just do:
#ifndef __UINT32_TYPE__
int
main ()
{
  return 0;
}
#else
typedef __UINT32_TYPE__ uint32_t;

...
#endif
?
> +
> +#define __fake_const_swab32(x) ((uint32_t)(			      \
> +        (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) |            \
> +        (((uint32_t)(x) & (uint32_t)0x0000ff00UL) <<  8) |            \
> +        (((uint32_t)(x) & (uint32_t)0x000000ffUL) <<  8) |            \
> +        (((uint32_t)(x) & (uint32_t)0x0000ff00UL)      ) |            \
> +        (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24)))
> +
> +/* Previous version of bswap optimization would detect byte swap when none
> +   happen. This test aims at catching such wrong detection to avoid
> +   regressions.  */
> +
> +uint32_t
> +fake_swap32 (uint32_t in) __attribute__ ((noinline, noclone))
> +{

This must not have been tested, this is a syntax error.
It should be
__attribute__ (noinline, noclone)) uint32_t
fake_swap32 (uint32_t in)
{
...

(the attribute can be after the arguments only for prototypes).

> +  return __fake_const_swab32 (in);
> +}
> +
> +int main(void)
> +{
> +  if (fake_swap32 (0x12345678) != 0x78567E12)
> +    abort ();

Use __builtin_abort (); here instead?  You aren't including stdlib.h
for abort.

After fixing up the testcase, you don't need to bootstrap/regtest it again,
make check-gcc RUNTESTFLAGS=execute.exp=pr60454.c
should be enough.

> --- a/gcc/tree-ssa-math-opts.c
> +++ b/gcc/tree-ssa-math-opts.c
> @@ -1801,7 +1801,9 @@ find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
>  
>    if (rhs_class == GIMPLE_BINARY_RHS)
>      {
> +      int i;
>        struct symbolic_number n1, n2;
> +      unsigned HOST_WIDEST_INT mask;
>        tree source_expr2;
>  
>        if (code != BIT_IOR_EXPR)
> @@ -1827,6 +1829,15 @@ find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
>  	    return NULL_TREE;
>  
>  	  n->size = n1.size;
> +	  for (i = 0, mask = 0xff; i < n->size; i++, mask <<= BITS_PER_UNIT)
> +	    {
> +	      unsigned HOST_WIDEST_INT masked1, masked2;
> +
> +	      masked1 = n1.n & mask;
> +	      masked2 = n2.n & mask;
> +	      if (masked1 && masked2 && masked1 != masked2)
> +		return NULL_TREE;
> +	    }
>  	  n->n = n1.n | n2.n;
>  
>  	  if (!verify_symbolic_number_p (n, stmt))

The rest looks good to me.

	Jakub

Patch

diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 748805e..b6d7d93 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-03-07  Thomas Preud'homme  <thomas.preudhomme@arm.com>
+
+	PR tree-optimization/60454
+	* tree-ssa-math-opts.c (find_bswap_1): Fix bswap detection.
+
 2014-02-23  David Holsgrove <david.holsgrove@xilinx.com>
 
 	* config/microblaze/microblaze.md: Correct ashrsi_reg / lshrsi_reg names
diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog
index f3c0c85..b95b2b3 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2014-03-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
+
+	PR tree-optimization/60454
+	* gcc.c-torture/execute/pr60454.c (fake_swap32): Testcase to track
+	regression of PR60454.
+
 2014-02-23  David Holsgrove <david.holsgrove@xilinx.com>
 
 	* gcc/testsuite/gcc.target/microblaze/others/mem_reload.c: New test.
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr60454.c b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
new file mode 100644
index 0000000..e5fbd8f
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
@@ -0,0 +1,25 @@ 
+#include <stdint.h>
+
+#define __fake_const_swab32(x) ((uint32_t)(			      \
+        (((uint32_t)(x) & (uint32_t)0x000000ffUL) << 24) |            \
+        (((uint32_t)(x) & (uint32_t)0x0000ff00UL) <<  8) |            \
+        (((uint32_t)(x) & (uint32_t)0x000000ffUL) <<  8) |            \
+        (((uint32_t)(x) & (uint32_t)0x0000ff00UL)      ) |            \
+        (((uint32_t)(x) & (uint32_t)0xff000000UL) >> 24)))
+
+/* Previous version of bswap optimization would detect byte swap when none
+   happen. This test aims at catching such wrong detection to avoid
+   regressions.  */
+
+uint32_t
+fake_swap32 (uint32_t in) __attribute__ ((noinline, noclone))
+{
+  return __fake_const_swab32 (in);
+}
+
+int main(void)
+{
+  if (fake_swap32 (0x12345678) != 0x78567E12)
+    abort ();
+  return 0;
+}
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 8e372ed..9ff857c 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1801,7 +1801,9 @@  find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
 
   if (rhs_class == GIMPLE_BINARY_RHS)
     {
+      int i;
       struct symbolic_number n1, n2;
+      unsigned HOST_WIDEST_INT mask;
       tree source_expr2;
 
       if (code != BIT_IOR_EXPR)
@@ -1827,6 +1829,15 @@  find_bswap_1 (gimple stmt, struct symbolic_number *n, int limit)
 	    return NULL_TREE;
 
 	  n->size = n1.size;
+	  for (i = 0, mask = 0xff; i < n->size; i++, mask <<= BITS_PER_UNIT)
+	    {
+	      unsigned HOST_WIDEST_INT masked1, masked2;
+
+	      masked1 = n1.n & mask;
+	      masked2 = n2.n & mask;
+	      if (masked1 && masked2 && masked1 != masked2)
+		return NULL_TREE;
+	    }
 	  n->n = n1.n | n2.n;
 
 	  if (!verify_symbolic_number_p (n, stmt))