On 27/05/14 17:09, Richard Sandiford wrote:
> Richard Earnshaw <[email protected]> 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).
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.
R.
> 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.
>
