From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29193 invoked by alias); 7 Jun 2019 00:05:01 -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 29170 invoked by uid 89); 7 Jun 2019 00:05:01 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS autolearn=ham version=3.3.1 spammy=H*Ad:U*dberlin, instability, fresh 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, 07 Jun 2019 00:05:00 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id DBD3337F43; Fri, 7 Jun 2019 00:04:53 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-57.rdu2.redhat.com [10.10.112.57]) by smtp.corp.redhat.com (Postfix) with ESMTP id D25E068427; Fri, 7 Jun 2019 00:04:50 +0000 (UTC) From: Jeff Law 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> <47b8d7f9-0039-593e-0813-7dc5e58acfe9@redhat.com> Openpgp: preference=signencrypt Message-ID: <3c106035-d678-8b61-6b55-6d17a55dea37@redhat.com> Date: Fri, 07 Jun 2019 00:05: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: <47b8d7f9-0039-593e-0813-7dc5e58acfe9@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2019-06/txt/msg00402.txt.bz2 On 5/31/19 9:03 AM, Jeff Law wrote: > 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. > :-) So just to satisfy my curiosity I put in some instrumentation to check for cases where the CMP is not in an immediate predecessor of the conditional branch. It happens. It's not terribly common though. I'd guess it's cases where this code is running before sinking. I went ahead and audited the patch for this case so that we could just eliminate that test. The key thing thing is that we don't use the block with the CMP insn at all in this code. So there's no possibility of duplicating the conditional into the wrong block or anything like that. Since this code is running from within thread_across_edge it can't be called with complex/abnormal edges or any other cases that can't be handled since we filter those out before calling thread_across_edge. So it should be safe to just eliminate that conditional. There's been a lot of testing instability lately, so it's hard to know if my testruns were clean or not since they update from the trunk each time. I'm going to do a fresh round and hopefully get usable results. jeff