From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 106343 invoked by alias); 31 May 2019 07:24:55 -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 106333 invoked by uid 89); 31 May 2019 07:24:55 -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=pressure, H*Ad:U*dberlin 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; Fri, 31 May 2019 07:24:53 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 3E1A1ACF5; Fri, 31 May 2019 07:24:50 +0000 (UTC) Date: Fri, 31 May 2019 07:30:00 -0000 From: Richard Biener To: Jeff Law cc: Jiufu Guo , gcc-patches@gcc.gnu.org, Jakub Jelinek , Daniel Berlin , segher@kernel.crashing.org, wschmidt@linux.ibm.com Subject: Re: [PATCH] A jump threading opportunity for condition branch In-Reply-To: <1db82c8d-b903-5396-1919-f4f39472b8a8@redhat.com> Message-ID: References: <1558446288-52444-1-git-send-email-guojiufu@linux.ibm.com> <59aaa715-13de-376c-e806-cc53d44aad03@redhat.com> <2EFC203E-8C86-4640-9CC1-5B4C916FD496@suse.de> <1db82c8d-b903-5396-1919-f4f39472b8a8@redhat.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2019-05/txt/msg02084.txt.bz2 On Thu, 30 May 2019, Jeff Law wrote: > On 5/30/19 12:41 AM, Richard Biener wrote: > > On May 29, 2019 10:18:01 PM GMT+02:00, Jeff Law wrote: > >> On 5/23/19 6:11 AM, Richard Biener wrote: > >>> On Thu, 23 May 2019, Jiufu Guo wrote: > >>> > >>>> Hi, > >>>> > >>>> Richard Biener writes: > >>>> > >>>>> On Tue, 21 May 2019, Jiufu Guo wrote: > >> > >>>>>> + } > >>>>>> + > >>>>>> + if (TREE_CODE_CLASS (gimple_assign_rhs_code (def)) != > >> tcc_comparison) > >>>>>> + return false; > >>>>>> + > >>>>>> + /* Check if phi's incoming value is defined in the incoming > >> basic_block. */ > >>>>>> + edge e = gimple_phi_arg_edge (phi, index); > >>>>>> + if (def->bb != 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 != e->src)" make sure the cmp is define in the incoming > >>>> block of the current; and then combining "cmp with gcond" is safe. > >> If > >>>> the cmp is defined far from the incoming block, it would be hard to > >>>> achieve the combining, and the transform may not needed. > >>> We're in SSA form so the "combining" doesn't really care where the > >>> definition comes from. > >> Combining doesn't care, but we need to make sure the copy of the > >> conditional ends up in the right block since it wouldn't necessarily be > >> associated with def->bb anymore. But I'd expect the sinking pass to > >> make this a non-issue in practice anyway. > >> > >>> > >>>>>> + > >>>>>> + 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. > >>> I mean threading will isolate the path making this trivially true. > >>> It's also no requirement for combining, in fact due to the single-use > >>> check the definition can be sinked across the edge already (if > >>> the edges dest didn't have multiple predecessors which this threading > >>> will fix as well). > >> I don't think so. The CMP source block could end with a call and have > >> an abnormal edge (for example). We can't put the copied conditional > >> before the call and putting it after the call essentially means > >> creating > >> a new block. > >> > >> The CMP source block could also end with a conditional. Where do we > >> put > >> the one we want to copy into the CMP source block in that case? :-) > >> > >> This is something else we'd want to check if we ever allowed the the > >> CMP > >> defining block to not be the immediate predecessor of the conditional > >> jump block. If we did that we'd need to validate that the block where > >> we're going to insert the copy of the jump has a single successor. > > > > But were just isolating a path here. The actual combine job is left to followup cleanups. > Absolutely agreed. My point was that there's some additional stuff we'd > have to verify does the right thing if we wanted to allow the CMP to be > somewhere other than in the immediate predecessor of the conditional > jump block. For correctness? No. For the CMP to be forwarded? No. For optimality maybe - forwarding a binary operation always incurs register pressure increase. Btw, as you already said sinking should have sinked the CMP to the predecessor (since we have a single use in the PHI). So I hardly see the point of making this difference. Richard.