From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 75306 invoked by alias); 14 Feb 2018 05:06:52 -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 75297 invoked by uid 89); 14 Feb 2018 05:06:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=georg, Georg, advisable 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; Wed, 14 Feb 2018 05:06:50 +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 5CF562D1EC4 for ; Wed, 14 Feb 2018 05:06:49 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-16.rdu2.redhat.com [10.10.112.16]) by smtp.corp.redhat.com (Postfix) with ESMTP id 590CA60BE7; Wed, 14 Feb 2018 05:06:48 +0000 (UTC) Subject: Re: [PR81611] improve auto-inc To: Alexandre Oliva , gcc-patches@gcc.gnu.org References: From: Jeff Law Message-ID: Date: Wed, 14 Feb 2018 05:06:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2018-02/txt/msg00806.txt.bz2 On 01/23/2018 08:42 PM, Alexandre Oliva wrote: > These two patches fix PR81611. > > The first one improves forwprop so that we avoid adding SSA conflicting > by forwpropping the iv increment, which may cause both the incremented > and the original value to be live, even when the iv is copied between > the PHI node and the increment. We already handled the case in which > there aren't any such copies. > > Alas, this is not enough to address the problem on avr, even though it > fixes it on e.g. ppc. The reason is that avr rejects var+offset > addresses, and this prevents the memory access in a post-inc code > sequence from being adjusted to an address that auto-inc-dec can > recognize. > > The second patch adjusts auto-inc-dec to recognize a code sequence in > which the original, unincremented pseudo is used in an address after > it's incremented into another pseudo, and turn that into a post-inc > address, leaving the copying for subsequent passes to eliminate. > > Regstrapped on x86_64-linux-gnu, i686-linux-gnu, ppc64-linux-gnu and > aarch64-linux-gnu. Ok to install? > > > I'd appreciate suggestions on how to turn the submitted testcase into a > regression test; I suppose an avr-specific test that requires the > auto-inc transformation is a possibility, but that feels a bit too > limited, doesn't it? Thoughts? Thanks in advance, > > [ snip the DOM change which was approved and installed ] > [PR81611] turn inc-and-use-of-dead-orig into auto-inc > > When the addressing modes available on the machine don't allow offsets > in addresses, odds are that post-increments will be represented in > trees and RTL as: > > y <= x + 1 > ... *(x) ... > x <= y > > so deal with this form so as to create auto-inc addresses that we'd > otherwise miss. > > > for gcc/ChangeLog > > PR rtl-optimization/81611 > * auto-inc-dec.c (attempt_change): Move dead note from > mem_insn if it's the next use of regno > (find_address): Take address use of reg holding > non-incremented value. > (merge_in_block): Attempt to use a mem insn that is the next > use of the original regno. > --- > gcc/auto-inc-dec.c | 46 ++++++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 44 insertions(+), 2 deletions(-) > > diff --git a/gcc/auto-inc-dec.c b/gcc/auto-inc-dec.c > index d02fa9d081c7..4ffbcf56a456 100644 > --- a/gcc/auto-inc-dec.c > +++ b/gcc/auto-inc-dec.c > @@ -508,7 +508,11 @@ attempt_change (rtx new_addr, rtx inc_reg) > before the memory reference. */ > gcc_assert (mov_insn); > emit_insn_before (mov_insn, inc_insn.insn); > - move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0); > + regno = REGNO (inc_insn.reg0); > + if (reg_next_use[regno] == mem_insn.insn) > + move_dead_notes (mov_insn, mem_insn.insn, inc_insn.reg0); > + else > + move_dead_notes (mov_insn, inc_insn.insn, inc_insn.reg0); > > regno = REGNO (inc_insn.reg_res); > reg_next_def[regno] = mov_insn; > @@ -1370,6 +1385,33 @@ merge_in_block (int max_reg, basic_block bb) > insn_is_add_or_inc = false; > } > } > + > + if (ok && insn_is_add_or_inc > + && inc_insn.reg0 != inc_insn.reg_res) > + { > + regno = REGNO (inc_insn.reg0); > + int luid = DF_INSN_LUID (mem_insn.insn); > + mem_insn.insn = get_next_ref (regno, bb, reg_next_use); So I think a comment is warranted right as we enter the TRUE arm. At that point INC_INSN is an inc/dec. But MEM_INSN is not necessarily a memory reference. It could be a memory reference, it could be a copy, it could be something completely different (it's just the next insn that references the result of the increment). In the case we care about we want it to be a copy of INC_INSN's REG_RES back to REG0. ISTM that verifying MEM_INSN is a reg->reg copy (reg_res -> reg0) before we call get_next_ref for reg0 is advisable and probably good from a compile-time standpoint by avoiding calls into find_address. After we call get_next_ref to get the next reference of the source of the increment, then we're hoping to find a memory reference that uses REG0. But it's not guaranteed it's a memory reference insn. I was having an awful time understanding how this code could work from the comments until I put it under a debugger and got a sense of the state as we entered that IF block. Then it was much clearer :-) I believe Georg had other testcases in subsequent comments in the BZ, but I don't believe they were flagged as regressions. So I think once you check in your patch we note in the BZ that the original testcase (and thus the regression) is fixed, but that the later tests are not. We then remove the regression marker. If Georg (or anyone else) does the analysis to show those later tests are also regressions, then we'll add the regression marker back. Seem reasonable? Jeff