From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18195 invoked by alias); 11 May 2014 12:23:49 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 18186 invoked by uid 89); 11 May 2014 12:23:47 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wi0-f176.google.com Received: from mail-wi0-f176.google.com (HELO mail-wi0-f176.google.com) (209.85.212.176) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 11 May 2014 12:23:45 +0000 Received: by mail-wi0-f176.google.com with SMTP id n15so3245079wiw.3 for ; Sun, 11 May 2014 05:23:42 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.105.72 with SMTP id gk8mr11262885wib.32.1399811022183; Sun, 11 May 2014 05:23:42 -0700 (PDT) Received: by 10.194.119.193 with HTTP; Sun, 11 May 2014 05:23:42 -0700 (PDT) In-Reply-To: <87r4411mat.fsf@talisman.default> References: <87r4411mat.fsf@talisman.default> Date: Sun, 11 May 2014 12:23:00 -0000 Message-ID: Subject: Re: PR 61136: wide-int fallout in int_const_binop_1 From: Richard Biener To: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Content-Type: text/plain; charset=UTF-8 X-IsSubscribed: yes X-SW-Source: 2014-05/txt/msg00707.txt.bz2 On Sat, May 10, 2014 at 9:11 PM, Richard Sandiford 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: > > type size > unit size > align 64 symtab 0 alias set -1 canonical type 0x7ffff11ec888 precision 64 min max > > > arg 0 constant 4> > arg 1 > readonly > arg 0 size > unit size > align 32 symtab 0 alias set -1 canonical type 0x7ffff11ec690 precision 32 min max > pointer_to_this > > ../../../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 > + bool multiple_of_p (const T1 &, const T2 &, signop); > + > + template > 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 > +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 > 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; > +}