On Sat, May 10, 2014 at 9:11 PM, Richard Sandiford
<rdsandif...@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;
> +}
