From patchwork Fri May 13 13:22:34 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kyrill Tkachov X-Patchwork-Id: 622029 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 3r5rBf0nBzz9t6g for ; Fri, 13 May 2016 23:22:48 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=OwZQNIgC; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; q=dns; s= default; b=hhndF/q2W2vd2+1rF9oTBkcqeUMtrQmypGI5Y+hoUQJmkA0Ycm+tS +on9jqvYLk9+AjJHq943Its0HJ9dx7bSct0VBRGUKT6/u5Rjd2Gwiy9PvZnsMClu nx0l6x7xbG1Avg+oT079HYGLd+DG71cCHkCy9IdhTd1EpPdOEsO8nw= 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 :message-id:date:from:mime-version:to:subject:references :in-reply-to:content-type:content-transfer-encoding; s=default; bh=hq6QhdhVZ9bhDn8MwhU4PrVGjTI=; b=OwZQNIgCMb878GGJeWyyYVtT3aiC GofuJmNacsX/tt/7HUwd1yHoKSoAUiPT07CPdE+RdKn5EvY/U4mKGgF5PmBvUdvG xEqcBr58MUdwIcmGe4J5XZiNzgZimsMkdY8wWppxV0ZvIpQh/VfwFbo/4rnwRRC5 vG5YwO1dIlI3cjg= Received: (qmail 7627 invoked by alias); 13 May 2016 13:22:41 -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 6739 invoked by uid 89); 13 May 2016 13:22:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 13 May 2016 13:22:38 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6A6B63A; Fri, 13 May 2016 06:22:50 -0700 (PDT) Received: from [10.2.206.43] (e100706-lin.cambridge.arm.com [10.2.206.43]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1EA103F21A; Fri, 13 May 2016 06:22:35 -0700 (PDT) Message-ID: <5735D51A.1050302@foss.arm.com> Date: Fri, 13 May 2016 14:22:34 +0100 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Bernd Schmidt , GCC Patches Subject: Re: [PATCH][combine] PR middle-end/71074 Check that const_op is >= 0 before potentially shifting in simplify_comparison References: <5735C6AF.8040002@foss.arm.com> <5735D043.708@redhat.com> In-Reply-To: <5735D043.708@redhat.com> On 13/05/16 14:01, Bernd Schmidt wrote: > On 05/13/2016 02:21 PM, Kyrill Tkachov wrote: >> Hi all, >> >> In this PR we may end up shifting a negative value left in >> simplify_comparison. >> The statement is: >> const_op <<= INTVAL (XEXP (op0, 1)); >> >> This patch guards the block of that statement by const_op >= 0. >> I _think_ that's a correct thing to do for that trasnformation: >> "If we have (compare (xshiftrt FOO N) (const_int C)) and >> the low order N bits of FOO are known to be zero, we can do this >> by comparing FOO with C shifted left N bits so long as no >> overflow occurs." > > Isn't the condition testing for overflow though? In a somewhat nonobvious way. > > So I think you should just use an unsigned version of const_op. While we're here we might as well make the code here a little more readable. How about the patch below? Can you test whether this works for you? > Looks reasonable to me barring some comments below. I'll test a version of the patch with the comments fixed. Thanks, Kyrill > > Bernd code = simplify_compare_const (code, mode, op0, &op1); - const_op = INTVAL (op1); + HOST_WIDE_INT const_op = INTVAL (op1); + unsigned HOST_WIDE_INT uconst_op = (unsigned HOST_WIDE_INT) const_op; Can this be just "unsigned HOST_WIDE_INT uconst_op = UINTVAL (op1);" ? ... + unsigned HOST_WIDE_INT low_mask + = (((unsigned HOST_WIDE_INT) 1 << INTVAL (amount)) - 1); unsigned HOST_WIDE_INT low_bits - = (nonzero_bits (XEXP (op0, 0), mode) - & (((unsigned HOST_WIDE_INT) 1 - << INTVAL (XEXP (op0, 1))) - 1)); + = (nonzero_bits (XEXP (op0, 0), mode) & low_mask); if (low_bits == 0 || !equality_comparison_p) { (unsigned HOST_WIDE_INT) 1 can be replaced with HOST_WIDE_INT_1U. Index: combine.c =================================================================== --- combine.c (revision 236113) +++ combine.c (working copy) @@ -11628,13 +11628,13 @@ simplify_comparison (enum rtx_code code, while (CONST_INT_P (op1)) { + HOST_WIDE_INT amount; This has to be an rtx rather than HOST_WIDE_INT from the way you use it later on. /* We only want to handle integral modes. This catches VOIDmode, CCmode, and the floating-point modes. An exception is that we @@ -11649,7 +11649,8 @@ simplify_comparison (enum rtx_code code, /* Try to simplify the compare to constant, possibly changing the comparison op, and/or changing op1 to zero. */