Bernd Schmidt <bschm...@redhat.com> writes:
> On 11/25/2015 01:26 PM, Richard Sandiford wrote:
>> Later patches in the series add a new form of attribute that takes the
>> attribute number as an argument, rather than it being stored in the
>> global which_alternative variable.
>>
>> Having both a local alternative number and a global alternative number
>> is likely to cause confusion. This patch therefore gets rid of the
>> global variable.
>
> I don't really feel that this is appropriate for this stage, and some of
> the formatting changes are pretty ugly. I'd put this pattern
> constrain_operands (1, get_enabled_alternatives (temp)) >= 0
> into an inline function
> constraints_ok_p (temp).
> and possibly also have a variant with an extract_insn call, similar to
> extract_constrain_insn.
The idea is to force callers to think about whether they're asking:
(1) Can I constrain this instruction to match any enabled alternative?
(2) Can I constrain this instruction to match an alternative that is
good for size?
(3) Can I constrain this instruction to match an alternative that is
good for speed?
Perhaps we could use the optimization_type added in patch 21 for this,
with a new enum value to mean "ignore optimisation".
I'm just worried that if we have a constraints_ok_p that hides the
decision altogether, it will get used by optimisation passes when
testing the result of a transform, whereas really they should be
taking the containing block's size/speed choice into account.
> Do any of the subsequent patches actually depend on this?
I guess not, but without it we have both local and global variables
called which_alternative. The new-style attributes will use a local
which_alternative and the old-style ones will use a global
which_alternative. The global which_alternative will still be visible
to arch.c files, so there's an obvious danger of confusion.
Thanks,
Richard
