RE: [PATCH] Fix PR61375: cancel bswap optimization when value doesn't fit in a HOST_WIDE_INT

["Thomas Preud'homme" <thomas dot preudhomme at arm dot com>]

> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Monday, June 23, 2014 4:37 PM
> 
> On Mon, Jun 23, 2014 at 10:18:16AM +0200, Richard Biener wrote:
> > > --- a/gcc/tree-ssa-math-opts.c
> > > +++ b/gcc/tree-ssa-math-opts.c
> > > @@ -1741,6 +1741,8 @@ find_bswap_1 (gimple stmt, struct
> symbolic_number *n, int limit)
> > >           if (n->size % BITS_PER_UNIT != 0)
> > >             return NULL_TREE;
> > >           n->size /= BITS_PER_UNIT;
> > > +         if (n->size > (int)sizeof (unsigned HOST_WIDEST_INT))
> > > +           return NULL_TREE;
> 
> This looks wrong, while the bswap pass is guarded with BITS_PER_UNIT == 8
> check (i.e. target), you don't know of HOST_BITS_PER_CHAR is 8.
> I'd move the test before the division by BITS_PER_UNIT, and compare
> against HOST_BITS_PER_WIDEST_INT.

I may misunderstand you but I don't think there is a problem here because we
just check if we can create a value on the host with as many bytes as the value
on the target. The value on the host is different, with each byte being a
number from 1 to SIZE, SIZE being the number of bytes on the target. So this
would fail only if the target value has so many bytes that this number of byte
cannot be represented in a HOST_WIDEST_INT.

> 
> > >           n->n = (sizeof (HOST_WIDEST_INT) < 8 ? 0 :
> > >                   (unsigned HOST_WIDEST_INT)0x08070605 << 32 | 0x04030201);
> > >
> > > @@ -1781,6 +1783,8 @@ find_bswap_1 (gimple stmt, struct
> symbolic_number *n, int limit)
> > >             type_size = TYPE_PRECISION (gimple_expr_type (stmt));
> > >             if (type_size % BITS_PER_UNIT != 0)
> > >               return NULL_TREE;
> > > +           if (type_size > (int)HOST_BITS_PER_WIDEST_INT)
> > > +             return NULL_TREE;
> > >
> > >             if (type_size / BITS_PER_UNIT < (int)(sizeof (HOST_WIDEST_INT)))
> > >               {
> 
> Similarly here.

I agree that here the test is not correct as we look at the number of bits on the
host which should be enough to count the number of byte on the target. To
reflect better the intent we should first compute the number of byte that
type_size forms and then compare to the size in byte of HOST_WIDEST_INT.

I'll rework the patch in this directly.

> 
> BTW, the formatting is wrong too, the (int) cast should be followed by space.

Right, but note that I merely followed the current style in this file. There are
many more occurences of this style mistake in this file. Do you want me to
fix this one anyway?

Best regards,

Thomas