[AARCH64] [PATCH 3/3] AArch64 Port

[Tejas Belagod]

Hi Richard,

Thanks for your comments. Some questions inline below.

Richard Sandiford wrote:
> Marcus Shawcroft <marcus.shawcroft@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
>