Re: [PATCH 1/6][ARM] Refactor NEON builtin framework to work for other builtins
Hi Andre, On 01/12/16 17:25, Andre Vieira (lists) wrote: On 17/11/16 10:42, Kyrill Tkachov wrote: Hi Andre, On 09/11/16 10:11, Andre Vieira (lists) wrote: Hi, Refactor NEON builtin framework such that it can be used to implement other builtins. Is this OK for trunk? Regards, Andre gcc/ChangeLog 2016-11-09 Andre Vieira* config/arm/arm-builtins.c (neon_builtin_datum): Rename to .. (arm_builtin_datum): ... this. (arm_init_neon_builtin): Rename to ... (arm_init_builtin): ... this. Add a new parameters PREFIX and USE_SIG_IN_NAME. (arm_init_neon_builtins): Replace 'arm_init_neon_builtin' with 'arm_init_builtin'. Replace type 'neon_builtin_datum' with 'arm_builtin_datum'. (arm_init_vfp_builtins): Likewise. (builtin_arg): Rename enum's replacing 'NEON_ARG' with 'ARG_BUILTIN' and add a 'ARG_BUILTIN_NEON_MEMORY. (arm_expand_neon_args): Rename to ... (arm_expand_builtin_args): ... this. Rename builtin_arg enum values and differentiate between ARG_BUILTIN_MEMORY and ARG_BUILTIN_NEON_MEMORY. (arm_expand_neon_builtin_1): Rename to ... (arm_expand_builtin_1): ... this. Rename builtin_arg enum values, arm_expand_builtin_args and add bool parameter NEON. (arm_expand_neon_builtin): Use arm_expand_builtin_1. (arm_expand_vfp_builtin): Likewise. (NEON_MAX_BUILTIN_ARGS): Remove, it was unused. /* Expand a neon builtin. This is also used for vfp builtins, which behave in the same way. These builtins are "special" because they don't have symbolic constants defined per-instruction or per instruction-variant. Instead, the - required info is looked up in the NEON_BUILTIN_DATA record that is passed + required info is looked up in the ARM_BUILTIN_DATA record that is passed into the function. */ The comment should be updated now that it's not just NEON builtins that are expanded through this function. static rtx -arm_expand_neon_builtin_1 (int fcode, tree exp, rtx target, - neon_builtin_datum *d) +arm_expand_builtin_1 (int fcode, tree exp, rtx target, + arm_builtin_datum *d, bool neon) { I'm not a fan of this 'neon' boolean as it can cause confusion among the users of the function (see long thread at https://gcc.gnu.org/ml/gcc/2016-10/msg4.html). Whether the builtin is a NEON/VFP builtin can be distinguished from FCODE, so lets just make that bool neon a local variable and initialise it accordingly from FCODE. Same for: +/* Set up a builtin. It will use information stored in the argument struct D to + derive the builtin's type signature and name. It will append the name in D + to the PREFIX passed and use these to create a builtin declaration that is + then stored in 'arm_builtin_decls' under index FCODE. This FCODE is also + written back to D for future use. If USE_SIG_IN_NAME is true the builtin's + name is appended with type signature information to distinguish between + signedness and poly. */ static void -arm_init_neon_builtin (unsigned int fcode, - neon_builtin_datum *d) +arm_init_builtin (unsigned int fcode, arm_builtin_datum *d, + const char * prefix, bool use_sig_in_name) use_sig_in_name is dependent on FCODE so just deduce it from that locally in arm_init_builtin. This is ok otherwise. Thanks, Kyrill Hi, Reworked patch according to comments. No changes to ChangeLog. Is this OK? Ok. Please wait for the other patches in the series to be approved before committing. Thanks, Kyrill Cheers, Andre
Re: [PATCH 1/6][ARM] Refactor NEON builtin framework to work for other builtins
On 17/11/16 10:42, Kyrill Tkachov wrote: > Hi Andre, > > On 09/11/16 10:11, Andre Vieira (lists) wrote: >> Hi, >> >> Refactor NEON builtin framework such that it can be used to implement >> other builtins. >> >> Is this OK for trunk? >> >> Regards, >> Andre >> >> gcc/ChangeLog >> 2016-11-09 Andre Vieira>> >> * config/arm/arm-builtins.c (neon_builtin_datum): Rename to .. >> (arm_builtin_datum): ... this. >> (arm_init_neon_builtin): Rename to ... >> (arm_init_builtin): ... this. Add a new parameters PREFIX >> and USE_SIG_IN_NAME. >> (arm_init_neon_builtins): Replace 'arm_init_neon_builtin' with >> 'arm_init_builtin'. Replace type 'neon_builtin_datum' with >> 'arm_builtin_datum'. >> (arm_init_vfp_builtins): Likewise. >> (builtin_arg): Rename enum's replacing 'NEON_ARG' with >> 'ARG_BUILTIN' and add a 'ARG_BUILTIN_NEON_MEMORY. >> (arm_expand_neon_args): Rename to ... >> (arm_expand_builtin_args): ... this. Rename builtin_arg >> enum values and differentiate between ARG_BUILTIN_MEMORY >> and ARG_BUILTIN_NEON_MEMORY. >> (arm_expand_neon_builtin_1): Rename to ... >> (arm_expand_builtin_1): ... this. Rename builtin_arg enum >> values, arm_expand_builtin_args and add bool parameter NEON. >> (arm_expand_neon_builtin): Use arm_expand_builtin_1. >> (arm_expand_vfp_builtin): Likewise. >> (NEON_MAX_BUILTIN_ARGS): Remove, it was unused. > > /* Expand a neon builtin. This is also used for vfp builtins, which > behave in > the same way. These builtins are "special" because they don't have > symbolic > constants defined per-instruction or per instruction-variant. > Instead, the > - required info is looked up in the NEON_BUILTIN_DATA record that is > passed > + required info is looked up in the ARM_BUILTIN_DATA record that is > passed > into the function. */ > > > The comment should be updated now that it's not just NEON builtins that > are expanded through this function. > > static rtx > -arm_expand_neon_builtin_1 (int fcode, tree exp, rtx target, > - neon_builtin_datum *d) > +arm_expand_builtin_1 (int fcode, tree exp, rtx target, > + arm_builtin_datum *d, bool neon) > { > > I'm not a fan of this 'neon' boolean as it can cause confusion among the > users of the function > (see long thread at https://gcc.gnu.org/ml/gcc/2016-10/msg4.html). > Whether the builtin is a NEON/VFP builtin > can be distinguished from FCODE, so lets just make that bool neon a > local variable and initialise it accordingly > from FCODE. > > Same for: > +/* Set up a builtin. It will use information stored in the argument > struct D to > + derive the builtin's type signature and name. It will append the > name in D > + to the PREFIX passed and use these to create a builtin declaration > that is > + then stored in 'arm_builtin_decls' under index FCODE. This FCODE is > also > + written back to D for future use. If USE_SIG_IN_NAME is true the > builtin's > + name is appended with type signature information to distinguish between > + signedness and poly. */ > > static void > -arm_init_neon_builtin (unsigned int fcode, > - neon_builtin_datum *d) > +arm_init_builtin (unsigned int fcode, arm_builtin_datum *d, > + const char * prefix, bool use_sig_in_name) > > use_sig_in_name is dependent on FCODE so just deduce it from that > locally in arm_init_builtin. > > This is ok otherwise. > Thanks, > Kyrill > > Hi, Reworked patch according to comments. No changes to ChangeLog. Is this OK? Cheers, Andre diff --git a/gcc/config/arm/arm-builtins.c b/gcc/config/arm/arm-builtins.c index 5ed38d1608cfbfbd1248d76705fcf675bc36c2b2..da6331fdc729461adeb81d84c0c425bc45b80b8c 100644 --- a/gcc/config/arm/arm-builtins.c +++ b/gcc/config/arm/arm-builtins.c @@ -202,7 +202,7 @@ typedef struct { const enum insn_code code; unsigned int fcode; enum arm_type_qualifiers *qualifiers; -} neon_builtin_datum; +} arm_builtin_datum; #define CF(N,X) CODE_FOR_neon_##N##X @@ -242,7 +242,7 @@ typedef struct { VAR11 (T, N, A, B, C, D, E, F, G, H, I, J, K) \ VAR1 (T, N, L) -/* The NEON builtin data can be found in arm_neon_builtins.def and +/* The builtin data can be found in arm_neon_builtins.def, arm_vfp_builtins.def. The entries in arm_neon_builtins.def require TARGET_NEON to be true. The feature tests are checked when the builtins are expanded. @@ -252,14 +252,14 @@ typedef struct { would be specified after the assembler mnemonic, which usually refers to the last vector operand. The modes listed per instruction should be the same as those defined for that - instruction's pattern in neon.md. */ + instruction's pattern, for instance in neon.md. */ -static neon_builtin_datum vfp_builtin_data[] = +static arm_builtin_datum vfp_builtin_data[] = { #include "arm_vfp_builtins.def" }; -static
Re: [PATCH 1/6][ARM] Refactor NEON builtin framework to work for other builtins
Hi Andre, On 09/11/16 10:11, Andre Vieira (lists) wrote: Hi, Refactor NEON builtin framework such that it can be used to implement other builtins. Is this OK for trunk? Regards, Andre gcc/ChangeLog 2016-11-09 Andre Vieira* config/arm/arm-builtins.c (neon_builtin_datum): Rename to .. (arm_builtin_datum): ... this. (arm_init_neon_builtin): Rename to ... (arm_init_builtin): ... this. Add a new parameters PREFIX and USE_SIG_IN_NAME. (arm_init_neon_builtins): Replace 'arm_init_neon_builtin' with 'arm_init_builtin'. Replace type 'neon_builtin_datum' with 'arm_builtin_datum'. (arm_init_vfp_builtins): Likewise. (builtin_arg): Rename enum's replacing 'NEON_ARG' with 'ARG_BUILTIN' and add a 'ARG_BUILTIN_NEON_MEMORY. (arm_expand_neon_args): Rename to ... (arm_expand_builtin_args): ... this. Rename builtin_arg enum values and differentiate between ARG_BUILTIN_MEMORY and ARG_BUILTIN_NEON_MEMORY. (arm_expand_neon_builtin_1): Rename to ... (arm_expand_builtin_1): ... this. Rename builtin_arg enum values, arm_expand_builtin_args and add bool parameter NEON. (arm_expand_neon_builtin): Use arm_expand_builtin_1. (arm_expand_vfp_builtin): Likewise. (NEON_MAX_BUILTIN_ARGS): Remove, it was unused. /* Expand a neon builtin. This is also used for vfp builtins, which behave in the same way. These builtins are "special" because they don't have symbolic constants defined per-instruction or per instruction-variant. Instead, the - required info is looked up in the NEON_BUILTIN_DATA record that is passed + required info is looked up in the ARM_BUILTIN_DATA record that is passed into the function. */ The comment should be updated now that it's not just NEON builtins that are expanded through this function. static rtx -arm_expand_neon_builtin_1 (int fcode, tree exp, rtx target, - neon_builtin_datum *d) +arm_expand_builtin_1 (int fcode, tree exp, rtx target, + arm_builtin_datum *d, bool neon) { I'm not a fan of this 'neon' boolean as it can cause confusion among the users of the function (see long thread at https://gcc.gnu.org/ml/gcc/2016-10/msg4.html). Whether the builtin is a NEON/VFP builtin can be distinguished from FCODE, so lets just make that bool neon a local variable and initialise it accordingly from FCODE. Same for: +/* Set up a builtin. It will use information stored in the argument struct D to + derive the builtin's type signature and name. It will append the name in D + to the PREFIX passed and use these to create a builtin declaration that is + then stored in 'arm_builtin_decls' under index FCODE. This FCODE is also + written back to D for future use. If USE_SIG_IN_NAME is true the builtin's + name is appended with type signature information to distinguish between + signedness and poly. */ static void -arm_init_neon_builtin (unsigned int fcode, - neon_builtin_datum *d) +arm_init_builtin (unsigned int fcode, arm_builtin_datum *d, + const char * prefix, bool use_sig_in_name) use_sig_in_name is dependent on FCODE so just deduce it from that locally in arm_init_builtin. This is ok otherwise. Thanks, Kyrill