From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19363 invoked by alias); 27 May 2012 09:49:31 -0000 Received: (qmail 19354 invoked by uid 22791); 27 May 2012 09:49:30 -0000 X-SWARE-Spam-Status: No, hits=-4.1 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_CP X-Spam-Check-By: sourceware.org Received: from mail-wi0-f173.google.com (HELO mail-wi0-f173.google.com) (209.85.212.173) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 27 May 2012 09:49:17 +0000 Received: by wibhj6 with SMTP id hj6so679731wib.8 for ; Sun, 27 May 2012 02:49:16 -0700 (PDT) Received: by 10.216.142.167 with SMTP id i39mr2285310wej.94.1338112155439; Sun, 27 May 2012 02:49:15 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk. [82.133.89.107]) by mx.google.com with ESMTPS id f19sm18968314wiw.11.2012.05.27.02.49.13 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 27 May 2012 02:49:14 -0700 (PDT) From: Richard Sandiford To: Marcus Shawcroft Mail-Followup-To: Marcus Shawcroft ,"gcc-patches\@gcc.gnu.org" , rdsandiford@googlemail.com Cc: "gcc-patches\@gcc.gnu.org" Subject: Re: [AARCH64] [PATCH 3/3] AArch64 Port References: <4FBF67BE.1090301@arm.com> <4FBF6866.1080904@arm.com> <4FBF68C1.5020006@arm.com> <4FBF6AB0.4040007@arm.com> Date: Sun, 27 May 2012 09:49:00 -0000 In-Reply-To: <4FBF6AB0.4040007@arm.com> (Marcus Shawcroft's message of "Fri, 25 May 2012 12:19:12 +0100") Message-ID: <8762bihs2u.fsf@talisman.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (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: 2012-05/txt/msg01823.txt.bz2 Marcus Shawcroft writes: > This patch adds an implementation of integer iterators. Nice. A few comments from an onlooker (on top of what Stephen said). > +/* Since GCC does not construct a table of valid constants, > + we have to accept any int as valid. No cross-checking can > + be done. */ > +static int > +find_int (const char *name) > +{ > + char *endptr; > + int ret; > + > + if (ISDIGIT (*name)) > + { > + ret = strtol (name, &endptr, 0); > + gcc_assert (*endptr == '\0'); I think this should be an error rather than an assert. > +/* Stand-alone int iterator usage-checking function. */ > +static bool > +uses_int_iterator_p (rtx x, struct mapping *iterator, int opno) > +{ > + int i; > + for (i=0; i < num_int_iterator_data; i++) > + if (int_iterator_data[i].iterator->group == iterator->group && > + int_iterator_data[i].iterator->index == iterator->index) Formatting: && should be at the beginning of the second line. > + { > + /* Found an existing entry. Check if X is in its list. */ > + struct int_iterator_mapping it = int_iterator_data[i]; > + int j; > + > + for (j=0; j < it.num_rtx; j++) > + { > + if (it.rtxs[j].x == x && it.rtxs[j].opno == opno) > + return true; > + } Formatting: redundant { ... }. It might be easier to store a pointer to XEXP (x, opno) than storing x and opno separately. > + } > + return false; > +} > + > /* Map a code or mode attribute string P to the underlying string for > ITERATOR and VALUE. */ > > @@ -341,7 +414,9 @@ > x = rtx_alloc (bellwether_code); > memcpy (x, original, RTX_CODE_SIZE (bellwether_code)); > > - /* Change the mode or code itself. */ > + /* Change the mode or code itself. > + For int iterators, apply_iterator () does nothing. This is > + because we want to apply int iterators to operands below. */ The way I imagined this working is that we'd just walk a list of rtx * pointers for the current iterator and substitute the current iterator value. Then we'd take a deep copy of the rtx once all iterators had been applied. Checking every operand against the substitution table seems a bit round-about. It'd be good to do the same for codes and modes, but I'll volunteer to do that as a follow-up. > +/* Add to triplet-database for int iterators. */ > +static void > +add_int_iterator (struct mapping *iterator, rtx x, int opno) > +{ > + > + /* Find iterator in int_iterator_data. If already present, > + add this R to its list of rtxs. If not present, create > + a new entry for INT_ITERATOR_DATA and add the R to its > + rtx list. */ > + int i; > + for (i=0; i < num_int_iterator_data; i++) > + if (int_iterator_data[i].iterator->index == iterator->index) > + { > + /* Found an existing entry. Add rtx to this iterator's list. */ > + int_iterator_data[i].rtxs = > + XRESIZEVEC (struct rtx_list, > + int_iterator_data[i].rtxs, > + int_iterator_data[i].num_rtx + 1); > + int_iterator_data[i].rtxs[int_iterator_data[i].num_rtx].x = x; > + int_iterator_data[i].rtxs[int_iterator_data[i].num_rtx].opno = opno; > + int_iterator_data[i].num_rtx++; > + return; > + } > + > + /* New INT_ITERATOR_DATA entry. */ > + if (num_int_iterator_data == 0) > + int_iterator_data = XNEWVEC (struct int_iterator_mapping, 1); > + else > + int_iterator_data = XRESIZEVEC (struct int_iterator_mapping, > + int_iterator_data, > + num_int_iterator_data + 1); > + int_iterator_data[num_int_iterator_data].iterator = iterator; > + int_iterator_data[num_int_iterator_data].rtxs = XNEWVEC (struct rtx_list, 1); > + int_iterator_data[num_int_iterator_data].rtxs[0].x = x; > + int_iterator_data[num_int_iterator_data].rtxs[0].opno = opno; > + int_iterator_data[num_int_iterator_data].num_rtx = 1; > + num_int_iterator_data++; > +} VECs might be better here. > @@ -1057,14 +1227,30 @@ > XWINT (return_rtx, i) = tmp_wide; > break; > > - case 'i': > case 'n': > - read_name (&name); > validate_const_int (name.string); > tmp_int = atoi (name.string); > XINT (return_rtx, i) = tmp_int; > break; > - > + case 'i': > + /* Can be an iterator or an integer constant. */ > + read_name (&name); > + if (!ISDIGIT (name.string[0])) > + { > + struct mapping *iterator; > + /* An iterator. */ > + iterator = find_int_iterator (&ints, name.string); > + /* Build (iterator, rtx, op) triplet-database. */ > + add_int_iterator (iterator, return_rtx, i); > + } > + else > + { > + /* A numeric constant. */ > + validate_const_int (name.string); > + tmp_int = atoi (name.string); > + XINT (return_rtx, i) = tmp_int; > + } > + break; > default: > gcc_unreachable (); > } I don't see any need to split "i" and "n". In fact we probably shouldn't handle "n" at all, since it's only used in NOTE_INSNs. So I think we should either remove the "n" case or continue to treat it in the same way as "i". Richard