From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32564 invoked by alias); 11 May 2015 20:16:03 -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 32550 invoked by uid 89); 11 May 2015 20:16:02 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.4 required=5.0 tests=AWL,BAYES_00,KAM_LAZY_DOMAIN_SECURITY,T_RP_MATCHES_RCVD autolearn=no version=3.3.2 X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 11 May 2015 20:16:00 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 3317E541AE2; Mon, 11 May 2015 22:15:56 +0200 (CEST) Date: Mon, 11 May 2015 20:16:00 -0000 From: Jan Hubicka To: Jeff Law Cc: Uros Bizjak , Jan Hubicka , Alexander Monakov , "gcc-patches@gcc.gnu.org" , Rich Felker Subject: Re: [PATCH i386] Extend sibcall peepholes to allow source in %eax Message-ID: <20150511201555.GA49471@kam.mff.cuni.cz> References: <1430757479-14241-1-git-send-email-amonakov@ispras.ru> <1430757479-14241-4-git-send-email-amonakov@ispras.ru> <20150510165402.GH9659@atrey.karlin.mff.cuni.cz> <20150511180038.GA22960@kam.mff.cuni.cz> <55510774.5030107@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55510774.5030107@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-SW-Source: 2015-05/txt/msg01002.txt.bz2 > On 05/11/2015 01:46 PM, Uros Bizjak wrote: > >On Mon, May 11, 2015 at 8:00 PM, Jan Hubicka wrote: > >>>On Sun, 10 May 2015, Jan Hubicka wrote: > >>> > >>>>>On i386, peepholes that transform memory load and register-indirect jump into > >>>>>memory-indirect jump are overly restrictive in that they don't allow combining > >>>>>when the jump target is loaded into %eax, and the called function returns a > >>>>>value (also in %eax, so it's not dead after the call). Fix this by checking > >>>>>for same source and output register operands separately. > >>>>> > >>>>>OK? > >>>>> * config/i386/i386.md (sibcall_value_memory): Extend peepholes to > >>>>> allow memory address in %eax. > >>>>> (sibcall_value_pop_memory): Likewise. > >>>> > >>>>Why do we need the check for liveness after all? There is SIBLING_CALL_P > >>>>(peep2_next_insn (1)) so we know that the function terminates by the call > >>>>and there are no other uses of the value. > >>> > >>>Indeed. Uros, the peep2_reg_dead_p check was added by your patch as svn > >>>revision 211776, git commit e51f8b8fed. Would you agree that the check is not > >>>necessary for sibcalls as Honza explains? Would you approve a patch that > >>>removes it in the sibcall peepholes I modify in the patch under discussion? > >>> > >>>>Don't we however need to check that operands[0] is not used by the call_insn as > >>>>parameter of the call? I.e. something like > >>>> > >>>>void > >>>>test(void (*callback ())) > >>>>{ > >>>> callback(callback); > >>>>} > >>> > >>>You need a pointer-to-pointer-to-function to trigger the peephole. Something > >>>like this: > >>> > >>> void foo() > >>> { > >>> void (**bar)(void*); > >>> asm("":"=r"(bar)); > >>> (*bar)(*bar); > >>> } > >>> > >>>>I think instead of peep2_reg_dead_p we want to check that the parameter is not in > >>>>CALL_INSN_FUNCTION_USAGE of the sibcall.. > >>> > >>>Playing with the above testcase I can't induce failure. It seems today GCC > >>>won't allocate the same register as callee address and one of the arguments. > >>>Do you want me to implement such a check anyway? > >> > >>Hmm, only way I can trigger same register is: > >>void foo() > >> { > >> void (**bar)(void*); > >> asm("":"=r"(bar)); > >> register void (*var)(void *) asm("%eax"); > >> var=*bar; > >> asm("":"+r"(var)); > >> var(var); > >> } > >> > >>removing the second asm makes CSE to forward propagae the memory operand > >>to call that makes the call different from the register variable. > >> > >>Still I would check for that, but this is more Uros' area. > > > >This is from [1], and reading this reference, it looks to me that the > >check was introduced due to: > > > >- Adds check that eliminated register is really dead after the call > >(maybe an overkill, but some hard-to-debug problems surfaced due to > >missing liveness checks in the past) > > > >Going down that memory lane, it looks like a safety check for > >something that *might* happen. Looking at the comment, I'd say we can > >remove the check, but we should look for possible fallout. > I'd tend to agree. Yes, to make my original email clear, I think we are safe to remove peep2_reg_dead_p. I would however introduce a check that the call target is not also among parameters of the function. In this case the peephole would remove the load and make the parameter unefined. While current mainline don't seem to be able to translate the testcase above that way, perhaps future improvements to LRA/postreload gcse may make it happen and generally RTL patterns are better to be safe by definition not only for the actual RTL we are able to generate. I suppose reg_mentioned_p on call usage is enough. Honza > > jeff