Re: PR 61136: wide-int fallout in int_const_binop_1

[Richard Biener <richard dot guenther at gmail dot com>]

On Sat, May 10, 2014 at 9:11 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> The wide-int version of int_const_binop_1 has:
>
> static tree
> int_const_binop_1 (enum tree_code code, const_tree arg1, const_tree parg2,
>                    int overflowable)
> {
>   [...]
>   tree type = TREE_TYPE (arg1);
>   [...]
>   wide_int arg2 = wide_int::from (parg2, TYPE_PRECISION (type),
>                                   TYPE_SIGN (TREE_TYPE (parg2)));
>
> The idea that the result should be the same type as arg1 predates
> wide-int but the conversion of arg2 to that type is new.
>
> The new conversion can't be right because it loses overflow information.
> The question then is whether this function should be prepared to handle
> mismatched arguments (as the pre-wide-int version effectively did, since
> it did everything in double_int precision) or whether the operation
> should be strictly typed.

Well.  Certainly they should be compatible enough - but trying to enforce
strict compatibility had too much fallout, also cost-wise.  There are too
many callers doing

int_const_binop (PLUS_EXPR, x, integer_one_cst)

(that there isn't a variant with a (unsigned) HWI 2nd arg was always odd).

So it is at least on purpose that the result is of type arg1 (also for
shifts, obviously).

>  If the former then the function should be
> doing the calculation in widest_int, which would make it a much more
> direct analogue of the original code.  If the latter then we should be
> able to operate directly on arg2.
>
> I assume it really is supposed to be strictly typed though.  It seems
> strange to use the type of the first operand as the type of the result
> otherwise.  E.g. in one of the cases I've found where it isn't strictly
> typed we create:
>
>  <mult_expr 0x7ffff1310758
>     type <integer_type 0x7ffff11ec888 long unsigned int public unsigned DI
>         size <integer_cst 0x7ffff11daf78 constant 64>
>         unit size <integer_cst 0x7ffff11daf90 constant 8>
>         align 64 symtab 0 alias set -1 canonical type 0x7ffff11ec888 precision 64 min <integer_cst 0x7ffff11f9258 0> max <integer_cst 0x7ffff11f0100 18446744073709551615>>
>
>     arg 0 <integer_cst 0x7ffff11f9720 type <integer_type 0x7ffff11ec690 int> constant 4>
>     arg 1 <sizeof_expr 0x7ffff130d280 type <integer_type 0x7ffff11ec888 long unsigned int>
>         readonly
>         arg 0 <integer_type 0x7ffff11ec690 int public type_6 SI
>             size <integer_cst 0x7ffff11f91c8 constant 32>
>             unit size <integer_cst 0x7ffff11f91e0 constant 4>
>             align 32 symtab 0 alias set -1 canonical type 0x7ffff11ec690 precision 32 min <integer_cst 0x7ffff11f9180 -2147483648> max <integer_cst 0x7ffff11f9198 2147483647>
>             pointer_to_this <pointer_type 0x7ffff1200738>>
>         ../../../gcc/gcc/testsuite/g++.dg/gomp/declare-simd-1.C:16:62>
>     ../../../gcc/gcc/testsuite/g++.dg/gomp/declare-simd-1.C:16:53>
>
> and these arguments later find their way to int_const_binop_1.
> The result and second argument are long (64 bits) but the first
> argument is int (32 bits).  So a simple conversion to widest_int
> would mean that the result would be 32 bits rather than 64 bits,
> which doesn't look intentional.
>
> I tried making int_const_binop_1 strictly typed and the only thing
> that was needed to pass bootstrap was a fairly obvious-looking patch
> to the Ada frontend.  But there's quite a bit of fallout in the testsuite
> itself (including the above).  I'm still working my way through that,
> so please shout if I'm going doing a rathole.
>
> This causes a problem in the testcase because of this code in
> multiple_of_p:
>
>     case INTEGER_CST:
>       if (TREE_CODE (bottom) != INTEGER_CST
>           || integer_zerop (bottom)
>           || (TYPE_UNSIGNED (type)
>               && (tree_int_cst_sgn (top) < 0
>                   || tree_int_cst_sgn (bottom) < 0)))
>         return 0;
>       return integer_zerop (int_const_binop (TRUNC_MOD_EXPR,
>                                              top, bottom));
>
> Here TOP and BOTTOM aren't necessarily the same type.  In the testcase
> for PR61136 BOTTOM is wider than TOP and when converted (truncated)
> to TOP's type becomes zero.  This then falls foul of the null escape in:
>
>     case TRUNC_MOD_EXPR:
>       if (arg2 == 0)
>         return NULL_TREE;
>       res = wi::mod_trunc (arg1, arg2, sign, &overflow);
>       break;
>
> So we end up calling integer_zerop on a null tree.
>
> We know at the point of the int_const_binop call that we're dealing
> with two INTEGER_CSTs, so it seems silly to create a tree for the result
> only to test whether it's zero.

Right.  The idea was always to avoid creating trees whenever possible.
Formerly you'd use double-ints carefully (because of their 'infinite'
precision).

> This patch therefore uses wi:: directly.
> I think this is worth doing independently of the eventual patches to fix
> the type incompatibilities elsewhere.

Yes.

> The test is "wi::smod_trunc (...) == 0" but it seemed more mnemonic to
> have a version of wi::multiple_of_p that doesn't compute the quotient.
>
> Tested on x86_64-linux-gnu.  OK to install?

Ok.

I still think the int_const_binop change needs investigation (Or it should
go entirely and replaced by wi:: uses).

Thanks,
Richard.

> Thanks,
> Richard
>
>
> gcc/
>         PR tree-optimization/61136
>         * wide-int.h (multiple_of_p): Define a version that doesn't return
>         the quotient.
>         * fold-const.c (extract_muldiv_1): Use wi::multiple_of_p instead of an
>         integer_zerop/const_binop pair.
>         (multiple_of_p): Likewise, converting both operands to widest_int
>         precision.
>
> gcc/testsuite/
>         * gcc.dg/torture/pr61136.c: New test.
>
> Index: gcc/wide-int.h
> ===================================================================
> --- gcc/wide-int.h      2014-05-10 19:45:34.743137375 +0100
> +++ gcc/wide-int.h      2014-05-10 20:03:17.275435486 +0100
> @@ -530,6 +530,9 @@ #define SHIFT_FUNCTION \
>    BINARY_FUNCTION mod_round (const T1 &, const T2 &, signop, bool * = 0);
>
>    template <typename T1, typename T2>
> +  bool multiple_of_p (const T1 &, const T2 &, signop);
> +
> +  template <typename T1, typename T2>
>    bool multiple_of_p (const T1 &, const T2 &, signop,
>                       WI_BINARY_RESULT (T1, T2) *);
>
> @@ -2791,6 +2794,15 @@ wi::mod_round (const T1 &x, const T2 &y,
>    return remainder;
>  }
>
> +/* Return true if X is a multiple of Y.  Treat X and Y as having the
> +   signedness given by SGN.  */
> +template <typename T1, typename T2>
> +inline bool
> +wi::multiple_of_p (const T1 &x, const T2 &y, signop sgn)
> +{
> +  return wi::mod_trunc (x, y, sgn) == 0;
> +}
> +
>  /* Return true if X is a multiple of Y, storing X / Y in *RES if so.
>     Treat X and Y as having the signedness given by SGN.  */
>  template <typename T1, typename T2>
> Index: gcc/fold-const.c
> ===================================================================
> --- gcc/fold-const.c    2014-05-10 19:45:34.743137375 +0100
> +++ gcc/fold-const.c    2014-05-10 20:04:55.661262761 +0100
> @@ -5708,7 +5708,7 @@ extract_muldiv_1 (tree t, tree c, enum t
>        /* For a constant, we can always simplify if we are a multiply
>          or (for divide and modulus) if it is a multiple of our constant.  */
>        if (code == MULT_EXPR
> -         || integer_zerop (const_binop (TRUNC_MOD_EXPR, t, c)))
> +         || wi::multiple_of_p (t, c, TYPE_SIGN (type)))
>         return const_binop (code, fold_convert (ctype, t),
>                             fold_convert (ctype, c));
>        break;
> @@ -5888,7 +5888,7 @@ extract_muldiv_1 (tree t, tree c, enum t
>        /* If it's a multiply or a division/modulus operation of a multiple
>           of our constant, do the operation and verify it doesn't overflow.  */
>        if (code == MULT_EXPR
> -         || integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
> +         || wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
>         {
>           op1 = const_binop (code, fold_convert (ctype, op1),
>                              fold_convert (ctype, c));
> @@ -5932,7 +5932,7 @@ extract_muldiv_1 (tree t, tree c, enum t
>           /* If the multiplication can overflow we cannot optimize this.  */
>           && TYPE_OVERFLOW_UNDEFINED (TREE_TYPE (t))
>           && TREE_CODE (TREE_OPERAND (t, 1)) == INTEGER_CST
> -         && integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
> +         && wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
>         {
>           *strict_overflow_p = true;
>           return omit_one_operand (type, integer_zero_node, op0);
> @@ -5989,7 +5989,7 @@ extract_muldiv_1 (tree t, tree c, enum t
>                   && code != FLOOR_MOD_EXPR && code != ROUND_MOD_EXPR
>                   && code != MULT_EXPR)))
>         {
> -         if (integer_zerop (const_binop (TRUNC_MOD_EXPR, op1, c)))
> +         if (wi::multiple_of_p (op1, c, TYPE_SIGN (type)))
>             {
>               if (TYPE_OVERFLOW_UNDEFINED (ctype))
>                 *strict_overflow_p = true;
> @@ -5998,7 +5998,7 @@ extract_muldiv_1 (tree t, tree c, enum t
>                                                 const_binop (TRUNC_DIV_EXPR,
>                                                              op1, c)));
>             }
> -         else if (integer_zerop (const_binop (TRUNC_MOD_EXPR, c, op1)))
> +         else if (wi::multiple_of_p (c, op1, TYPE_SIGN (type)))
>             {
>               if (TYPE_OVERFLOW_UNDEFINED (ctype))
>                 *strict_overflow_p = true;
> @@ -15314,8 +15314,8 @@ multiple_of_p (tree type, const_tree top
>               && (tree_int_cst_sgn (top) < 0
>                   || tree_int_cst_sgn (bottom) < 0)))
>         return 0;
> -      return integer_zerop (int_const_binop (TRUNC_MOD_EXPR,
> -                                            top, bottom));
> +      return wi::multiple_of_p (wi::to_widest (top), wi::to_widest (bottom),
> +                               SIGNED);
>
>      default:
>        return 0;
> Index: gcc/testsuite/gcc.dg/torture/pr61136.c
> ===================================================================
> --- /dev/null   2014-05-03 11:58:38.033951363 +0100
> +++ gcc/testsuite/gcc.dg/torture/pr61136.c      2014-05-10 20:03:17.274435478 +0100
> @@ -0,0 +1,5 @@
> +unsigned long long
> +foo (int a)
> +{
> +  return a * 7 & 1ULL << 63;
> +}