From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109470 invoked by alias); 17 Nov 2015 10:17:37 -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 109461 invoked by uid 89); 17 Nov 2015 10:17:36 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.7 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.2 X-HELO: eu-smtp-delivery-143.mimecast.com Received: from eu-smtp-delivery-143.mimecast.com (HELO eu-smtp-delivery-143.mimecast.com) (146.101.78.143) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 17 Nov 2015 10:17:35 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by eu-smtp-1.mimecast.com with ESMTP id uk-mta-37-xpklKAAwSDCiUHSkuThlxg-1; Tue, 17 Nov 2015 10:17:29 +0000 Received: from [10.2.206.200] ([10.1.2.79]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Tue, 17 Nov 2015 10:17:29 +0000 Message-ID: <564AFEB9.7040703@arm.com> Date: Tue, 17 Nov 2015 10:17:00 -0000 From: Kyrill Tkachov User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.2.0 MIME-Version: 1.0 To: Bernd Schmidt , GCC Patches CC: Jeff Law Subject: Re: [PATCH][RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves References: <5649E333.4090904@arm.com> <564A2339.3030308@redhat.com> <564AEE94.3070708@arm.com> <564AF80C.3010605@arm.com> In-Reply-To: <564AF80C.3010605@arm.com> X-MC-Unique: xpklKAAwSDCiUHSkuThlxg-1 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg02060.txt.bz2 On 17/11/15 09:49, Kyrill Tkachov wrote: > > On 17/11/15 09:08, Kyrill Tkachov wrote: >> Hi Bernd, >> >> On 16/11/15 18:40, Bernd Schmidt wrote: >>> On 11/16/2015 03:07 PM, Kyrill Tkachov wrote: >>> >>>> I've explained in the comments in the patch what's going on but the >>>> short version is trying to change the destination of a defining insn >>>> that feeds into an extend insn is not valid if the defining insn >>>> doesn't feed directly into the extend insn. In the ree pass the only >>>> way this can happen is if there is an intermediate conditional move >>>> that the pass tries to handle in a special way. An equivalent fix >>>> would have been to check on that path (when copy_needed in >>>> combine_reaching_defs is true) that the state->copies_list vector >>>> (that contains the conditional move insns feeding into the extend >>>> insn) is empty. >>> >>> I ran this through gdb, and I think I see what's going on. For referenc= e, here's a comment from the source: >>> >>> /* Considering transformation of >>> (set (reg1) (expression)) >>> ... >>> (set (reg2) (any_extend (reg1))) >>> >>> into >>> >>> (set (reg2) (any_extend (expression))) >>> (set (reg1) (reg2)) >>> ... */ >>> >>> I was thinking that another possible fix would be to also check !reg_us= ed_between_p for reg1 to ensure it's not used. I'm thinking this might be a= little clearer - what is your opinion? >> >> Yes, I had considered that as well. It should be equivalent. I didn't us= e !reg_used_between_p because I thought >> it'd be more expensive than checking reg_overlap_mentioned_p since we mu= st iterate over a number of instructions >> and call reg_overlap_mentioned_p on each one. But I suppose this case is= rare enough that it wouldn't make any >> measurable difference. > > Actually, I tried it out. And while a check reg_used_between_p fixed the = testcase, it caused code quality regressions > on aarch64. Seems it's too aggressive in restricting ree. > > I'll have a closer look. Ok, so the testcases that regress code-quality-wise on aarch64 look like th= is before ree: (insn 48 57 49 7 (set (reg:SI 7 x7) (zero_extend:SI (mem:QI (plus:DI (reg/f:DI 1 x1) (const_int 2 [0x2])))))) (insn 49 48 52 7 (set (reg/v:SI 2 x2) (reg:SI 7 x7))) (insn 52 49 53 7 (set (reg:DI 8 x8) (zero_extend:DI (reg:SI 7 x7)))) ree wants to transform this into: (insn 48 57 296 7 (set (reg:DI 8 x8) (zero_extend:DI (mem:QI (plus:DI (reg/f:DI 1 x1) (const_int 2 [0x2])))))) (insn 296 48 49 7 (set (reg:DI 7 x7) (reg:DI 8 x8))) (insn 49 296 53 7 (set (reg/v:SI 2 x2) (reg:SI 7 x7))) which is valid, but we reject that with the reg_used_between_p check becaus= e x7 is used in the intermediate insn 49. Note that no conditional move is present here. So, I think that the crucial element here is that the destination of the de= f_insn should feed directly into the extend, and that is what my original patch was testi= ng for. So, I'd like to keep my original proposed patch as is, although I think I'l= l add a couple of testcases from the duplicate PRs to it for the testsuite. What do you think? Thanks, Kyrill > > Kyrill > >> >> Would you prefer to use !reg_used_between_p here? >> >>> >>> The added comment could lead to some confusion since it's placed in fro= nt of an existing if statement that also tests a different condition. Also,= if we go with your fix, >>> >>>> + || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->i= nsn)))) >>> >>> Shouldn't this really be !rtx_equal_p? >>> >> >> Maybe, will it behave the right way if the two regs have different modes= or when subregs are involved? >> (will we even hit such a case in this path?) >> >> Thanks, >> Kyrill >>> >>> Bernd >>> >> >