From patchwork Sat May 10 19:11:22 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 347719 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4029C140086 for ; Sun, 11 May 2014 05:11:37 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:date:message-id:mime-version:content-type; q=dns; s= default; b=KhWGPhntfXH+S3F0XZKGbWEWsgd7wYc0EuuQPJT6AqatnhPNQy2KC z0pzlYQ9/qpN94JjgsO0/Y3T5chEp5maJn5D9P6HCUMkaiAPqZOHTsk3TA396lvj EVgW0JsMNukoT+9iRzt14krZ2wprB4IDI/RJgC7WIuy+3fwET2lh7A= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:subject:date:message-id:mime-version:content-type; s= default; bh=ugvV2uMkvmzUezq0L16vifnjGhU=; b=KGTRkXWWPYHCt7qaBoa3 e3Ktg6kC2ga5SopYT9TVO4x8CPnpAm0dJuZTybvKO9M2kq/gXKse5uqsTO7ecNQo Ve2aWQMrN5llJZ+X0GUQAQnEcnuoB5jL6VRxj4/4H1Wp4zefNX63nv/n6SXUsYNe UdRqg4Ruc4CSprNliw+QH2k= Received: (qmail 3194 invoked by alias); 10 May 2014 19:11:29 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 3180 invoked by uid 89); 10 May 2014 19:11:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-we0-f169.google.com Received: from mail-we0-f169.google.com (HELO mail-we0-f169.google.com) (74.125.82.169) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sat, 10 May 2014 19:11:27 +0000 Received: by mail-we0-f169.google.com with SMTP id u56so5351131wes.0 for ; Sat, 10 May 2014 12:11:23 -0700 (PDT) X-Received: by 10.180.185.100 with SMTP id fb4mr8666130wic.11.1399749083920; Sat, 10 May 2014 12:11:23 -0700 (PDT) Received: from localhost ([2.26.169.52]) by mx.google.com with ESMTPSA id fw13sm6054205wic.6.2014.05.10.12.11.22 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 10 May 2014 12:11:23 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Subject: PR 61136: wide-int fallout in int_const_binop_1 Date: Sat, 10 May 2014 20:11:22 +0100 Message-ID: <87r4411mat.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 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. 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: 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 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. This patch therefore uses wi:: directly. I think this is worth doing independently of the eventual patches to fix the type incompatibilities elsewhere. 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? 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; +}