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

login
register
mail settings
Submitter Thomas Preud'homme
Date March 12, 2014, 3:45 a.m.
Message ID <000501cf3da5$75143330$5f3c9990$@arm.com>
Download mbox | patch
Permalink /patch/329281/
State New
Headers show

Comments

Thomas Preud'homme - March 12, 2014, 3:45 a.m.
> From: Jakub Jelinek [mailto:jakub@redhat.com]
> 
> In theory you could have __CHAR_BIT__ different from 8 and what you care
> about is that uint32_t has exactly 32 bits, so the check would need to be
>   if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
>     return 0;

I could go with:

In = (0x12 << (__CHAR_BIT__ * 3))
        | (0x34 << (__CHAR_BIT__ * 2))
        | (0x56 << __CHAR_BIT__)
        | 0x78;

and compare with a similarly constructed out so that I could run the test whenever sizeof (uint32_t) * __CHAR_BIT__ >= 32, isn't it?

> 
> Also, for int16 targets where __UINT32_TYPE__ is supposedly unsigned long,
> I think you would need to use:
> 
>   if (fake_swap32 (0x12345678UL) != 0x78567E12UL)
>     __builtin_abort ();
> 
> (the C standard guarantees that unsigned long is at least 32-bit and
> unsigned int at least 16-bit).

Right. Note to myself: ยง5.2.4.2.1 in C99. I guess so far I've only considered only some kind of architecture heterogeneity. Thanks for catching that.

> 
> Ok with those changes.
> 
> Do you have write access, or will somebody from your coworkers commit it
> for
> you?  Are you covered by ARM GCC Copyright assignment?

Yes and yes.

> 
> 	Jakub

Thanks for the review. See attachment and below to check the version you approved.


Best regards,

Thomas
Jakub Jelinek - March 12, 2014, 7:08 a.m.
On Wed, Mar 12, 2014 at 11:45:03AM +0800, Thomas Preud'homme wrote:
> > From: Jakub Jelinek [mailto:jakub@redhat.com]
> > 
> > In theory you could have __CHAR_BIT__ different from 8 and what you care
> > about is that uint32_t has exactly 32 bits, so the check would need to be
> >   if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
> >     return 0;
> 
> I could go with:
> 
> In = (0x12 << (__CHAR_BIT__ * 3))
>         | (0x34 << (__CHAR_BIT__ * 2))
>         | (0x56 << __CHAR_BIT__)
>         | 0x78;
> 
> and compare with a similarly constructed out so that I could run the test whenever sizeof (uint32_t) * __CHAR_BIT__ >= 32, isn't it?

No need to bother, I think we don't have any __CHAR_BIT__ != 8 targets right
now, and after all, the bswap code doesn't bother with it either:
static unsigned int
execute_optimize_bswap (void)
{
  basic_block bb;
  bool bswap16_p, bswap32_p, bswap64_p;
  bool changed = false;
  tree bswap16_type = NULL_TREE, bswap32_type = NULL_TREE, bswap64_type =
  NULL_TREE;

  if (BITS_PER_UNIT != 8)
    return 0;

> Thanks for the review. See attachment and below to check the version you approved.

Thanks, this is ok.

	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..04ce403 100644
--- a/gcc/testsuite/ChangeLog
+++ b/gcc/testsuite/ChangeLog
@@ -1,3 +1,8 @@ 
+2014-03-10  Thomas Preud'homme  <thomas.preudhomme@arm.com>
+
+	PR tree-optimization/60454
+	* gcc.c-torture/execute/pr60454.c: New test.
+
 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..ceec45e
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr60454.c
@@ -0,0 +1,31 @@ 
+#ifdef __UINT32_TYPE__
+typedef __UINT32_TYPE__ uint32_t;
+#else
+typedef unsigned 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.  */
+
+__attribute__ ((noinline, noclone)) uint32_t
+fake_swap32 (uint32_t in)
+{
+  return __fake_const_swab32 (in);
+}
+
+int main(void)
+{
+  if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
+    return 0;
+  if (fake_swap32 (0x12345678UL) != 0x78567E12UL)
+    __builtin_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))