Message ID | Y6QxsdUy2S2w//u/@tucnak |
---|---|
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 65C623857BB3 for <patchwork@sourceware.org>; Thu, 22 Dec 2022 10:30:45 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 65C623857BB3 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1671705045; bh=iRBOWLhtzbdBoY9fYa1EnR6jJ/SPRMuKVCEBSgUsjsE=; h=Date:To:Cc:Subject:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=o4VfZ3As5+TJVqOzgulrdhlKn0Oq57GexB6kzGpQTZKxL8ZRXPtZeWHOK/bwHP5N1 K22iocXvP4voGZLr4gZNiK5m6eKUcFJBIMYSesnwlskMoEFp8WW+0y6PToPhY3JpTh tfsLSiNdrxCmoZoiC9qhL33qfCd4wcsVcHMvIZ+4= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 8AFC33858425 for <gcc-patches@gcc.gnu.org>; Thu, 22 Dec 2022 10:30:17 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8AFC33858425 Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-349-ZWXoqpj9NCC5eK9F8Urh8A-1; Thu, 22 Dec 2022 05:30:15 -0500 X-MC-Unique: ZWXoqpj9NCC5eK9F8Urh8A-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 21B89101A521; Thu, 22 Dec 2022 10:30:15 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.195.114]) by smtp.corp.redhat.com (Postfix) with ESMTPS id ABE0853A0; Thu, 22 Dec 2022 10:30:14 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 2BMAUAWs1189295 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Thu, 22 Dec 2022 11:30:11 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 2BMAUAmT1189294; Thu, 22 Dec 2022 11:30:10 +0100 Date: Thu, 22 Dec 2022 11:30:09 +0100 To: Richard Biener <rguenther@suse.de> Cc: gcc-patches@gcc.gnu.org, Aldy Hernandez <aldyh@redhat.com>, Andrew MacLeod <amacleod@redhat.com> Subject: [PATCH] phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166] Message-ID: <Y6QxsdUy2S2w//u/@tucnak> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-3.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) 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> From: Jakub Jelinek via Gcc-patches <gcc-patches@gcc.gnu.org> Reply-To: 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 |
phiopt: Drop SSA_NAME_RANGE_INFO in maybe equal case [PR108166]
|
|
Commit Message
Jakub Jelinek
Dec. 22, 2022, 10:30 a.m. UTC
Hi! The following place in value_replacement is after proving that x == cst1 ? cst2 : x phi result is only used in a comparison with constant which doesn't care if it compares cst1 or cst2 and replaces it with x. The testcase is miscompiled because we have after the replacement incorrect range info for the phi result, we would need to effectively union the phi result range with cst1 (oarg in the code) because previously that constant might be missing in the range, but newly it can appear (we've just verified that the single use stmt of the phi result doesn't care about that value in particular). The following patch just resets the info, bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO (including adjusting non-zero bits and the like)? 2022-12-22 Jakub Jelinek <jakub@redhat.com> PR tree-optimization/108166 * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result. * g++.dg/torture/pr108166.C: New test. Jakub
Comments
On Thu, Dec 22, 2022 at 11:30 AM Jakub Jelinek <jakub@redhat.com> wrote: > > Hi! > > The following place in value_replacement is after proving that > x == cst1 ? cst2 : x > phi result is only used in a comparison with constant which doesn't > care if it compares cst1 or cst2 and replaces it with x. > The testcase is miscompiled because we have after the replacement > incorrect range info for the phi result, we would need to > effectively union the phi result range with cst1 (oarg in the code) > because previously that constant might be missing in the range, but > newly it can appear (we've just verified that the single use stmt > of the phi result doesn't care about that value in particular). > > The following patch just resets the info, bootstrapped/regtested > on x86_64-linux and i686-linux, ok for trunk? > > Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO > with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO > (including adjusting non-zero bits and the like)? if (get_global_range_query ()->range_of_expr (r, <SSA>, <STMT>)) { int_range<2> tmp (<INTEGER_CST>, <INTEGER_CST>); r.union_ (tmp); set_range_info (<SSA>, r); } Note that the <STMT> is unused, as the global range doesn't have context. But it is good form to pass it since we could decide at a later time to replace get_global_range_query() with a context-aware get_range_query(), or a ranger instance. But <STMT> is not strictly necessary. Hmmm, set_range_info() is an intersect operation, because we always update what's already there, never replace it. If want to replace the global range, throwing away whatever range we had there, you may want to call this first: /* Reset all flow sensitive data on NAME such as range-info, nonzero bits and alignment. */ void reset_flow_sensitive_info (tree name) { } Aldy > > 2022-12-22 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/108166 > * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p > case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result. > > * g++.dg/torture/pr108166.C: New test. > > --- gcc/tree-ssa-phiopt.cc.jj 2022-10-28 11:00:53.970243821 +0200 > +++ gcc/tree-ssa-phiopt.cc 2022-12-21 14:27:58.118326548 +0100 > @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb, > default: > break; > } > + if (equal_p) > + /* After the optimization PHI result can have value > + which it couldn't have previously. > + We could instead of resetting it union the range > + info with oarg. */ > + reset_flow_sensitive_info (gimple_phi_result (phi)); > if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS) > { > imm_use_iterator imm_iter; > --- gcc/testsuite/g++.dg/torture/pr108166.C.jj 2022-12-21 14:31:02.638661322 +0100 > +++ gcc/testsuite/g++.dg/torture/pr108166.C 2022-12-21 14:30:45.441909725 +0100 > @@ -0,0 +1,26 @@ > +// PR tree-optimization/108166 > +// { dg-do run } > + > +bool a, b; > +int d, c; > + > +const int & > +foo (const int &f, const int &g) > +{ > + return !f ? f : g; > +} > + > +__attribute__((noipa)) void > +bar (int) > +{ > +} > + > +int > +main () > +{ > + c = foo (b, 0) > ((b ? d : b) ?: 8); > + a = b ? d : b; > + bar (a); > + if (a != 0) > + __builtin_abort (); > +} > > Jakub >
On Thu, 22 Dec 2022, Jakub Jelinek wrote: > Hi! > > The following place in value_replacement is after proving that > x == cst1 ? cst2 : x > phi result is only used in a comparison with constant which doesn't > care if it compares cst1 or cst2 and replaces it with x. > The testcase is miscompiled because we have after the replacement > incorrect range info for the phi result, we would need to > effectively union the phi result range with cst1 (oarg in the code) > because previously that constant might be missing in the range, but > newly it can appear (we've just verified that the single use stmt > of the phi result doesn't care about that value in particular). > > The following patch just resets the info, bootstrapped/regtested > on x86_64-linux and i686-linux, ok for trunk? OK. > Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO > with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO > (including adjusting non-zero bits and the like)? There's no get_range_info on SSA_NAMEs (anymore?) but you can construct a value_range from the INTEGER_CST singleton and union that into the SSA_NAMEs range and then do set_range_info with the altered range I guess. Richard. > 2022-12-22 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/108166 > * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p > case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result. > > * g++.dg/torture/pr108166.C: New test. > > --- gcc/tree-ssa-phiopt.cc.jj 2022-10-28 11:00:53.970243821 +0200 > +++ gcc/tree-ssa-phiopt.cc 2022-12-21 14:27:58.118326548 +0100 > @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb, > default: > break; > } > + if (equal_p) > + /* After the optimization PHI result can have value > + which it couldn't have previously. > + We could instead of resetting it union the range > + info with oarg. */ > + reset_flow_sensitive_info (gimple_phi_result (phi)); > if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS) > { > imm_use_iterator imm_iter; > --- gcc/testsuite/g++.dg/torture/pr108166.C.jj 2022-12-21 14:31:02.638661322 +0100 > +++ gcc/testsuite/g++.dg/torture/pr108166.C 2022-12-21 14:30:45.441909725 +0100 > @@ -0,0 +1,26 @@ > +// PR tree-optimization/108166 > +// { dg-do run } > + > +bool a, b; > +int d, c; > + > +const int & > +foo (const int &f, const int &g) > +{ > + return !f ? f : g; > +} > + > +__attribute__((noipa)) void > +bar (int) > +{ > +} > + > +int > +main () > +{ > + c = foo (b, 0) > ((b ? d : b) ?: 8); > + a = b ? d : b; > + bar (a); > + if (a != 0) > + __builtin_abort (); > +} > > Jakub > >
On Thu, Dec 22, 2022, 12:33 Richard Biener <rguenther@suse.de> wrote: > On Thu, 22 Dec 2022, Jakub Jelinek wrote: > > > Hi! > > > > The following place in value_replacement is after proving that > > x == cst1 ? cst2 : x > > phi result is only used in a comparison with constant which doesn't > > care if it compares cst1 or cst2 and replaces it with x. > > The testcase is miscompiled because we have after the replacement > > incorrect range info for the phi result, we would need to > > effectively union the phi result range with cst1 (oarg in the code) > > because previously that constant might be missing in the range, but > > newly it can appear (we've just verified that the single use stmt > > of the phi result doesn't care about that value in particular). > > > > The following patch just resets the info, bootstrapped/regtested > > on x86_64-linux and i686-linux, ok for trunk? > > OK. > > > Aldy/Andrew, how would one instead union the SSA_NAME_RANGE_INFO > > with some INTEGER_CST and store it back into SSA_NAME_RANGE_INFO > > (including adjusting non-zero bits and the like)? > > There's no get_range_info on SSA_NAMEs (anymore?) but you can > construct a value_range from the This is my fault. When we added get_global_range_query, I removed get_range_info. I should've left that entry point. I'll look into adding it next cycle for readability's sake INTEGER_CST singleton and > union that into the SSA_NAMEs range and then do set_range_info > with the altered range I guess. > Note that set_range_info is an intersect operation. It should really be called update_range_info. Up to now, there were no users that wanted to clobber old ranges completely. Aldy > Richard. > > > 2022-12-22 Jakub Jelinek <jakub@redhat.com> > > > > PR tree-optimization/108166 > > * tree-ssa-phiopt.cc (value_replacement): For the maybe_equal_p > > case turned into equal_p reset SSA_NAME_RANGE_INFO of phi result. > > > > * g++.dg/torture/pr108166.C: New test. > > > > --- gcc/tree-ssa-phiopt.cc.jj 2022-10-28 11:00:53.970243821 +0200 > > +++ gcc/tree-ssa-phiopt.cc 2022-12-21 14:27:58.118326548 +0100 > > @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb, > > default: > > break; > > } > > + if (equal_p) > > + /* After the optimization PHI result can have value > > + which it couldn't have previously. > > + We could instead of resetting it union the range > > + info with oarg. */ > > + reset_flow_sensitive_info (gimple_phi_result (phi)); > > if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS) > > { > > imm_use_iterator imm_iter; > > --- gcc/testsuite/g++.dg/torture/pr108166.C.jj 2022-12-21 > 14:31:02.638661322 +0100 > > +++ gcc/testsuite/g++.dg/torture/pr108166.C 2022-12-21 > 14:30:45.441909725 +0100 > > @@ -0,0 +1,26 @@ > > +// PR tree-optimization/108166 > > +// { dg-do run } > > + > > +bool a, b; > > +int d, c; > > + > > +const int & > > +foo (const int &f, const int &g) > > +{ > > + return !f ? f : g; > > +} > > + > > +__attribute__((noipa)) void > > +bar (int) > > +{ > > +} > > + > > +int > > +main () > > +{ > > + c = foo (b, 0) > ((b ? d : b) ?: 8); > > + a = b ? d : b; > > + bar (a); > > + if (a != 0) > > + __builtin_abort (); > > +} > > > > Jakub > > > > > > -- > Richard Biener <rguenther@suse.de> > SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, > Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; > HRB 36809 (AG Nuernberg) > >
--- gcc/tree-ssa-phiopt.cc.jj 2022-10-28 11:00:53.970243821 +0200 +++ gcc/tree-ssa-phiopt.cc 2022-12-21 14:27:58.118326548 +0100 @@ -1491,6 +1491,12 @@ value_replacement (basic_block cond_bb, default: break; } + if (equal_p) + /* After the optimization PHI result can have value + which it couldn't have previously. + We could instead of resetting it union the range + info with oarg. */ + reset_flow_sensitive_info (gimple_phi_result (phi)); if (equal_p && MAY_HAVE_DEBUG_BIND_STMTS) { imm_use_iterator imm_iter; --- gcc/testsuite/g++.dg/torture/pr108166.C.jj 2022-12-21 14:31:02.638661322 +0100 +++ gcc/testsuite/g++.dg/torture/pr108166.C 2022-12-21 14:30:45.441909725 +0100 @@ -0,0 +1,26 @@ +// PR tree-optimization/108166 +// { dg-do run } + +bool a, b; +int d, c; + +const int & +foo (const int &f, const int &g) +{ + return !f ? f : g; +} + +__attribute__((noipa)) void +bar (int) +{ +} + +int +main () +{ + c = foo (b, 0) > ((b ? d : b) ?: 8); + a = b ? d : b; + bar (a); + if (a != 0) + __builtin_abort (); +}