Hi Richard,
Thanks for your comments. Some questions inline below.
Richard Sandiford wrote:
Marcus Shawcroft <marcus.shawcr...@arm.com> 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.
I understand how this would work for mode and code iterators, but I'm a
bit confused about how it would for int iterators. Don't we have to
traverse each operand to figure out which ones to substitute for an int
iterator value? Also, when you say take a deep copy after all the
iterators have been applied, do you mean code, mode and int iterators or
do you mean values of a particular iterator? As I understand the current
implementation, mode and code iterators use placeholder integral
constants that are replaced with actual iterator values during the rtx
traverse. If we take a deep copy after the replacement, won't we lose
these placeholder codes?
Thanks,
Tejas.
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
