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.
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
