Re: RFA: cache enabled attribute by insn code

[Richard Sandiford <rdsandiford at googlemail dot com>]

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