On Wed, Aug 07, 2024 at 03:17:24PM +0100, Richard Sandiford wrote:
> Looks like a nice speed-up thanks.
>
> A couple of general points:
>
> * Could you try using the more type-safe hash-table.h, instead of hashtab.h?
> Similarly inchash.h for the hashing.
>
> * Although this wasn't always the style in older code, the preference now
> is to put new functions before their first use where possible, to avoid
> forward declarations.
>
> A couple of very minor comments below.
>
Thanks, I will submit a new patch to address these issues.
However, there is one more question.
> > @@ -532,6 +540,13 @@ compare_operands (struct operand_data *d0, struct
> > operand_data *d1)
> > {
> > const char *p0, *p1;
> >
> > + /* On one hand, comparing strings for predicate and constraint
> > + is time-consuming, and on the other hand, the probability of
> > + different modes is relatively high. Therefore, checking the mode
> > + first can speed up the execution of the program. */
> > + if (d0->mode != d1->mode)
> > + return 0;
> > +
> > p0 = d0->predicate;
> > if (!p0)
> > p0 = "";
> > @@ -550,9 +565,6 @@ compare_operands (struct operand_data *d0, struct
> > operand_data *d1)
> > if (strcmp (p0, p1) != 0)
> > return 0;
> >
> > - if (d0->mode != d1->mode)
> > - return 0;
> > -
> > if (d0->strict_low != d1->strict_low)
> > return 0;
> >
> > @@ -577,9 +589,9 @@ place_operands (class data *d)
> > return;
> > }
> >
> > + od = lookup_operand_data (&d->operand[0]);
> > /* Brute force substring search. */
> > - for (od = odata, i = 0; od; od = od->next, i = 0)
> > - if (compare_operands (od, &d->operand[0]))
> > + for (i = 0; od; od = od->eq_next, i = 0)
>
> I think we should move the i = 0 to after the loop, for the "no match" case.
> As it stands, each iteration immediate sets i to 1.
>
As the method for finding matching operands changes to hash table lookup,
a "no match" scenario will correspond to 'lookup_operand_data (&d->operand[0])'
returning NULL and assigning it to 'od'. This means that 'i' will remain 0
and indicate "no match" in the subsequent code.
Therefore, I think we still need to iterate starting from 'i = 0',
but reassigning i to 0 on each iteration is redundant.
We can modify it to
'for (i = 0; od; od = od->eq_next)'.
BR,
Xianmiao
