From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 17828 invoked by alias); 11 May 2015 19:48:08 -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 17814 invoked by uid 89); 11 May 2015 19:48:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.8 required=5.0 tests=AWL,BAYES_00,SPF_HELO_PASS,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 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 (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 11 May 2015 19:48:06 +0000 Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id t4BJm5b3032404 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Mon, 11 May 2015 15:48:05 -0400 Received: from localhost.localdomain (ovpn-113-21.phx2.redhat.com [10.3.113.21]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id t4BJm48c026636; Mon, 11 May 2015 15:48:04 -0400 Message-ID: <55510774.5030107@redhat.com> Date: Mon, 11 May 2015 19:48:00 -0000 From: Jeff Law User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-Version: 1.0 To: Uros Bizjak , Jan Hubicka CC: Alexander Monakov , "gcc-patches@gcc.gnu.org" , Rich Felker Subject: Re: [PATCH i386] Extend sibcall peepholes to allow source in %eax 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> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-05/txt/msg01001.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. jeff