Re: [AARCH64] [PATCH 3/3] AArch64 Port

[Tejas Belagod <tbelagod at arm dot com>]

Richard Sandiford wrote:
> Tejas Belagod <tbelagod@arm.com> writes:
> > 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.
>
> 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.

Thanks for the clarification.

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

In the currect scheme, when multiple code/mode iterators are in an rtx pattern, 
they are expanded for each combination of iterator values in 
apply_iterator_traverse () and a repeated traversal of the expanded rtx's for 
each iterator achieves the 'cross-product' of rtx's for iterator value 
combinations. This allows for having the place-holder codes/modes in new rtx's 
that are shallow-copied and a subsequent traversal will eventually replace them 
with actual code/mode iterator values. Doing away with the placeholders means 
that we have to build a 'cross-product' of iterator values from a database of 
iterator values as used in a pattern and make an rtx copy(deep) corresponding to 
each element of the cross-product with the iterator value combinations 
substituted in. This is my understanding of the new implementation - am I on the 
right track?

Thanks,
Tejas.