RFA: cache enabled attribute by insn code

Richard Earnshaw <rearnsha@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.

On 27/05/14 17:09, Richard Sandiford wrote:
> Richard Earnshaw <rearnsha@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).

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.
>