Xianmiao Qu <cooper...@linux.alibaba.com> writes:
> 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)'.
But what I mean is: the code could just be:
od = lookup_operand_data (&d->operand[0]);
/* Brute force substring search. */
for (; od; od = od->eq_next)
{
i = 1;
while (1)
{
if (i == d->n_operands)
goto full_match;
if (od2 == NULL)
goto partial_match;
if (! compare_operands (od2, &d->operand[i]))
break;
++i, od2 = od2->next;
}
}
i = 0;
/* Either partial match at the end of the list, or no match. In either
case, we tack on what operands are remaining to the end of the list. */
partial_match:
...
full_match:
...
Each partial and full match starts at index 1, because the hash
table guarantees a match for index 0. partial_match is entered
with i == 0 iff this is an entirely new entry.
Thanks,
Richard
>
>
>
> BR,
> Xianmiao
