From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 122509 invoked by alias); 1 May 2017 09:24:31 -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 122330 invoked by uid 89); 1 May 2017 09:24:22 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=pops, Hx-languages-length:3058, bullet X-HELO: mail-vk0-f54.google.com Received: from mail-vk0-f54.google.com (HELO mail-vk0-f54.google.com) (209.85.213.54) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 01 May 2017 09:24:20 +0000 Received: by mail-vk0-f54.google.com with SMTP id k4so56860577vki.1 for ; Mon, 01 May 2017 02:24:07 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=64f+jd23e7+Jxgrtx00wBWcq0ItoK1//78MFQStqa0o=; b=LBuDItNVTwIxSHEqWOr7JInfPZNqfclxw2uqsWtI3PHVuBKoP4776WBcfKIb/dcxmf EL+Yl2rexulcWxtIuKzqpgm+esy6Md3XzDzkwpE7WBBf3KBo5HRs9+hKPyo6dcL/755c G58YsvmJlnJwu1PyLdCjIUQAXV4mkckZ3R54y+e6LLYOgpLZ3+XZUfxPQ2cJpQM6ruMK A7Yqdj7kmNe6qkK1hKscigux2yr15QQHpz3liMYZ3YKCz2NZh8dSFua+1Ad45l5emBB7 VFwxmgB+Mnd9C0OKuh+bAx4A4tEAVT1qHmVj9aFw0DyFdkOMGr1nRnaVH2Y1i0PAitzs lzfg== X-Gm-Message-State: AN3rC/4yqqWGlzI6HHlnig+nvOD4D8irxt4PGOAGpfAnw5ZfyJdXbV9Z g/ZlnAkb7aWJicvM43kzP3eh4nCRAQ== X-Received: by 10.31.15.8 with SMTP id 8mr7488768vkp.5.1493630645663; Mon, 01 May 2017 02:24:05 -0700 (PDT) MIME-Version: 1.0 Received: by 10.103.49.206 with HTTP; Mon, 1 May 2017 02:24:05 -0700 (PDT) In-Reply-To: <9038c2be-280d-7db2-8fa2-7320e2324bb2@redhat.com> References: <20170427073250.GW1809@tucnak> <9038c2be-280d-7db2-8fa2-7320e2324bb2@redhat.com> From: Uros Bizjak Date: Mon, 01 May 2017 09:24:00 -0000 Message-ID: Subject: Re: [PATCH] Check for sp push/pop insns in reg_set_p (PR target/79430) To: Jeff Law Cc: Jakub Jelinek , Richard Biener , Eric Botcazou , "gcc-patches@gcc.gnu.org" Content-Type: text/plain; charset=UTF-8 X-SW-Source: 2017-05/txt/msg00004.txt.bz2 On Thu, Apr 27, 2017 at 6:13 PM, Jeff Law wrote: > 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. LGTM, with comparison to stack_pointer_rtx, as mentioned by Jeff above. Thanks, Uros.