Fix PR61375: cancel bswap optimization when value doesn't fit in a HOST_WIDE_INT

> From: Richard Biener [mailto:richard.guenther@gmail.com]
> Sent: Tuesday, June 10, 2014 5:05 PM
> 
> Backports are welcome - please post a patch.
> 

Sorry for the delay. Here you are:


Ok for GCC 4.8 and GCC 4.9 branches?

Best regards,

Thomas

On Fri, Jun 20, 2014 at 12:41 PM, Thomas Preud'homme
<thomas.preudhomme@arm.com> wrote:
>> From: Richard Biener [mailto:richard.guenther@gmail.com]
>> Sent: Tuesday, June 10, 2014 5:05 PM
>>
>> Backports are welcome - please post a patch.
>>
>
> Sorry for the delay. Here you are:
>
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr61375.c b/gcc/testsuite/gcc.c-torture/execute/pr61375.c
> new file mode 100644
> index 0000000..d3b54a8
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr61375.c
> @@ -0,0 +1,35 @@
> +#ifdef __UINT64_TYPE__
> +typedef __UINT64_TYPE__ uint64_t;
> +#else
> +typedef unsigned long long uint64_t;
> +#endif
> +
> +#ifndef __SIZEOF_INT128__
> +#define __int128 long long
> +#endif
> +
> +/* Some version of bswap optimization would ICE when analyzing a mask constant
> +   too big for an HOST_WIDE_INT (PR61375).  */
> +
> +__attribute__ ((noinline, noclone)) uint64_t
> +uint128_central_bitsi_ior (unsigned __int128 in1, uint64_t in2)
> +{
> +  __int128 mask = (__int128)0xffff << 56;
> +  return ((in1 & mask) >> 56) | in2;
> +}
> +
> +int
> +main (int argc)
> +{
> +  __int128 in = 1;
> +#ifdef __SIZEOF_INT128__
> +  in <<= 64;
> +#endif
> +  if (sizeof (uint64_t) * __CHAR_BIT__ != 64)
> +    return 0;
> +  if (sizeof (unsigned __int128) * __CHAR_BIT__ != 128)
> +    return 0;
> +  if (uint128_central_bitsi_ior (in, 2) != 0x102)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
> index 9ff857c..9d64205 100644
> --- 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;
>           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)))
>               {
>
> Ok for GCC 4.8 and GCC 4.9 branches?

Ok.

Thanks,
Richard.

> Best regards,
>
> Thomas
>
>

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.

> >           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.

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

	Jakub

> 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

On Mon, Jun 23, 2014 at 04:50:49PM +0800, Thomas Preud'homme wrote:
> > 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.

Host could e.g. in theory have CHAR_BIT 32, while target BITS_PER_UNIT 8
(otherwise bswap pass would give up).  sizeof (unsigned HOST_WIDE_INT) could
very well be 2 in that case.

Anyway, another option is to also not run the bswap pass if CHAR_BIT != 8,
e.g. fold-const.c does something similar when deciding if VIEW_CONVERT_EXPR
of constants can be safely folded.

> > 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?

Just don't introduce new formatting issues and on lines you touch anyway
also fix formatting issues.

	Jakub

> From: Jakub Jelinek [mailto:jakub@redhat.com]
> Sent: Monday, June 23, 2014 4:59 PM
> 
> Host could e.g. in theory have CHAR_BIT 32, while target BITS_PER_UNIT 8
> (otherwise bswap pass would give up).  sizeof (unsigned HOST_WIDE_INT)
> could
> very well be 2 in that case.

In this case the pass would skip any value of more than 2 bytes. However although the original comments on struct symbolic_number implies that there is a mapping between host bytes (the bytes of the symbolic number) and target bytes, it isn't the case since do_shift_rotate () shift the symbolic number by quantity of BYTES_PER_UNIT instead of CHAR_BIT. Also there is quite a few 8 here and there. Although not a problem in practice, the mix of 8 and BITS_PER_UNIT does not look very good. I guess a quick review would be in order. Of course, with regards to the backport the mix of 8 and BITS_PER_UNIT should be left as is and only confusion about how to represent a target value into a host type should be fixed if any.

I'll come back to you whenever this is done.

Best regards,

Thomas

> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Thomas Preud'homme
> 
> However
> although the original comments on struct symbolic_number implies that
> there is a mapping between host bytes (the bytes of the symbolic number)
> and target bytes, it isn't the case since do_shift_rotate () shift the symbolic
> number by quantity of BYTES_PER_UNIT instead of CHAR_BIT.

My bad, the comment can be understood both ways.

Best regards,

Thomas