From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 59935 invoked by alias); 31 May 2019 15:03:50 -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 59927 invoked by uid 89); 31 May 2019 15:03:50 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy= X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 31 May 2019 15:03:48 +0000 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 56E9D30BC584; Fri, 31 May 2019 15:03:34 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-56.rdu2.redhat.com [10.10.112.56]) by smtp.corp.redhat.com (Postfix) with ESMTP id 74108891A0; Fri, 31 May 2019 15:03:29 +0000 (UTC) Subject: Re: [PATCH] A jump threading opportunity for condition branch To: Richard Biener Cc: Jiufu Guo , gcc-patches@gcc.gnu.org, Jakub Jelinek , Daniel Berlin , segher@kernel.crashing.org, wschmidt@linux.ibm.com 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> From: Jeff Law Openpgp: preference=signencrypt Message-ID: <47b8d7f9-0039-593e-0813-7dc5e58acfe9@redhat.com> Date: Fri, 31 May 2019 15:28:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-05/txt/msg02136.txt.bz2 On 5/31/19 1:24 AM, Richard Biener wrote: > 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. For correctness of the patch. Conceptually I have _no_ issues with having the CMP in a different block than an immediate predecessor of the conditional jump block. But the patch does certain code which would need to be audited with that change in mind. > > 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. :-) jeff