On 27/05/14 17:31, Richard Sandiford wrote:
> 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.
>
LGTM.
R.
> 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.
>
