From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32628 invoked by alias); 26 Aug 2011 12:57:50 -0000 Received: (qmail 32614 invoked by uid 22791); 26 Aug 2011 12:57:49 -0000 X-SWARE-Spam-Status: No, hits=-2.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-fx0-f47.google.com (HELO mail-fx0-f47.google.com) (209.85.161.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 26 Aug 2011 12:57:35 +0000 Received: by fxg11 with SMTP id 11so2743740fxg.20 for ; Fri, 26 Aug 2011 05:57:33 -0700 (PDT) Received: by 10.223.7.10 with SMTP id b10mr1608322fab.76.1314363453512; Fri, 26 Aug 2011 05:57:33 -0700 (PDT) Received: from richards-thinkpad.stglab.manchester.uk.ibm.com (gbibp9ph1--blueice3n2.emea.ibm.com [195.212.29.84]) by mx.google.com with ESMTPS id 22sm1304479fat.41.2011.08.26.05.57.31 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 26 Aug 2011 05:57:32 -0700 (PDT) From: Richard Sandiford To: Bernd Schmidt Mail-Followup-To: Bernd Schmidt ,GCC Patches , richard.sandiford@linaro.org Cc: GCC Patches Subject: Re: Rename across basic block boundaries References: <4E00A296.4000306@codesourcery.com> <4E569522.7080008@codesourcery.com> Date: Fri, 26 Aug 2011 13:33:00 -0000 In-Reply-To: <4E569522.7080008@codesourcery.com> (Bernd Schmidt's message of "Thu, 25 Aug 2011 20:32:02 +0200") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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 X-SW-Source: 2011-08/txt/msg02157.txt.bz2 Rather than using global variables and then copying them into a bb structure, would it be possible to write directly into the bb structure? The answer's probably "no", just asking. :-) Bernd Schmidt writes: > * regrename.c (struct du_head): Make nregs signed. > (scan_rtx_reg, scan_rtx_address, dump_def_use_chain): Remove > declarations. This bit was split out. > (mark_conflict, create_new_chain): Move upwards in the file. Same here. Should mention the change to create_new_chain's interface though. > - 2. For each chain, the set of possible renaming registers is computed. > + 2. We try combine the local chains across basic block boundaries by > + comparing chains that were open at the start or end of a block to > + those in successor/predecessor blocks. "try to combine" > +/* Dump all def/use chains, starting at id FROM. */ > > static void > -dump_def_use_chain (struct du_head *head) > +dump_def_use_chain (int from) > { > - while (head) > + du_head_p head; > + int i; > + FOR_EACH_VEC_ELT (du_head_p, id_to_chain, i, head) > { > struct du_chain *this_du = head->first; > + if (i < from) > + continue; > fprintf (dump_file, "Register %s (%d):", > reg_names[head->regno], head->nregs); > while (this_du) I know it's only a dumping function, but maybe this'd be a good excuse to add: #define FOR_EACH_VEC_ELT_FROM(T, V, I, P, FROM) \ for (I = (FROM); VEC_iterate (T, (V), (I), (P)); ++(I)) > +/* A structure recording information about each basic block. It is saved > + and restored around basic block boundaries. */ > +struct bb_rename_info Probably worth saying here or elsewhere that the bb->aux field points to this information and that (more importantly) the bb->aux is null for blocks that could not be optimised, including the exit block. > +/* Record in RI that the block corresponding to it has an incoming > + live value, described by CHAIN. */ > +static void > +set_incoming_from_chain (struct bb_rename_info *ri, du_head_p chain) > +{ > + int min_reg, max_reg, i; > + int incoming_nregs = ri->incoming[chain->regno].nregs; > + int nregs; > + > + /* If we've recorded the same information before, everything is fine. */ > + if (incoming_nregs == chain->nregs) > + { > + if (dump_file) > + fprintf (dump_file, "reg %d/%d already recorded\n", > + chain->regno, chain->nregs); > + return; > + } > + > + /* If we have no information for any of the involved registers, update > + the incoming array. */ > + nregs = chain->nregs; > + while (nregs-- > 0) > + if (ri->incoming[chain->regno + nregs].nregs != 0 > + || ri->incoming[chain->regno + nregs].unusable) > + break; > + if (nregs < 0) > + { > + nregs = chain->nregs; > + ri->incoming[chain->regno].nregs = nregs; > + while (nregs-- > 1) > + ri->incoming[chain->regno + nregs].nregs = -nregs; > + if (dump_file) > + fprintf (dump_file, "recorded reg %d/%d\n", > + chain->regno, chain->nregs); > + return; > + } > + > + /* There must be some kind of conflict. Set the unusable for all > + overlapping registers. */ > + min_reg = chain->regno; > + if (incoming_nregs < 0) > + min_reg += incoming_nregs; > + max_reg = chain->regno + chain->nregs; > + for (i = min_reg; i < max_reg; i++) > + ri->incoming[i].unusable = true; In the incoming_nregs < 0 case, we only need to set ri->incoming[chain->regno + incoming_nregs] itself, right, not the other registers between that and ri->incoming[chain->regno]? If so, I think it'd be clearer to have: /* There must be some kind of conflict. Prevent both the old and new ranges from being used. */ if (incoming_nregs < 0) ri->incoming[chain->regno + incoming_nregs].unusable = true; for (i = 0; i < chain->nregs; i++) ri->incoming[chain->regno + i].unusable = true; When I first looked at the code, I was wondering why we changed every register in (chain->regno + incoming_nregs, chain_regno), but none in [chain->regno + chain->nregs, OLD_END). Seems like we should do neither (as in the above suggestion) or both. > + /* Process all basic blocks by using a worklist, adding unvisited successor > + blocks whenever we reach the end of one basic blocks. This ensures that > + whenever possible, we only process a block after at least one of its > + predecessors, which provides a "seeding" effect to make the logic in > + set_incoming_from_chain and init_rename_info useful. */ Wouldn't a reverse post-order (inverted_post_order_compute) allow even more pre-opening (as well as being less code)? Looked good to me otherwise. Richard