Re: RFA: patch to fix a bad code generation for PR64110 -- new constraints addition

[Richard Sandiford <richard dot sandiford at arm dot com>]

Vladimir Makarov <vmakarov@redhat.com> writes:
> On 01/27/2015 09:08 AM, Richard Sandiford wrote:
>> Yeah, but in practice that's only ever going to be a partial transition.
>> Many port maintainers won't look at this, so we'll have to support both
>> versions indefinitely, even if the new behaviour turns out to be the
>> best for all cases.
>>
>> I just think we're going to regret having two sets of constraints with
>> such subtly different meanings.
>>
>> Looking back at the original PR, Jakub said:
>>
>>   The ! has been added by me for PR63594, so it isn't there from the era
>>   when i?86 backend was using reload.  If there is a better way to
>>   express that RA should prefer to use memory or xmm register and only
>>   use r constraint if it already is in a r register and doesn't need to
>>   be reloaded, I can use that.  Whether it is ?, ??? or something else.
>>   ! description in gcc docs just fitted most what I wanted...
>>
>> In some ways this seems to match the intention of "*".  Originally I think
>> it was just an RA-only thing and was ignored by reload, but LRA does take it
>> into account too (which sounds like progress to me).
>   I guess we don't need '*' in many cases.  It is overused.  Imho, IRA
> should decide what class is better based on costs of alternatives and
> the explicit exclusion of register class by using '*' is a bad practice.
>
>   Saying that I believe we should do register class preferrencing
> algorithm more alternative oriented.  The algorithm should choose first
> an alternative (of may be subset of alternatives) and then register
> classes.  I think it is more logical.  It would permits us to rid off
> all such constraints including '*' and use only one like '?' which
> increases the alternative cost.
>
>   In perspective it is even better to rid of '?' too and have some hook
> (or attribute) to get insn alternative costs which can be depended on
> sub-target or other run-time characteristics.  Otherwise we need to
> duplicate insn descriptions and put different insn guards.  I am going
> to work on this.  But it is hard to say will it work well (may be I have
> some performance issues with this).  This hook somehow (min or average
> of the values for all alternatives) can be used in combiner and other
> algorithms need an insn cost. That is how I see the solution of the
> problem in a long perspective.

Definitely agree that it'd be better to remove these constraints
in favour of a new attribute.  preferred_for_size and preferred_for_speed
give something similar, though they're much more stringent than what
we need here.

>> If I revert the patch locally and change the *vec_dup<mode> pattern to
>> use "*", it passes both the test for PR64110 and the tests for PR63594.
>> Would that be OK as an alternative?
>>
>   I don't think it will work in general case.  It probably works because
> a different class is chosen in IRA.  If IRA for some reasons choose the
> same class, we might see the same problem in LRA.

But isn't that the point of '*'?  It should stop IRA from using the 'r'
alternative as an indication that 'r' is a good choice for this instruction.
If IRA chooses 'r' anyway, it must be because other instructions that
use the same allocno strongly prefer 'r'.

And in those some circumstances -- i.e. if IRA does choose 'r' despite
the constraints in this instruction -- then I think we do want to use the
'r' alternative.  And AIUI that's also what the new constraint is designed
to do.  If IRA chooses 'r' anyway, the new constraint causes LRA to prefer
the 'r' alternative _even if_ another operand (the destination) has to
be reloaded, which is the fundamental difference between the new constraint
and '!'.

So I'm still not sure why '*' wouldn't do what we want.

>  I also don't like when register classes are excluded by '*' for IRA
> (see my thoughts above).

Understood, and I agree it would be good to move to attributes.
But in a way, I think that's an even better reason to try to avoid
adding these new constraints.  It sounds like we're hoping to get rid
of them as soon as we've added them :-)

Thanks,
Richard