Message ID | 000401cf3d17$76a460a0$63ed21e0$@arm.com |
---|---|
State | New |
Headers | show |
On Tue, Mar 11, 2014 at 06:48:37PM +0800, Thomas Preud'homme wrote: > I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right. 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; > + if (fake_swap32 (0x12345678) != 0x78567E12) > + __builtin_abort (); 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). 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? Jakub
On 03/11/14 03:48, Thomas Preud'homme wrote: > 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 Don't include the ChangeLog entry as part of your patch. It isn't likely to apply.
4.8 also has this bug. OK to backport? On Tue, Mar 11, 2014 at 6:59 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Mar 11, 2014 at 06:48:37PM +0800, Thomas Preud'homme wrote: >> I also added a typedef unsigned uint32_t for when sizeof(unsigned) == 4. I hope it's right. > > 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; > >> + if (fake_swap32 (0x12345678) != 0x78567E12) >> + __builtin_abort (); > > 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). > > 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? > > Jakub
On Wed, Mar 12, 2014 at 11:43:00AM +0800, Joey Ye wrote:
> 4.8 also has this bug. OK to backport?
Yes.
Jakub
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..83f2987 --- /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) != 4) + return 0; + if (fake_swap32 (0x12345678) != 0x78567E12) + __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))