From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 124464 invoked by alias); 30 May 2019 06:40:12 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 124451 invoked by uid 89); 30 May 2019 06:40:12 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.9 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 30 May 2019 06:40:10 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id F0D81AB42; Thu, 30 May 2019 06:40:07 +0000 (UTC) Date: Thu, 30 May 2019 06:41:00 -0000 User-Agent: K-9 Mail for Android In-Reply-To: <18b7e6a9-ee69-434d-adbf-75661113f1f6@redhat.com> References: <1558446288-52444-1-git-send-email-guojiufu@linux.ibm.com> <18b7e6a9-ee69-434d-adbf-75661113f1f6@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [PATCH] A jump threading opportunity for condition branch To: Jeff Law ,Jiufu Guo CC: gcc-patches@gcc.gnu.org,Jakub Jelinek ,Daniel Berlin ,segher@kernel.crashing.org,wschmidt@linux.ibm.com From: Richard Biener Message-ID: <63105C16-2CE6-417D-A99A-7C13B45378B2@suse.de> X-SW-Source: 2019-05/txt/msg01989.txt.bz2 On May 29, 2019 10:12:31 PM GMT+02:00, Jeff Law wrote: >On 5/23/19 6:05 AM, Jiufu Guo wrote: >> Hi, >>=20 >> Richard Biener writes: >>=20 >>> On Tue, 21 May 2019, Jiufu Guo wrote: >>> > >>>>=20=20 >>>> +/* Return true if PHI's INDEX-th incoming value is a CMP, and the >CMP is >>>> + defined in the incoming basic block. Otherwise return false.=20 >*/ >>>> +static bool >>>> +cmp_from_unconditional_block (gphi *phi, int index) >>>> +{ >>>> + tree value =3D gimple_phi_arg_def (phi, index); >>>> + if (!(TREE_CODE (value) =3D=3D SSA_NAME && has_single_use (value))) >>>> + return false; >>> Not sure why we should reject a constant here but I guess we >>> expect it to find a simplified condition anyways ;) >>> >> Const could be accepted here, like "# t_9 =3D PHI <5(3), t_17(4)>". I >> found this case is already handled by other jump-threading code, like >> 'ethread' pass. >Right. There's no need to handle constants here. They'll result in >trivially discoverable jump threading opportunities. > >>>> + /* Check if phi's incoming value is defined in the incoming >basic_block. */ >>>> + edge e =3D gimple_phi_arg_edge (phi, index); >>>> + if (def->bb !=3D e->src) >>>> + return false; >>> why does this matter? >>> >> Through preparing pathes and duplicating block, this transform can >also >> help to combine a cmp in previous block and a gcond in current block. >> "if (def->bb !=3D e->src)" make sure the cmp is define in the incoming >> block of the current; and then combining "cmp with gcond" is safe.=20 >If >> the cmp is defined far from the incoming block, it would be hard to >> achieve the combining, and the transform may not needed. >I don't think it's strictly needed in the long term and could be >addressed in a follow-up if we can find cases where it helps. I think >we'd just need to double check insertion of the new conditional branch >to relax this if we cared. > >However, I would expect sinking to have done is job here and would be >surprised if trying to handle this actually improved any real world >code. >>=20 >>>> + >>>> + if (!single_succ_p (def->bb)) >>>> + return false; >>> Or this? The actual threading will ensure this will hold true. >>> >> Yes, other thread code check this and ensure it to be true, like >> function thread_through_normal_block. Since this new function is >invoked >> outside thread_through_normal_block, so, checking single_succ_p is >also >> needed for this case. >Agreed that it's needed. Consider if the source block has multiple >successors. Where do we insert the copy of the conditional branch? We're duplicating its block? That is, we are isolating a path into a condit= ional - that's always possible? I wanted to make sure that when threading t= hreads through a conditional in the block with the compare we'd add the ext= ra tail duplication? AFAIK we're still looking at unmodified CFG here? > >>>> +{ >>>> + gimple *gs =3D last_and_only_stmt (bb); >>>> + if (gs =3D=3D NULL) >>>> + return false; >>>> + >>>> + if (gimple_code (gs) !=3D GIMPLE_COND) >>>> + return false; >>>> + >>>> + tree cond =3D gimple_cond_lhs (gs); >>>> + >>>> + if (TREE_CODE (cond) !=3D SSA_NAME) >>>> + return false; >>> space after if( too much vertical space in this function >>> for my taste btw. >> Will update this. >>> For the forwarding to work we want a NE_EXPR or EQ_EXPR >>> as gimple_cond_code and integer_one_p or integer_zero_p >>> gimple_cond_rhs. >> Right, checking those would be more safe. Since no issue found, >during >> bootstrap and regression tests, so I did not add these checking. I >will >> add this checking. >Definitely want to verify that we're dealing with an equality test >against 0/1. > >Jeff