Richard Earnshaw <rearn...@arm.com> writes:
> On 27/05/14 17:09, Richard Sandiford wrote:
>> Richard Earnshaw <rearn...@arm.com> writes:
>>> On 27/05/14 16:27, Jakub Jelinek wrote:
>>>> On Tue, May 27, 2014 at 04:15:47PM +0100, Richard Earnshaw wrote:
>>>>> On 27/05/14 15:08, Richard Sandiford wrote:
>>>>>> Hmm, is this because of "insn_enabled"? If so, how did that work before
>>>>>> the patch? LRA already assumed that the "enabled" attribute didn't
>>>>>> depend
>>>>>> on the operands.
>>>>>
>>>>> Huh! "enabled" can be applied to each alternative. Alternatives are
>>>>> selected based on the operands. If LRA can't cope with that we have a
>>>>> serious problem. In fact, a pattern that matches but has no enabled
>>>>> alternatives is meaningless and guaranteed to cause problems during
>>>>> register allocation.
>>>>
>>>> This is not LRA fault, but the backend misusing the "enabled" attribute
>>>> for something it wasn't designed for, and IMHO against the documentation
>>>> of the attribute too.
>>>> Just look at the original submission why it has been added.
>>>>
>>>> Jakub
>>>>
>>>
>>> <quote>
>>> The @code{enabled} insn attribute may be used to disable certain insn
>>> alternatives for machine-specific reasons.
>>> <quote>
>>>
>>> The rest of the text just says what happens when this is done and then
>>> gives an example usage. It doesn't any time, either explicitly or
>>> implicitly, say that this must be a static choice determined once-off at
>>> run time.
>>
>> OK, how about the doc patch below?
>>
>>> That being said, I agree that this particular use case is pushing the
>>> boundaries -- but that's always a risk when the boundaries aren't
>>> clearly defined.
>>>
>>> A better solution here would be to get rid of all those match_operator
>>> patterns and replace them with iterators; but that's a lot of work,
>>> particularly for all the conditinal operation patterns we have. It
>>> would probably also bloat the number of patterns quite alarmingly.
>>
>> Yeah, which is why I was just going for the one place where it mattered.
>> I think match_operator still has a place in cases where the insn pattern
>> is entirely regular, which seems to be the case for most other uses
>> of shiftable_operator. It's just that in this case there really were
>> two separate cases per operator (plus vs. non-plus and mult vs. true shift).
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> * doc/md.texi: Document the restrictions on the "enabled" attribute.
>>
>> Index: gcc/doc/md.texi
>> ===================================================================
>> --- gcc/doc/md.texi (revision 210972)
>> +++ gcc/doc/md.texi (working copy)
>> @@ -4094,11 +4094,11 @@
>> @subsection Disable insn alternatives using the @code{enabled} attribute
>> @cindex enabled
>>
>> -The @code{enabled} insn attribute may be used to disable certain insn
>> -alternatives for machine-specific reasons. This is useful when adding
>> -new instructions to an existing pattern which are only available for
>> -certain cpu architecture levels as specified with the @code{-march=}
>> -option.
>> +The @code{enabled} insn attribute may be used to disable insn
>> +alternatives that are not available for the current subtarget.
>> +This is useful when adding new instructions to an existing pattern
>> +which are only available for certain cpu architecture levels as
>> +specified with the @code{-march=} option.
>>
>> If an insn alternative is disabled, then it will never be used. The
>> compiler treats the constraints for the disabled alternative as
>> @@ -4112,6 +4112,9 @@
>> A definition of the @code{enabled} insn attribute. The attribute is
>> defined as usual using the @code{define_attr} command. This
>> definition should be based on other insn attributes and/or target flags.
>> +It must not depend directly or indirectly on the current operands,
>> +since the attribute is expected to be a static property of the subtarget.
>> +
>
> I'd reverse the two components of that sentence. Something like:
>
> The attribute must be a static property of the subtarget; that is, it
> must not depend on the current operands or any other dynamic context
> (for example, location of the insn within the body of a loop).
OK, how about the attached?
> It would be useful if we could precisely define 'static property'
> somewhere, so that it encompases per-function changing of the ISA or
> optimization variant via __attribute__ annotations. That does need to
> work, since it could be used for switching between ARM and Thumb.
Yeah, the cache depends on the current target for SWITCHABLE_TARGETs,
is invalidated by target_reinit for other targets, and is also invalidated
by changes to the register classes.
Thanks,
Richard
gcc/
* doc/md.texi: Document the restrictions on the "enabled" attribute.
Index: gcc/doc/md.texi
===================================================================
--- gcc/doc/md.texi (revision 210972)
+++ gcc/doc/md.texi (working copy)
@@ -4094,11 +4094,11 @@
@subsection Disable insn alternatives using the @code{enabled} attribute
@cindex enabled
-The @code{enabled} insn attribute may be used to disable certain insn
-alternatives for machine-specific reasons. This is useful when adding
-new instructions to an existing pattern which are only available for
-certain cpu architecture levels as specified with the @code{-march=}
-option.
+The @code{enabled} insn attribute may be used to disable insn
+alternatives that are not available for the current subtarget.
+This is useful when adding new instructions to an existing pattern
+which are only available for certain cpu architecture levels as
+specified with the @code{-march=} option.
If an insn alternative is disabled, then it will never be used. The
compiler treats the constraints for the disabled alternative as
@@ -4112,6 +4112,10 @@
A definition of the @code{enabled} insn attribute. The attribute is
defined as usual using the @code{define_attr} command. This
definition should be based on other insn attributes and/or target flags.
+The attribute must be a static property of the subtarget; that is, it
+must not depend on the current operands or any other dynamic context
+(for example, the location of the insn within the body of a loop).
+
The @code{enabled} attribute is a numeric attribute and should evaluate to
@code{(const_int 1)} for an enabled alternative and to
@code{(const_int 0)} otherwise.
