| Message ID | 87zji3yyhk.fsf@sandifor-thinkpad.stglab.manchester.uk.ibm.com |
|---|---|
| State | New |
| Headers | show |
On 27/05/14 17:31, Richard Sandiford wrote: > Richard Earnshaw <rearnsha@arm.com> writes: >> 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). > > 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. >
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.