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).
Best regards,
Thomas
