From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 74141 invoked by alias); 9 Oct 2015 07:10:34 -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 74131 invoked by uid 89); 9 Oct 2015 07:10:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.3 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 09 Oct 2015 07:10:31 +0000 Received: from KLMAIL01.kl.imgtec.org (unknown [192.168.5.35]) by Websense Email Security Gateway with ESMTPS id 9DA679A98F8ED; Fri, 9 Oct 2015 08:10:25 +0100 (IST) Received: from hhmail02.hh.imgtec.org (10.100.10.20) by KLMAIL01.kl.imgtec.org (192.168.5.35) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 9 Oct 2015 08:10:27 +0100 Received: from hhmail02.hh.imgtec.org ([::1]) by hhmail02.hh.imgtec.org ([::1]) with mapi id 14.03.0235.001; Fri, 9 Oct 2015 08:10:27 +0100 From: Robert Suchanek To: Bernd Schmidt , "ebotcazou@adacore.com" , "gcc-patches@gcc.gnu.org" Subject: RE: [RFC][PATCH] Preferred rename register in regrename pass Date: Fri, 09 Oct 2015 07:10:00 -0000 Message-ID: References: <55FC2B8D.6000106@redhat.com> In-Reply-To: <55FC2B8D.6000106@redhat.com> Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-IsSubscribed: yes X-SW-Source: 2015-10/txt/msg00921.txt.bz2 Hi Bernd, Thanks for the comments, much appreciated. Comments inlined and a reworked patch attached. > On 09/17/2015 04:38 PM, Robert Suchanek wrote: > > We came across a situation for MIPS64 where moves for sign-extension we= re > > not converted into a nop because of IRA spilled some of the allocnos and > > assigned different hard register for the output operand in the move. > > LRA is not fixing this up as most likely the move was not introduced by > > the LRA itself. I found it hard to fix this in LRA and looked at > > an alternative solution where regrename pass appeared to be the best > candidate. >=20 > For reference, please post examples of the insn pattern(s) where you > would hope to get an improvement. Do they use matching constraints > between the input and output operands in at least one alternative? It all started because of 'extendmn2' SPN on MIPS64 where IRA broke the tie= =20 between input and output registers causing the move to stay around. Genera= lly, I was expecting some of the moves to disappear. Initially I thought that the approach would benefit for matching constraints but whilst reworki= ng and more detailed analysis I couldn't find a case.=20 > So this does look like something that could be addressed in regrename, > but I think the patch is not quite the way to do it. >=20 > > +/* Return a preferred rename register for HEAD. */ >=20 > Function comments ideally ought to be a little more detailed. Preferred > how and why? Noted. Will provide better description in the future. >=20 > > +static int > > +find_preferred_rename_reg (du_head_p head) > > +{ > > + struct du_chain *this_du; > > + int preferred_reg =3D -1; > > + > > + for (this_du =3D head->first; this_du; this_du =3D this_du->next_use) >=20 > This loop seems to search for the insn where the chain terminates (i.e. > the register dies). It seems strange to do this here rather than during > the initial scan in record_out_operands where we visit every insn and > already look for REG_DEAD notes. At the time, I thought it seemed to ok to do this separately.=20 >=20 > > + rtx note; > > + insn_rr_info *p; > > + > > + /* The preferred rename register is an output register iff an in= put > > + register dies in an instruction but the candidate must be validated = by > > + check_new_reg_p. */ > > + for (note =3D REG_NOTES (this_du->insn); note; note =3D XEXP (no= te, 1)) > > + if (insn_rr.exists() > > + && REG_NOTE_KIND (note) =3D=3D REG_DEAD > > + && REGNO (XEXP (note, 0)) =3D=3D head->regno > > + && (p =3D &insn_rr[INSN_UID (this_du->insn)]) > > + && p->op_info) > > + { > > + int i; > > + for (i =3D 0; i < p->op_info->n_chains; i++) > > + { > > + struct du_head *next_head =3D p->op_info->heads[i]; > > + if (head !=3D next_head) >=20 > Here you're not actually verifying the chosen preferred reg is an > output? Is the use of plain "p->op_info" (which is actually an array) > intentional as a guess that operand 0 is the output? I'm not thrilled > with this, and at the very least it should be "p->op_info[0]." to avoid > reader confusion. > It's also not verifying that this is indeed a case where choosing a > preferred reg has a beneficial effect at all. I realized that the check done in find_rename_reg should have been moved he= re as, indeed, it is rather confusing. This is more like finding a candidate rather than validating it. AFAICS, "p->op_info" is a pointer to struct operand_rr_info and it can be n= ull if a chain is opened but not used in a BB. It appears this happens if a register is live across BB but not used in the currently processed one. >=20 > The use of insn_rr would probably also be unnecessary if this was done > during the scan phase. >=20 > > + preferred_reg =3D next_head->regno; >=20 > The problem here is that there's an ordering issue. What if next_head > gets renamed afterwards? The choice of preferred register hasn't bought > us anything in that case. >=20 > For all these reasons I'd suggest a different approach, looking for such > situations during the scan. Try to detect a situation where > * we have a REG_DEAD note for an existing chain > * the insn fulfils certain conditions (i.e. it's a move, or maybe one > of the alternatives has a matching constraint). After all, there's > not much point in tying if the reg that dies was used in a memory > address. > * a new chain is started for a single output > Then, instead of picking a best register, mark the two chains as tied. > Then, when choosing a rename register, see if a tied chain already was > renamed, and try to pick the same register first. I had something similar in mind at beginning but I didn't see how this woul= d fit into the scan. I think I have now better understanding of the regrename pass, th= us, the attached patch is likely the way go to. I limited the conditions to moves only as I didn't see any tying for matching constraints. >=20 > > @@ -1826,7 +1900,7 @@ regrename_optimize (void) > > df_analyze (); > > df_set_flags (DF_DEFER_INSN_RESCAN); > > > > - regrename_init (false); > > + regrename_init (true); >=20 > It would be good to avoid this as it makes the renamer more expensive. I > expect that if you follow the strategy described above, this won't be > necessary. >=20 > > - struct du_chain *chains[MAX_REGS_PER_ADDRESS]; > > - struct du_head *heads[MAX_REGS_PER_ADDRESS]; > > + vec chains; > > + vec heads; >=20 > Given that MAX_REGS_PER_ADDRESS tends to be 1 or 2 this appears to make > things more heavyweight, especially with the extra loop needed to free > the vecs. If possible, try to avoid this. (Again, AFAICS this > information shouldn't really be necessary for what you're trying to do). It happens that MIPS defines the macro to 1 and one of the DSP tests in Dej= agnu failed. I wasn't sure if the macro used here is the correct one or rather it should be relying on NREGS but this cannot be determined statically. Alternatively, a new macro like MAX_REGS_PER_OPERAND would probably better but now this change is redundant. >=20 >=20 > Bernd > > I'll let Bernd comment on the patch itself. But I would say that if > > you're setting up cases where we can tie the source/dest of an extension > > together, then it's a good thing. It'll cause more of them to turn into > > NOPs and it'll make the redundant extension elimination pass more > > effective as well. > > Not if you do it in regrename though, as it's run very late in the pipeli= ne. > If you want to make REE more effective, this would need to be done during= RA. > > --=20 > Eric Botcazou My immediate thought on this is to add a target hook and enable/disable this on a port basis if this hurts performance. I tried to fix this in IRA using a similar technique as in this patch but couldn't find the right place to apply this. I'll come back to this if I have some more time.=20 Regards, Robert gcc/ * regrename.c (create_new_chain): Initialize terminated_dead, renamed and tied_chain. (find_best_rename_reg): Pick and check register from the tied chain. (regrename_do_replace): Mark head as renamed. (scan_rtx_reg): Tie chains in move insns. Set terminate_dead flag. * regrename.h (struct du_head): Add tied_chain, renamed and terminated_dead members. --- gcc/regrename.c | 48 +++++++++++++++++++++++++++++++++++++++++++++++- gcc/regrename.h | 6 ++++++ 2 files changed, 53 insertions(+), 1 deletion(-) diff --git a/gcc/regrename.c b/gcc/regrename.c index 6517f4e..7356a20 100644 --- a/gcc/regrename.c +++ b/gcc/regrename.c @@ -230,6 +230,9 @@ create_new_chain (unsigned this_regno, unsigned this_nr= egs, rtx *loc, head->nregs =3D this_nregs; head->need_caller_save_reg =3D 0; head->cannot_rename =3D 0; + head->terminated_dead =3D 0; + head->renamed =3D 0; + head->tied_chain =3D NULL; =20 id_to_chain.safe_push (head); head->id =3D current_id++; @@ -373,6 +376,13 @@ find_best_rename_reg (du_head_p this_head, enum reg_cl= ass super_class, preferred_class =3D (enum reg_class) targetm.preferred_rename_class (super_class); =20 + /* Pick and check the register from the tied chain iff the tied chain + is not renamed. */ + if (this_head->tied_chain && !this_head->tied_chain->renamed + && check_new_reg_p (old_reg, this_head->tied_chain->regno, + this_head, *unavailable)) + return this_head->tied_chain->regno; + /* If PREFERRED_CLASS is not NO_REGS, we iterate in the first pass over registers that belong to PREFERRED_CLASS and try to find the best register within the class. If that failed, we iterate in @@ -952,6 +962,7 @@ regrename_do_replace (struct du_head *head, int reg) } =20 mode =3D GET_MODE (*head->first->loc); + head->renamed =3D 1; head->regno =3D reg; head->nregs =3D hard_regno_nregs[reg][mode]; } @@ -1035,7 +1046,40 @@ scan_rtx_reg (rtx insn, rtx *loc, enum reg_class cl,= enum scan_actions action, if (action =3D=3D mark_write) { if (type =3D=3D OP_OUT) - create_new_chain (this_regno, this_nregs, loc, insn, cl); + { + int i; + du_head_p c; + du_head_p head; + rtx pat =3D PATTERN (insn); + + c =3D create_new_chain (this_regno, this_nregs, loc, insn, cl); + + /* We try to tie chains in a move instruction for + a single output. */ + if (recog_data.n_operands =3D=3D 2 + && GET_CODE (pat) =3D=3D SET + && GET_CODE (SET_DEST (pat)) =3D=3D REG + && GET_CODE (SET_SRC (pat)) =3D=3D REG) + { + /* Find the input chain. */ + for (i =3D c->id - 1; id_to_chain.iterate (i, &head); i--) + if (head->last && head->last->insn =3D=3D insn + && head->terminated_dead) + { + gcc_assert (head->regno =3D=3D REGNO (recog_data.operand[1])); + c->tied_chain =3D head; + head->tied_chain =3D c; + + if (dump_file) + fprintf (dump_file, "Tying chain %s (%d) with %s (%d)\n", + reg_names[c->regno], c->id, + reg_names[head->regno], head->id); + /* Once tied, we're done. */ + break; + } + } + } + return; } =20 @@ -1143,6 +1187,8 @@ scan_rtx_reg (rtx insn, rtx *loc, enum reg_class cl, = enum scan_actions action, SET_HARD_REG_BIT (live_hard_regs, head->regno + nregs); } =20 + if (action =3D=3D terminate_dead) + (*p)->terminated_dead =3D 1; *p =3D next; if (dump_file) fprintf (dump_file, diff --git a/gcc/regrename.h b/gcc/regrename.h index 9a611f0..a61c5bd 100644 --- a/gcc/regrename.h +++ b/gcc/regrename.h @@ -28,6 +28,8 @@ struct du_head struct du_head *next_chain; /* The first and last elements of this chain. */ struct du_chain *first, *last; + /* The chain that this chain is tied to. */ + struct du_head *tied_chain; /* Describes the register being tracked. */ unsigned regno; int nregs; @@ -45,6 +47,10 @@ struct du_head such as the SET_DEST of a CALL_INSN or an asm operand that used to be a hard register. */ unsigned int cannot_rename:1; + /* Nonzero if the chain is renamed. */ + unsigned int renamed:1; + /* Nonzero if the chain is marked as dead. */ + unsigned int terminated_dead:1; }; =20 typedef struct du_head *du_head_p; --=20 2.4.5