Message ID | 014f01d7a93c$4dda4100$e98ec300$@nextmovesoftware.com |
---|---|
State | New |
Headers |
Return-Path: <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 9349A3858415 for <patchwork@sourceware.org>; Tue, 14 Sep 2021 07:44:28 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from server.nextmovesoftware.com (server.nextmovesoftware.com [162.254.253.69]) by sourceware.org (Postfix) with ESMTPS id 7431F3858402 for <gcc-patches@gcc.gnu.org>; Tue, 14 Sep 2021 07:44:11 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7431F3858402 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=nextmovesoftware.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=nextmovesoftware.com DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=nextmovesoftware.com; s=default; h=Content-Type:MIME-Version:Message-ID: Date:Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:In-Reply-To:References:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=ur+wFPrj0ba86YazdKvxRzTaOCdUfh6vXbc7wynt76k=; b=aH8j+r0Xb54vU4lgAunazd/8Nt QVFQXS9kZd0/cggYVzK63ntsZeazpZgCguHjGFL7qiGVsImHl01UE6pI6ojR5KC8UJcU9PmiJdMZ5 ZKO663nTFdvnJsztC0AvSqHjBmfEjVNHCH8Tq7mWzje6uQz8v39kTw+N2EUkl06xP9Ic8WJXg/qeR D4VeJfWGx3dtJWBI7qRTRk/MeBqkm2idbsdUS64RVOnR1LGF2Xueo5kkrNR3mv1hNsK9qgGCg2vnM 31Pr8/AzynJqJFWgAJjvIM59tuWptlZhlkgS8VWPxumYOwL16f5SPgW0cTeVzDXGkbzR2TKCN+Ugg iu8PzG5g==; Received: from host86-168-251-41.range86-168.btcentralplus.com ([86.168.251.41]:53142 helo=Dell) by server.nextmovesoftware.com with esmtpsa (TLS1.2) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from <roger@nextmovesoftware.com>) id 1mQ36w-00037C-Px; Tue, 14 Sep 2021 03:44:11 -0400 From: "Roger Sayle" <roger@nextmovesoftware.com> To: "'GCC Patches'" <gcc-patches@gcc.gnu.org> Subject: [PATCH #2] PR c/102245: Disable sign-changing optimization for shifts by zero. Date: Tue, 14 Sep 2021 08:44:08 +0100 Message-ID: <014f01d7a93c$4dda4100$e98ec300$@nextmovesoftware.com> MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----=_NextPart_000_0150_01D7A944.AFA02FA0" X-Mailer: Microsoft Outlook 16.0 Thread-Index: AdepO2ZG/NZhjIOAT92J4fYS9weITA== Content-Language: en-gb X-AntiAbuse: This header was added to track abuse, please include it with any abuse report X-AntiAbuse: Primary Hostname - server.nextmovesoftware.com X-AntiAbuse: Original Domain - gcc.gnu.org X-AntiAbuse: Originator/Caller UID/GID - [47 12] / [47 12] X-AntiAbuse: Sender Address Domain - nextmovesoftware.com X-Get-Message-Sender-Via: server.nextmovesoftware.com: authenticated_id: roger@nextmovesoftware.com X-Authenticated-Sender: server.nextmovesoftware.com: roger@nextmovesoftware.com X-Source: X-Source-Args: X-Source-Dir: X-Spam-Status: No, score=-10.7 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_BARRACUDACENTRAL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list <gcc-patches.gcc.gnu.org> List-Unsubscribe: <https://gcc.gnu.org/mailman/options/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=unsubscribe> List-Archive: <https://gcc.gnu.org/pipermail/gcc-patches/> List-Post: <mailto:gcc-patches@gcc.gnu.org> List-Help: <mailto:gcc-patches-request@gcc.gnu.org?subject=help> List-Subscribe: <https://gcc.gnu.org/mailman/listinfo/gcc-patches>, <mailto:gcc-patches-request@gcc.gnu.org?subject=subscribe> Cc: 'Jakub Jelinek' <jakub@redhat.com> Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" <gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org> |
Series |
[#2] PR c/102245: Disable sign-changing optimization for shifts by zero.
|
|
Commit Message
Roger Sayle
Sept. 14, 2021, 7:44 a.m. UTC
Respecting Jakub's suggestion that it may be better to warn-on-valid for "if (x << 0)" as the author might have intended "if (x < 0)" [which will also warn when x is _Bool], the simplest way to resolve this regression is to disable the recently added fold transformation for shifts by zero; these will be optimized later (elsewhere). Guarding against integer_zerop is the simplest of three alternatives; the second being to only apply this transformation to GIMPLE and not GENERIC, and the third (potentially) being to explicitly handle shifts by zero here, with an (if cond then else), optimizing the expression to a convert, but awkwardly duplicating the more general transformation earlier in match.pd's shift simplifications. This patch has been tested (against a recent snapshot without build issues) on x86_64-pc-linux-gnu with "make bootstrap" and "make -k check" with no new failures. Note that test1 in the new testcase is changed from dg-bogus to dg-warning compared with version #1. Ok for mainline? 2021-09-14 Roger Sayle <roger@nextmovesoftware.com> gcc/ChangeLog PR c/102245 * match.pd (shift optimizations): Disable recent sign-changing optimization for shifts by zero, these will be folded later. gcc/testsuite/ChangeLog PR c/102245 * gcc.dg/Wint-in-bool-context-4.c: New test case. Roger -----Original Message----- From: Jakub Jelinek <jakub@redhat.com> Sent: 13 September 2021 11:58 To: Roger Sayle <roger@nextmovesoftware.com> Cc: 'GCC Patches' <gcc-patches@gcc.gnu.org> Subject: Re: [PATCH] PR c/102245: Don't warn that ((_Bool)x<<0) isn't a truthvalue. On Mon, Sep 13, 2021 at 11:42:08AM +0100, Roger Sayle wrote: > gcc/c-family/ChangeLog > PR c/102245 > * c-common.c (c_common_truthvalue_conversion) [LSHIFT_EXPR]: > Special case (optimize) shifts by zero. > > gcc/testsuite/ChangeLog > PR c/102245 > * gcc.dg/Wint-in-bool-context-4.c: New test case. > > Roger > -- > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index > 017e415..44b5fcc 100644 > --- a/gcc/c-family/c-common.c > +++ b/gcc/c-family/c-common.c > @@ -3541,6 +3541,10 @@ c_common_truthvalue_conversion (location_t location, tree expr) > break; > > case LSHIFT_EXPR: > + /* Treat shifts by zero as a special case. */ > + if (integer_zerop (TREE_OPERAND (expr, 1))) > + return c_common_truthvalue_conversion (location, > + TREE_OPERAND (expr, 0)); > /* We will only warn on signed shifts here, because the majority of > false positive warnings happen in code where unsigned arithmetic > was used in anticipation of a possible overflow. > /* PR c/102245 */ > /* { dg-options "-Wint-in-bool-context" } */ > /* { dg-do compile } */ > > _Bool test1(_Bool x) > { > return !(x << 0); /* { dg-bogus "boolean context" } */ } While this exact case is unlikely a misspelling of !(x < 0) as no _Bool is less than zero and hopefully we get a warning for !(x < 0), what about _Bool test1a(int x) { return !(x << 0); } ? I think there is a non-zero chance this was meant to be !(x < 0) and the current pr102245.c: In function ‘test1a’: pr102245.c:3:14: warning: ‘<<’ in boolean context, did you mean ‘<’? [-Wint-in-bool-context] 3 | return !(x << 0); | ~~~^~~~~ warning seems to be useful. Jakub /* PR c/102245 */ /* { dg-options "-Wint-in-bool-context" } */ /* { dg-do compile } */ _Bool test1(_Bool x) { return !(x << 0); /* { dg-warning "boolean context" } */ } _Bool test2(_Bool x) { return !(x << 1); /* { dg-warning "boolean context" } */ } _Bool test3(_Bool x, int y) { return !(x << y); /* { dg-warning "boolean context" } */ } _Bool test4(int x, int y) { return !(x << y); /* { dg-warning "boolean context" } */ } _Bool test5(int x, int y) { return !((x << y) << 0); /* { dg-warning "boolean context" } */ } int test6(_Bool x) { int v = 0; return (v & ~1L) | (1L & (x << 0)); /* { dg-bogus "boolean context" } */ }
Comments
On Tue, Sep 14, 2021 at 9:44 AM Roger Sayle <roger@nextmovesoftware.com> wrote: > > > Respecting Jakub's suggestion that it may be better to warn-on-valid for > "if (x << 0)" as the author might have intended "if (x < 0)" [which will > also warn when x is _Bool], the simplest way to resolve this regression > is to disable the recently added fold transformation for shifts by zero; > these will be optimized later (elsewhere). Guarding against integer_zerop > is the simplest of three alternatives; the second being to only apply > this transformation to GIMPLE and not GENERIC, and the third (potentially) > being to explicitly handle shifts by zero here, with an (if cond then else), > optimizing the expression to a convert, but awkwardly duplicating the > more general transformation earlier in match.pd's shift simplifications. > > This patch has been tested (against a recent snapshot without build issues) > on x86_64-pc-linux-gnu with "make bootstrap" and "make -k check" with no > new failures. Note that test1 in the new testcase is changed from > dg-bogus to dg-warning compared with version #1. Ok for mainline? OK. Thanks, Richard. > 2021-09-14 Roger Sayle <roger@nextmovesoftware.com> > > gcc/ChangeLog > PR c/102245 > * match.pd (shift optimizations): Disable recent sign-changing > optimization for shifts by zero, these will be folded later. > > gcc/testsuite/ChangeLog > PR c/102245 > * gcc.dg/Wint-in-bool-context-4.c: New test case. > > > Roger > > -----Original Message----- > From: Jakub Jelinek <jakub@redhat.com> > Sent: 13 September 2021 11:58 > To: Roger Sayle <roger@nextmovesoftware.com> > Cc: 'GCC Patches' <gcc-patches@gcc.gnu.org> > Subject: Re: [PATCH] PR c/102245: Don't warn that ((_Bool)x<<0) isn't a truthvalue. > > On Mon, Sep 13, 2021 at 11:42:08AM +0100, Roger Sayle wrote: > > gcc/c-family/ChangeLog > > PR c/102245 > > * c-common.c (c_common_truthvalue_conversion) [LSHIFT_EXPR]: > > Special case (optimize) shifts by zero. > > > > gcc/testsuite/ChangeLog > > PR c/102245 > > * gcc.dg/Wint-in-bool-context-4.c: New test case. > > > > Roger > > -- > > > > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index > > 017e415..44b5fcc 100644 > > --- a/gcc/c-family/c-common.c > > +++ b/gcc/c-family/c-common.c > > @@ -3541,6 +3541,10 @@ c_common_truthvalue_conversion (location_t location, tree expr) > > break; > > > > case LSHIFT_EXPR: > > + /* Treat shifts by zero as a special case. */ > > + if (integer_zerop (TREE_OPERAND (expr, 1))) > > + return c_common_truthvalue_conversion (location, > > + TREE_OPERAND (expr, 0)); > > /* We will only warn on signed shifts here, because the majority of > > false positive warnings happen in code where unsigned arithmetic > > was used in anticipation of a possible overflow. > > > /* PR c/102245 */ > > /* { dg-options "-Wint-in-bool-context" } */ > > /* { dg-do compile } */ > > > > _Bool test1(_Bool x) > > { > > return !(x << 0); /* { dg-bogus "boolean context" } */ } > > While this exact case is unlikely a misspelling of !(x < 0) as no _Bool is less than zero and hopefully we get a warning for !(x < 0), what about _Bool test1a(int x) { > return !(x << 0); > } > ? I think there is a non-zero chance this was meant to be !(x < 0) and the current > pr102245.c: In function ‘test1a’: > pr102245.c:3:14: warning: ‘<<’ in boolean context, did you mean ‘<’? [-Wint-in-bool-context] > 3 | return !(x << 0); > | ~~~^~~~~ > warning seems to be useful. > > Jakub >
diff --git a/gcc/match.pd b/gcc/match.pd index 008f775..c6cc967 100644 --- a/gcc/match.pd +++ b/gcc/match.pd @@ -3401,13 +3401,15 @@ DEFINE_INT_AND_FLOAT_ROUND_FN (RINT) (cmp @0 @2))))) /* Both signed and unsigned lshift produce the same result, so use - the form that minimizes the number of conversions. */ + the form that minimizes the number of conversions. Postpone this + transformation until after shifts by zero have been folded. */ (simplify (convert (lshift:s@0 (convert:s@1 @2) INTEGER_CST@3)) (if (INTEGRAL_TYPE_P (type) && tree_nop_conversion_p (type, TREE_TYPE (@0)) && INTEGRAL_TYPE_P (TREE_TYPE (@2)) - && TYPE_PRECISION (TREE_TYPE (@2)) <= TYPE_PRECISION (type)) + && TYPE_PRECISION (TREE_TYPE (@2)) <= TYPE_PRECISION (type) + && !integer_zerop (@3)) (lshift (convert @2) @3))) /* Simplifications of conversions. */