From patchwork Thu Aug 17 12:46:11 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marek Polacek X-Patchwork-Id: 802611 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-460505-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="khQzBd9d"; dkim-atps=neutral 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 3xY5Z0216Zz9t42 for ; Thu, 17 Aug 2017 22:46:31 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=G/XxSKWBKe9OHm86VDcKkFzbyxKMV6JYFKxrjRddOrOOwALj1Edkn xEpI1bt7DSVgA5IIUlbL7KVUM1Ujro4qATh45XwqtQL1hmAW6kP9MbZ11xo0lUDz 8liazQ78AsQ3Z2spBe88NBVV8glXE8nMrTI2VUrlW9zZjO1vT4rn6s= 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:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=MWPA62BfYKQoSnQjKUQIWuNPkqQ=; b=khQzBd9dCEgQTtX9MgrK Iy5dBUGGkzm1YI4f1wF9PcxtDjY0GoDYJXKOHdElh05+kkoklfync9fVN0XD2bZD /QrIok5z5aF88QK2Z2Sc+h/3JaL7VXGkQ8dUEJxpEkZUQdb/uQv3BwMi78BRe1zM JjxZ22vwT3ykPHXGguh0CME= Received: (qmail 21219 invoked by alias); 17 Aug 2017 12:46:22 -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 19016 invoked by uid 89); 17 Aug 2017 12:46:20 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=60000, uneasy X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 17 Aug 2017 12:46:19 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DE50F76B41; Thu, 17 Aug 2017 12:46:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com DE50F76B41 Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx01.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=polacek@redhat.com Received: from redhat.com (ovpn-204-189.brq.redhat.com [10.40.204.189]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 037EA88D7E; Thu, 17 Aug 2017 12:46:16 +0000 (UTC) Date: Thu, 17 Aug 2017 14:46:11 +0200 From: Marek Polacek To: GCC Patches , Richard Biener Subject: [PATCH] Fix ancient wrong-code with ?: (PR middle-end/81814) Message-ID: <20170817124611.GH17069@redhat.com> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.8.3 (2017-05-23) This PR is about wrong-code and has gone undetected for over 10 years (!). The issue is that e.g. the following (signed char) x == 0 ? (unsigned long long) x : 0 was wrongly folded to 0, because fold_cond_expr_with_comparison will fold A != 0 ? A : 0 to 0. But for x = 0x01000000 this is wrong: (signed char) is 0, but (unsigned long long) x is not. The culprit is operand_equal_for_comparison_p which contains shorten_compare-like code which says that the above is safe to fold. The code harks back to 1992 so I thought it worth to just get rid of it. But I did some measurements and it turns out that substituting operand_equal_p for operand_equal_for_comparison_p prevents folding ~60000 times in bootstrap. So I feel uneasy about removing the function completely. Instead, I propose to remove just the part that is causing trouble. (Maybe I should also delete the first call to operand_equal_p in operand_equal_for_comparison_p.) Bootstrapped/regtested on x86_64-linux, ok for trunk? What about 7? 2017-08-17 Marek Polacek PR middle-end/81814 * fold-const.c (operand_equal_for_comparison_p): Remove code that used to mimic what shorten_compare did. Change the return type to bool. (fold_cond_expr_with_comparison): Update call to operand_equal_for_comparison_p. (fold_ternary_loc): Likewise. * gcc.dg/torture/pr81814.c: New test. Marek diff --git gcc/fold-const.c gcc/fold-const.c index 0a5b168c320..fef9b1a707a 100644 --- gcc/fold-const.c +++ gcc/fold-const.c @@ -113,7 +113,6 @@ static tree negate_expr (tree); static tree associate_trees (location_t, tree, tree, enum tree_code, tree); static enum comparison_code comparison_to_compcode (enum tree_code); static enum tree_code compcode_to_comparison (enum comparison_code); -static int operand_equal_for_comparison_p (tree, tree, tree); static int twoval_comparison_p (tree, tree *, tree *, int *); static tree eval_subst (location_t, tree, tree, tree, tree, tree); static tree optimize_bit_field_compare (location_t, enum tree_code, @@ -3365,60 +3364,27 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags) #undef OP_SAME_WITH_NULL } -/* Similar to operand_equal_p, but see if ARG0 might have been made by - shorten_compare from ARG1 when ARG1 was being compared with OTHER. +/* Similar to operand_equal_p, but strip nops first. */ - When in doubt, return 0. */ - -static int -operand_equal_for_comparison_p (tree arg0, tree arg1, tree other) +static bool +operand_equal_for_comparison_p (tree arg0, tree arg1) { - int unsignedp1, unsignedpo; - tree primarg0, primarg1, primother; - unsigned int correct_width; - if (operand_equal_p (arg0, arg1, 0)) - return 1; + return true; if (! INTEGRAL_TYPE_P (TREE_TYPE (arg0)) || ! INTEGRAL_TYPE_P (TREE_TYPE (arg1))) - return 0; + return false; /* Discard any conversions that don't change the modes of ARG0 and ARG1 and see if the inner values are the same. This removes any signedness comparison, which doesn't matter here. */ - primarg0 = arg0, primarg1 = arg1; - STRIP_NOPS (primarg0); - STRIP_NOPS (primarg1); - if (operand_equal_p (primarg0, primarg1, 0)) - return 1; - - /* Duplicate what shorten_compare does to ARG1 and see if that gives the - actual comparison operand, ARG0. - - First throw away any conversions to wider types - already present in the operands. */ - - primarg1 = get_narrower (arg1, &unsignedp1); - primother = get_narrower (other, &unsignedpo); - - correct_width = TYPE_PRECISION (TREE_TYPE (arg1)); - if (unsignedp1 == unsignedpo - && TYPE_PRECISION (TREE_TYPE (primarg1)) < correct_width - && TYPE_PRECISION (TREE_TYPE (primother)) < correct_width) - { - tree type = TREE_TYPE (arg0); - - /* Make sure shorter operand is extended the right way - to match the longer operand. */ - primarg1 = fold_convert (signed_or_unsigned_type_for - (unsignedp1, TREE_TYPE (primarg1)), primarg1); - - if (operand_equal_p (arg0, fold_convert (type, primarg1), 0)) - return 1; - } + STRIP_NOPS (arg0); + STRIP_NOPS (arg1); + if (operand_equal_p (arg0, arg1, 0)) + return true; - return 0; + return false; } /* See if ARG is an expression that is either a comparison or is performing @@ -5300,7 +5266,7 @@ fold_cond_expr_with_comparison (location_t loc, tree type, expressions will be false, so all four give B. The min() and max() versions would give a NaN instead. */ if (!HONOR_SIGNED_ZEROS (element_mode (type)) - && operand_equal_for_comparison_p (arg01, arg2, arg00) + && operand_equal_for_comparison_p (arg01, arg2) /* Avoid these transformations if the COND_EXPR may be used as an lvalue in the C++ front-end. PR c++/19199. */ && (in_gimple_form @@ -11357,8 +11323,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type, Also try swapping the arguments and inverting the conditional. */ if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - arg1, TREE_OPERAND (arg0, 1)) + && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), arg1) && !HONOR_SIGNED_ZEROS (element_mode (arg1))) { tem = fold_cond_expr_with_comparison (loc, type, arg0, op1, op2); @@ -11367,9 +11332,7 @@ fold_ternary_loc (location_t loc, enum tree_code code, tree type, } if (COMPARISON_CLASS_P (arg0) - && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), - op2, - TREE_OPERAND (arg0, 1)) + && operand_equal_for_comparison_p (TREE_OPERAND (arg0, 0), op2) && !HONOR_SIGNED_ZEROS (element_mode (op2))) { location_t loc0 = expr_location_or (arg0, loc); diff --git gcc/testsuite/gcc.dg/torture/pr81814.c gcc/testsuite/gcc.dg/torture/pr81814.c index e69de29bb2d..aaf7c7f3041 100644 --- gcc/testsuite/gcc.dg/torture/pr81814.c +++ gcc/testsuite/gcc.dg/torture/pr81814.c @@ -0,0 +1,36 @@ +/* PR middle-end/81814 */ +/* { dg-do run } */ + +int +main () +{ + int i = 0x01000000; + int a; + + a = ((signed char) i) != 0 ? 0 : (unsigned long long int) i; + if (a != 0x01000000) + __builtin_abort (); + a = ((signed short int) i) != 0 ? 0 : (unsigned long long int) i; + if (a != 0x01000000) + __builtin_abort (); + a = ((unsigned short int) i) != 0 ? 0 : (unsigned long long int) i; + if (a != 0x01000000) + __builtin_abort (); + a = ((unsigned char) i) != 0 ? 0 : (unsigned long long int) i; + if (a != 0x01000000) + __builtin_abort (); + a = ((signed char) i) == 0 ? (unsigned long long int) i : 0; + if (a != 0x01000000) + __builtin_abort (); + a = ((signed short int) i) == 0 ? (unsigned long long int) i : 0; + if (a != 0x01000000) + __builtin_abort (); + a = ((unsigned short int) i) == 0 ? (unsigned long long int) i : 0; + if (a != 0x01000000) + __builtin_abort (); + a = ((unsigned char) i) == 0 ? (unsigned long long int) i : 0; + if (a != 0x01000000) + __builtin_abort (); + + return 0; +}