From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 124870 invoked by alias); 27 Apr 2017 16:13:18 -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 124856 invoked by uid 89); 27 Apr 2017 16:13:17 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 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; Thu, 27 Apr 2017 16:13:16 +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 1378133C155; Thu, 27 Apr 2017 16:13:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 1378133C155 Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx05.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=law@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 1378133C155 Received: from localhost.localdomain (ovpn-117-12.phx2.redhat.com [10.3.117.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id A7853996D6; Thu, 27 Apr 2017 16:13:16 +0000 (UTC) Subject: Re: [PATCH] Check for sp push/pop insns in reg_set_p (PR target/79430) To: Jakub Jelinek , Richard Biener , Eric Botcazou , Uros Bizjak Cc: gcc-patches@gcc.gnu.org References: <20170427073250.GW1809@tucnak> From: Jeff Law Message-ID: <9038c2be-280d-7db2-8fa2-7320e2324bb2@redhat.com> Date: Thu, 27 Apr 2017 17:38:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 MIME-Version: 1.0 In-Reply-To: <20170427073250.GW1809@tucnak> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-04/txt/msg01415.txt.bz2 On 04/27/2017 01:32 AM, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR and can be seen on the testcase (too large for > testsuite, with lots of delta reduction I got 48KB *.f90 file still using > a dozen of modules), we miscompile it because we have mem(sp+64) memory > (what %st is loaded from) and are checking whether it is safe to move > earlier in the insn stream, and modified_between_p tells us it is, except > there is a stack pop instruction (i.e. sp autoinc). > And sp autoinc is apparently special in GCC: > /* There are no REG_INC notes for SP. */ Right. It's been the source of numerous problems through the years. One could argue that we should just bite the bullet and add them. The cost can't be that high and it'd avoid these kinds of problems in the future and allow for some code cleanups as well. We could probably scan the IL at the end of auto-inc-dec.c to add the missing notes. I thought I saw a comment once which indicates the rationale behind not including REG_INC notes for pushes/pops, but I can't find it anymore. > > The following patch handles that, plus then undoes that in ix86_agi_dependent > where from what I understood we want the previous behavior - push, pop and > call modifications of SP don't cause AGI stalls for addresses that have > SP base (SP can't appear as index). > > Not really sure about the == stack_pointer_rtx vs. > REG_P () && REGNO () == STACK_POINTER_REGNUM, there is lots of code that > just uses pointer comparisons and others that check REGNO, as an example > of the former e.g. push/pop_operand. So, is SP always shared, or can there > be other REGs with SP regno? SP is supposed to be shared, you should be able to compare against stack_pointer_rtx. > > Other than the ix86_agi_dependent which in my stats was the single case > that hit this difference, I've seen it making a difference e.g. in ifcvt > decisions, but at least the cases I've debugged didn't end up in any code > generation changes. E.g. both x86_64 and i686 libstdc++.so.6 and > libgo.so.11 as the two largest shared libraries built during bootstrap > are identical without/with this patch (objdump -dr is identical that is). > While without the config/i386/i386.c changes there were tons of differences. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2017-04-27 Jakub Jelinek > > PR target/79430 > * rtlanal.c (reg_set_p): If reg is a stack_pointer_rtx, also > check for stack push/pop autoinc. > * config/i386/i386.c (ix86_agi_dependent): Return false > if the only reason why modified_in_p returned true is that > addr is SP based and set_insn is a push or pop. THe rtlanal.c changes are fine by me. Uros should chime in on the x86 specific bits. Jeff