Tejas Belagod <tbela...@arm.com> writes:
> 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.
Probably because of a typo, sorry. I meant "int *" in the above.
At least, they'd be "int *" for int iterators and "rtx" for mode
and code 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?
If you don't convert codes and modes (and leave it to me), then you'd
probably need to apply all int interators first. I expect it'd be easier
to convert modes and codes at the same time.
Richard
