On Thu, 21 Jan 2016, Thomas Preud'homme wrote:
> On Friday, January 08, 2016 10:05:25 AM Richard Biener wrote:
> > On Tue, 5 Jan 2016, Thomas Preud'homme wrote:
> > > Hi,
> > >
> > > bswap optimization pass generate wrong code on big endian targets when the
> > > result of a bit operation it analyzed is a partial load of the range of
> > > memory accessed by the original expression (when one or more bytes at
> > > lowest address were lost in the computation). This is due to the way
> > > cmpxchg and cmpnop are adjusted in find_bswap_or_nop before being
> > > compared to the result of the symbolic expression. Part of the adjustment
> > > is endian independent: it's to ignore the bytes that were not accessed by
> > > the original gimple expression. However, when the result has less byte
> > > than that original expression, some more byte need to be ignored and this
> > > is endian dependent.
> > >
> > > The current code only support loss of bytes at the highest addresses
> > > because there is no code to adjust the address of the load. However, for
> > > little and big endian targets the bytes at highest address translate into
> > > different byte significance in the result. This patch first separate
> > > cmpxchg and cmpnop adjustement into 2 steps and then deal with endianness
> > > correctly for the second step.
> > >
> > > ChangeLog entries are as follow:
> > >
> > >
> > > *** gcc/ChangeLog ***
> > >
> > > 2015-12-16 Thomas Preud'homme <thomas.preudho...@arm.com>
> > >
> > > PR tree-optimization/67781
> > > * tree-ssa-math-opts.c (find_bswap_or_nop): Zero out bytes in
> > > cmpxchg
> > > and cmpnop in two steps: first the ones not accessed in original
> > > gimple expression in a endian independent way and then the ones
> > > not
> > > accessed in the final result in an endian-specific way.
> > >
> > > *** gcc/testsuite/ChangeLog ***
> > >
> > > 2015-12-16 Thomas Preud'homme <thomas.preudho...@arm.com>
> > >
> > > PR tree-optimization/67781
> > > * gcc.c-torture/execute/pr67781.c: New file.
> > >
> > > diff --git a/gcc/testsuite/gcc.c-torture/execute/pr67781.c
> > > b/gcc/testsuite/gcc.c-torture/execute/pr67781.c
> > > new file mode 100644
> > > index 0000000..bf50aa2
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.c-torture/execute/pr67781.c
> > > @@ -0,0 +1,34 @@
> > > +#ifdef __UINT32_TYPE__
> > > +typedef __UINT32_TYPE__ uint32_t;
> > > +#else
> > > +typedef unsigned uint32_t;
> > > +#endif
> > > +
> > > +#ifdef __UINT8_TYPE__
> > > +typedef __UINT8_TYPE__ uint8_t;
> > > +#else
> > > +typedef unsigned char uint8_t;
> > > +#endif
> > > +
> > > +struct
> > > +{
> > > + uint32_t a;
> > > + uint8_t b;
> > > +} s = { 0x123456, 0x78 };
> > > +
> > > +int pr67781()
> > > +{
> > > + uint32_t c = (s.a << 8) | s.b;
> > > + return c;
> > > +}
> > > +
> > > +int
> > > +main ()
> > > +{
> > > + if (sizeof (uint32_t) * __CHAR_BIT__ != 32)
> > > + return 0;
> > > +
> > > + if (pr67781 () != 0x12345678)
> > > + __builtin_abort ();
> > > + return 0;
> > > +}
> > > diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
> > > index b00f046..e5a185f 100644
> > > --- a/gcc/tree-ssa-math-opts.c
> > > +++ b/gcc/tree-ssa-math-opts.c
> > > @@ -2441,6 +2441,8 @@ find_bswap_or_nop_1 (gimple *stmt, struct
> > > symbolic_number *n, int limit)
> > >
> > > static gimple *
> > > find_bswap_or_nop (gimple *stmt, struct symbolic_number *n, bool *bswap)
> > > {
> > >
> > > + unsigned rsize;
> > > + uint64_t tmpn, mask;
> > >
> > > /* The number which the find_bswap_or_nop_1 result should match in order
> > >
> > > to have a full byte swap. The number is shifted to the right
> > > according to the size of the symbolic number before using it. */
> > >
> > > @@ -2464,24 +2466,38 @@ find_bswap_or_nop (gimple *stmt, struct
> > > symbolic_number *n, bool *bswap)
> > >
> > > /* Find real size of result (highest non-zero byte). */
> > > if (n->base_addr)
> > >
> > > - {
> > > - int rsize;
> > > - uint64_t tmpn;
> > > -
> > > - for (tmpn = n->n, rsize = 0; tmpn; tmpn >>= BITS_PER_MARKER,
> > > rsize++); - n->range = rsize;
> > > - }
> > > + for (tmpn = n->n, rsize = 0; tmpn; tmpn >>= BITS_PER_MARKER,
> > > rsize++);
> > > + else
> > > + rsize = n->range;
> > >
> > > - /* Zero out the extra bits of N and CMP*. */
> > > + /* Zero out the bits corresponding to untouched bytes in original
> > > gimple
> > > + expression. */
> > >
> > > if (n->range < (int) sizeof (int64_t))
> > >
> > > {
> > >
> > > - uint64_t mask;
> > > -
> > >
> > > mask = ((uint64_t) 1 << (n->range * BITS_PER_MARKER)) - 1;
> > > cmpxchg >>= (64 / BITS_PER_MARKER - n->range) * BITS_PER_MARKER;
> > > cmpnop &= mask;
> > >
> > > }
> > >
> > > + /* Zero out the bits corresponding to unused bytes in the result of the
> > > + gimple expression. */
> > > + if (rsize < n->range)
> > > + {
> > > + if (BYTES_BIG_ENDIAN)
> > > + {
> > > + mask = ((uint64_t) 1 << (rsize * BITS_PER_MARKER)) - 1;
> > > + cmpxchg &= mask;
> > > + cmpnop >>= (n->range - rsize) * BITS_PER_MARKER;
> > > + }
> > > + else
> > > + {
> > > + mask = ((uint64_t) 1 << (rsize * BITS_PER_MARKER)) - 1;
> > > + cmpxchg >>= (n->range - rsize) * BITS_PER_MARKER;
> > > + cmpnop &= mask;
> > > + }
> > > + n->range = rsize;
> > > + }
> > > +
> > >
> > > /* A complete byte swap should make the symbolic number to start with
> > >
> > > the largest digit in the highest order byte. Unchanged symbolic
> > > number indicates a read with same endianness as target architecture.
> > > */
> > >
> > > Regression testsuite was run on a bootstrapped native x86_64-linux-gnu GCC
> > > and on an arm-none-eabi GCC cross-compiler without any regression. I'm
> > > waiting for a slot on gcc110 to do a big endian bootstrap but at least
> > > the testcase works on mips-linux. I'll send an update once bootstrap is
> > > complete.
> > >
> > > Is this ok for trunk and 5 branch in a week time if no regression is
> > > reported?
> > Yes.
>
> What about a backport to GCC 5? The patch applies cleanly and shows no
> regression when running the testsuite with an arm-none-eabi GCC
> cross-compiler
> and a natively bootstrapped x86_64-linux-gnu GCC compiler.
>
> It also pass the test added in the patch with a nativly bootstrapped
> powerpc64-unknown-linux-gnu GCC compiler (on gcc110 in compile farm).
The "Yes" above approved a backport.
Richard.
