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