Re: Use predicates for RTL objects
On Thu, Aug 08, 2019 at 10:42:06AM -0600, Jeff Law wrote: > On 8/5/19 12:29 PM, Segher Boessenkool wrote: > > On Mon, Aug 05, 2019 at 02:14:50PM -0400, Arvind Sankar wrote: > >> On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: > >>> First: do you have a copyright assignment? See > >>> https://gcc.gnu.org/contribute.html > >>> for instructions. > >> > >> Nope, is this really substantial enough to warrant one? > > > > Some might say it is, the sheer amount of code changed :-) If some > > maintainer okays it without assignment, that is fine with me of course. > > > > This is also easier if it is all machine-generated and nice simple > > patches :-) > If it's entirely machine generated and you don't want to do a copyright > assignment, then the way to go will be for someone with an assignment to > run the scripts and commit the change. Then we'd only need an > assignment if we wanted the scripts in the repo. > I have just got the forms from GNU, probably a few days to actually send it in. I am not too fussed about how to do this particular change, whatever's easiest. > > > > > -/* Predicate yielding true iff X is an rtx for a double-int. */ > +/* Predicate yielding true iff X is an rtx for a floating point > constant. */ > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > >>> > >>> Is const_double really only used for floating point these days? > > > >> Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code > >> now? That's beyond my current level of understanding of the code, > >> hopefully someone else will chime in. > > > > I am asking if the change to this comment is correct. > I thought we used CONST_DOUBLE for scalar constants that are wider than > a HOST_WIDE_INT, or perhaps wider than 2 HOST_WIDE_INTs. If that's > still the case, then the comment change is wrong. > > Jeff Please note that this is the comment for CONST_DOUBLE_AS_FLOAT_P, not CONST_DOUBLE_P. CONST_DOUBLE_AS_INT_P is the predicate to represent a CONST_DOUBLE being used to store large integers, and, as I understand it, it will be used if the target does not define TARGET_SUPPORTS_WIDE_INT. At present, the comment for both CONST_DOUBLE_AS_INT_P and CONST_DOUBLE_AS_FLOAT_P claim that they are the predicates for double-int.
Re: Use predicates for RTL objects
On Thu, Aug 08, 2019 at 10:42:06AM -0600, Jeff Law wrote: > On 8/5/19 12:29 PM, Segher Boessenkool wrote: > > On Mon, Aug 05, 2019 at 02:14:50PM -0400, Arvind Sankar wrote: > >> On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: > >>> First: do you have a copyright assignment? See > >>> https://gcc.gnu.org/contribute.html > >>> for instructions. > >> > >> Nope, is this really substantial enough to warrant one? > > > > Some might say it is, the sheer amount of code changed :-) If some > > maintainer okays it without assignment, that is fine with me of course. > > > > This is also easier if it is all machine-generated and nice simple > > patches :-) > If it's entirely machine generated and you don't want to do a copyright > assignment, then the way to go will be for someone with an assignment to > run the scripts and commit the change. Then we'd only need an > assignment if we wanted the scripts in the repo. (/me thinks someone set him up to volunteer for this... Well I guess that was me who did that? Oh well.) Yes, I'll do that, and I'll rewrite the script so it is maintainable (as far as that is possible with REs :-) ). That leaves the new predicates... > -/* Predicate yielding true iff X is an rtx for a double-int. */ > +/* Predicate yielding true iff X is an rtx for a floating point > constant. */ > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > >>> > >>> Is const_double really only used for floating point these days? > > > >> Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code > >> now? That's beyond my current level of understanding of the code, > >> hopefully someone else will chime in. > > > > I am asking if the change to this comment is correct. > I thought we used CONST_DOUBLE for scalar constants that are wider than > a HOST_WIDE_INT, or perhaps wider than 2 HOST_WIDE_INTs. If that's > still the case, then the comment change is wrong. Some lines above there is /* Predicate yielding true iff X is an rtx for a double-int. */ #define CONST_DOUBLE_AS_INT_P(X) \ (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) == VOIDmode) so the change is actually good? But these macros should be moved together. Segher
Re: Use predicates for RTL objects
On Thu, Aug 08, 2019 at 10:42:06AM -0600, Jeff Law wrote: > -/* Predicate yielding true iff X is an rtx for a double-int. */ > +/* Predicate yielding true iff X is an rtx for a floating point > constant. */ > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > >>> > >>> Is const_double really only used for floating point these days? No. But it is used for integers only if it has VOIDmode, so I think the above comment change is fine. CONST_DOUBLE isn't used for integers on targets that define TARGET_SUPPORTS_WIDE_INT, but that is a minority of targets (though, the most common ones). Jakub
Re: Use predicates for RTL objects
On 8/5/19 12:29 PM, Segher Boessenkool wrote: > On Mon, Aug 05, 2019 at 02:14:50PM -0400, Arvind Sankar wrote: >> On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: >>> First: do you have a copyright assignment? See >>> https://gcc.gnu.org/contribute.html >>> for instructions. >> >> Nope, is this really substantial enough to warrant one? > > Some might say it is, the sheer amount of code changed :-) If some > maintainer okays it without assignment, that is fine with me of course. > > This is also easier if it is all machine-generated and nice simple > patches :-) If it's entirely machine generated and you don't want to do a copyright assignment, then the way to go will be for someone with an assignment to run the scripts and commit the change. Then we'd only need an assignment if we wanted the scripts in the repo. > -/* Predicate yielding true iff X is an rtx for a double-int. */ +/* Predicate yielding true iff X is an rtx for a floating point constant. */ #define CONST_DOUBLE_AS_FLOAT_P(X) \ (GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) >>> >>> Is const_double really only used for floating point these days? > >> Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code >> now? That's beyond my current level of understanding of the code, >> hopefully someone else will chime in. > > I am asking if the change to this comment is correct. I thought we used CONST_DOUBLE for scalar constants that are wider than a HOST_WIDE_INT, or perhaps wider than 2 HOST_WIDE_INTs. If that's still the case, then the comment change is wrong. Jeff
Re: Use predicates for RTL objects
Here's the patches split up. The ones that say autogenerated were
generated from the script below.
I haven't included that as a patch yet since not sure about the
copyright/licensing boilerplate to insert in it.
contrib/rtl-pred.sh:
#!/bin/sh
#
codes="CONST_INT|CONST_WIDE_INT|CONST_FIXED|CONST_DOUBLE|CONST_VECTOR|CONST_STRING|REG|SUBREG|MEM|LABEL_REF|SYMBOL_REF|DEBUG_INSN"
if test $# != 1 -a $# != 2; then
echo "Usage: $0 []" >&2
exit 1
fi
rtx_code="$1"
rtx_pred="$2"
if [ "x${rtx_pred}" == "x" ]; then
rtx_pred=${rtx_code}
fi
find . -path ./testsuite -prune -o -type f -regex '.*\.\(c\|cc\|h\|md\)' \!
-path ./rtl.h -print | xargs \
perl -pi -e 's/\bGET_CODE[ ]?(\((?:(?>[^()]+)|(?1))*\)) (?|(!)=|==)
('${rtx_code}')\b/$2'${rtx_pred}'_P $1/g'
Re: Use predicates for RTL objects
On Mon, Aug 05, 2019 at 01:29:26PM -0500, Segher Boessenkool wrote: > On Mon, Aug 05, 2019 at 02:14:50PM -0400, Arvind Sankar wrote: > > On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: > > > > -/* Predicate yielding true iff X is an rtx for a double-int. */ > > > > +/* Predicate yielding true iff X is an rtx for a floating point > > > > constant. */ > > > > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > > > >(GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > > > > > > Is const_double really only used for floating point these days? > > > Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code > > now? That's beyond my current level of understanding of the code, > > hopefully someone else will chime in. > > I am asking if the change to this comment is correct. > Ah. The comment was originally copy-pasted from the one for CONST_DOUBLE_AS_INT_P, and it seemed clearly wrong. The comment for CONST_DOUBLE_P (which only checks the code and not the machine mode) says it can either be a double-int or a floating point constant.
Re: Use predicates for RTL objects
On Mon, Aug 05, 2019 at 02:14:50PM -0400, Arvind Sankar wrote: > On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: > > First: do you have a copyright assignment? See > > https://gcc.gnu.org/contribute.html > > for instructions. > > Nope, is this really substantial enough to warrant one? Some might say it is, the sheer amount of code changed :-) If some maintainer okays it without assignment, that is fine with me of course. This is also easier if it is all machine-generated and nice simple patches :-) > > This is easier to review, and even to commit as obvious, if you did a > > patch per macro, certainly for the new macros; and, put the script in > > contrib/, and then say with every patch that is just the output of the > > script that that is the case. > > Ok, I can split it up that way. Thanks! > > Please mention the exact macros you now use, and/or the actual rtx codes. > That'll be easier once its split up by macro. I'll combine the backend + > target code into one patch per macro. Looking forward to it. In general, writing changelogs for simpler patches is easier :-) > > > * gcc/config/rx/constraints.md, gcc/config/rx/rx.c, gcc/config/rx/rx.h: > > > Likewise. > > > > And you normally have a separate entry for every file. > > I tried to make the changelog a bit shorter by combining filenames into > the same line-- should be easier all around to generate that with one entry > per file. Yeah. > > > -/* Predicate yielding true iff X is an rtx for a double-int. */ > > > +/* Predicate yielding true iff X is an rtx for a floating point > > > constant. */ > > > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > > >(GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > > > > Is const_double really only used for floating point these days? > Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code > now? That's beyond my current level of understanding of the code, > hopefully someone else will chime in. I am asking if the change to this comment is correct. Thanks, Segher
Re: Use predicates for RTL objects
On Mon, Aug 05, 2019 at 12:48:46PM -0500, Segher Boessenkool wrote: > Hi Arvind, > > First: do you have a copyright assignment? See > https://gcc.gnu.org/contribute.html > for instructions. Nope, is this really substantial enough to warrant one? > > This is easier to review, and even to commit as obvious, if you did a > patch per macro, certainly for the new macros; and, put the script in > contrib/, and then say with every patch that is just the output of the > script that that is the case. Ok, I can split it up that way. > > Some (mostly changelog) comments: > > On Mon, Aug 05, 2019 at 01:09:10PM -0400, Arvind Sankar wrote: > > * gcc/alias.c, gcc/asan.c, gcc/bb-reorder.c, gcc/bt-load.c: Use > > predicate macros for rtx_code comparisons. > > Many of your changelog lines are much too long. Don't use more than 80 > columns (including the leading tab, which is 8 columns). > > Please mention the exact macros you now use, and/or the actual rtx codes. That'll be easier once its split up by macro. I'll combine the backend + target code into one patch per macro. > > Filenames are relative to the directory containing the changelog file > itself, so you shouldn't have the gcc/ in those filenames. > > > * gcc/config/microblaze/predicates.md, > > There shouldn't be trailing spaces. > > > * gcc/config/rx/constraints.md, gcc/config/rx/rx.c, gcc/config/rx/rx.h: > > Likewise. > > And you normally have a separate entry for every file. > I tried to make the changelog a bit shorter by combining filenames into the same line-- should be easier all around to generate that with one entry per file. > > -/* Predicate yielding true iff X is an rtx for a double-int. */ > > +/* Predicate yielding true iff X is an rtx for a floating point constant. > > */ > > #define CONST_DOUBLE_AS_FLOAT_P(X) \ > >(GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) > > Is const_double really only used for floating point these days? > > > Segher Are you asking if the CONST_DOUBLE_AS_INT_P possibility is dead code now? That's beyond my current level of understanding of the code, hopefully someone else will chime in.
Re: Use predicates for RTL objects
Hi Arvind, First: do you have a copyright assignment? See https://gcc.gnu.org/contribute.html for instructions. This is easier to review, and even to commit as obvious, if you did a patch per macro, certainly for the new macros; and, put the script in contrib/, and then say with every patch that is just the output of the script that that is the case. Some (mostly changelog) comments: On Mon, Aug 05, 2019 at 01:09:10PM -0400, Arvind Sankar wrote: > * gcc/alias.c, gcc/asan.c, gcc/bb-reorder.c, gcc/bt-load.c: Use > predicate macros for rtx_code comparisons. Many of your changelog lines are much too long. Don't use more than 80 columns (including the leading tab, which is 8 columns). Please mention the exact macros you now use, and/or the actual rtx codes. Filenames are relative to the directory containing the changelog file itself, so you shouldn't have the gcc/ in those filenames. > * gcc/config/microblaze/predicates.md, There shouldn't be trailing spaces. > * gcc/config/rx/constraints.md, gcc/config/rx/rx.c, gcc/config/rx/rx.h: > Likewise. And you normally have a separate entry for every file. > -/* Predicate yielding true iff X is an rtx for a double-int. */ > +/* Predicate yielding true iff X is an rtx for a floating point constant. */ > #define CONST_DOUBLE_AS_FLOAT_P(X) \ >(GET_CODE (X) == CONST_DOUBLE && GET_MODE (X) != VOIDmode) Is const_double really only used for floating point these days? Segher
Re: Use predicates for RTL objects
On Fri, Aug 02, 2019 at 01:55:04PM -0400, Arvind Sankar wrote: > On Fri, Aug 02, 2019 at 12:49:56PM -0500, Segher Boessenkool wrote: > > On Fri, Aug 02, 2019 at 01:38:21PM -0400, Arvind Sankar wrote: > > > Hi, I have taken a crack at the beginner GCC project mentioned at > > > https://gcc.gnu.org/projects/beginner.html to replace uses of GET_CODE > > > to check rtx_code with the appropriate predicate macros. > > > > > > Would someone be able to review/actually apply the changes if they look > > > acceptable? > > > > > > Most of the change is auto-generated using the enclosed script [1]. In > > > addition I have added 3 new predicates to rtl.h: CONST_VECTOR_P, > > > CONST_STRING_P and CONST_P. After the autogenned patch there is a small > > > cleanup for a couple instances where the existing comparison is split > > > across source lines and wasn't picked up by the script. > > > > Thank you for doing this! > > > > I don't think there should be a CONST_P like this, the name suggests > > too much that the macro returns true for constants, which isn't what it > > does (not in either direction; neither more or less than it). > > > > It's worse than SET_P or PLUS_P would be even, imo. > > Yes, I agree CONST_P is a little confusing. There are about 60 > occurences of GET_CODE (..) being compared to CONST in generic code. > Should I just leave it out of the conversion That's shat I would do. > or perhaps rename it to > RTL_CONST_P? Though none of the other macros is namespaced, unfortunately. But please get other people's opinion. Like, people who can actually approve your patch in the first place ;-) Segher
Re: Use predicates for RTL objects
On Fri, Aug 02, 2019 at 12:49:56PM -0500, Segher Boessenkool wrote: > On Fri, Aug 02, 2019 at 01:38:21PM -0400, Arvind Sankar wrote: > > Hi, I have taken a crack at the beginner GCC project mentioned at > > https://gcc.gnu.org/projects/beginner.html to replace uses of GET_CODE > > to check rtx_code with the appropriate predicate macros. > > > > Would someone be able to review/actually apply the changes if they look > > acceptable? > > > > Most of the change is auto-generated using the enclosed script [1]. In > > addition I have added 3 new predicates to rtl.h: CONST_VECTOR_P, > > CONST_STRING_P and CONST_P. After the autogenned patch there is a small > > cleanup for a couple instances where the existing comparison is split > > across source lines and wasn't picked up by the script. > > Thank you for doing this! > > I don't think there should be a CONST_P like this, the name suggests > too much that the macro returns true for constants, which isn't what it > does (not in either direction; neither more or less than it). > > It's worse than SET_P or PLUS_P would be even, imo. > > Maybe you can put a script like this in contrib/ ? > > > Segher Yes, I agree CONST_P is a little confusing. There are about 60 occurences of GET_CODE (..) being compared to CONST in generic code. Should I just leave it out of the conversion or perhaps rename it to RTL_CONST_P? Though none of the other macros is namespaced, unfortunately. Thanks.
Re: Use predicates for RTL objects
On Fri, Aug 02, 2019 at 01:38:21PM -0400, Arvind Sankar wrote: > Hi, I have taken a crack at the beginner GCC project mentioned at > https://gcc.gnu.org/projects/beginner.html to replace uses of GET_CODE > to check rtx_code with the appropriate predicate macros. > > Would someone be able to review/actually apply the changes if they look > acceptable? > > Most of the change is auto-generated using the enclosed script [1]. In > addition I have added 3 new predicates to rtl.h: CONST_VECTOR_P, > CONST_STRING_P and CONST_P. After the autogenned patch there is a small > cleanup for a couple instances where the existing comparison is split > across source lines and wasn't picked up by the script. Thank you for doing this! I don't think there should be a CONST_P like this, the name suggests too much that the macro returns true for constants, which isn't what it does (not in either direction; neither more or less than it). It's worse than SET_P or PLUS_P would be even, imo. Maybe you can put a script like this in contrib/ ? Segher
