* [RFC] Adding a new attribute to function param to mark it as constant @ 2021-07-23 10:53 Prathamesh Kulkarni 2021-07-23 17:59 ` Andrew Pinski 2021-08-03 21:55 ` Segher Boessenkool 0 siblings, 2 replies; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-07-23 10:53 UTC (permalink / raw) To: GCC Development, Richard Earnshaw Hi, Continuing from this thread, https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html The proposal is to provide a mechanism to mark a parameter in a function as a literal constant. Motivation: Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: __extension__ extern __inline int32x2_t __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) vshl_n_s32 (int32x2_t __a, const int __b) { return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); } and it's caller: int32x2_t f (int32x2_t x) { return vshl_n_s32 (x, 1); } The constraint here is that, vshl_n<type> intrinsics require that the second arg (__b), should be an immediate value. Currently, this check is performed by arm_expand_builtin_args, and if a non-constant value gets passed, it emits the following diagnostic: ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must be a constant immediate 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ However, we're trying to replace builtin calls with gcc's C vector extensions where possible (PR66791), because the builtins are opaque to the optimizers. Unfortunately, we lose type checking of immediate value if we replace the builtin with << operator: __extension__ extern __inline int32x2_t __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) vshl_n_s32 (int32x2_t __a, const int __b) { return __a << __b; } So, I was wondering if we should have an attribute for a parameter to specifically mark it as a constant value with optional range value info ? As Richard suggested, sth like: void foo(int x __attribute__((literal_constant (min_val, max_val))); Thanks, Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-07-23 10:53 [RFC] Adding a new attribute to function param to mark it as constant Prathamesh Kulkarni @ 2021-07-23 17:59 ` Andrew Pinski 2021-07-26 9:04 ` Prathamesh Kulkarni 2021-08-03 21:55 ` Segher Boessenkool 1 sibling, 1 reply; 31+ messages in thread From: Andrew Pinski @ 2021-07-23 17:59 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: GCC Development, Richard Earnshaw On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc <gcc@gcc.gnu.org> wrote: > > Hi, > Continuing from this thread, > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > The proposal is to provide a mechanism to mark a parameter in a > function as a literal constant. > > Motivation: > Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > > __extension__ extern __inline int32x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshl_n_s32 (int32x2_t __a, const int __b) > { > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > } > > and it's caller: > > int32x2_t f (int32x2_t x) > { > return vshl_n_s32 (x, 1); > } Can't you do similar to what is done already in the aarch64 back-end: #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) #define __AARCH64_LANE_CHECK(__vec, __idx) \ __builtin_aarch64_im_lane_boundsi (sizeof(__vec), sizeof(__vec[0]), __idx) ? Yes this is about lanes but you could even add one for min/max which is generic and such; add an argument to say the intrinsics name even. You could do this as a non-target builtin if you want and reuse it also for the aarch64 backend. Thanks, Andrew Pinski > > The constraint here is that, vshl_n<type> intrinsics require that the > second arg (__b), > should be an immediate value. > Currently, this check is performed by arm_expand_builtin_args, and if > a non-constant > value gets passed, it emits the following diagnostic: > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > be a constant immediate > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > However, we're trying to replace builtin calls with gcc's C vector > extensions where > possible (PR66791), because the builtins are opaque to the optimizers. > > Unfortunately, we lose type checking of immediate value if we replace > the builtin > with << operator: > > __extension__ extern __inline int32x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshl_n_s32 (int32x2_t __a, const int __b) > { > return __a << __b; > } > > So, I was wondering if we should have an attribute for a parameter to > specifically > mark it as a constant value with optional range value info ? > As Richard suggested, sth like: > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > Thanks, > Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-07-23 17:59 ` Andrew Pinski @ 2021-07-26 9:04 ` Prathamesh Kulkarni 2021-07-27 8:19 ` Richard Biener 0 siblings, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-07-26 9:04 UTC (permalink / raw) To: Andrew Pinski; +Cc: GCC Development, Richard Earnshaw On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: > > On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > <gcc@gcc.gnu.org> wrote: > > > > Hi, > > Continuing from this thread, > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > > The proposal is to provide a mechanism to mark a parameter in a > > function as a literal constant. > > > > Motivation: > > Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > > > > __extension__ extern __inline int32x2_t > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > vshl_n_s32 (int32x2_t __a, const int __b) > > { > > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > } > > > > and it's caller: > > > > int32x2_t f (int32x2_t x) > > { > > return vshl_n_s32 (x, 1); > > } > > Can't you do similar to what is done already in the aarch64 back-end: > #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > #define __AARCH64_LANE_CHECK(__vec, __idx) \ > __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > sizeof(__vec[0]), __idx) > > ? > Yes this is about lanes but you could even add one for min/max which > is generic and such; add an argument to say the intrinsics name even. > You could do this as a non-target builtin if you want and reuse it > also for the aarch64 backend. Hi Andrew, Thanks for the suggestions. IIUC, we could use this approach to check if the argument falls within a certain range (min / max), but I am not sure how it will help to determine if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require that the 2nd arg is immediate ? Even the current RTL builtin checking is not consistent across optimization levels: For eg: int32x2_t f(int32_t *restrict a) { int32x2_t v = vld1_s32 (a); int b = 2; return vshl_n_s32 (v, b); } With pristine trunk, compiling with -O2 results in no errors because constant propagation replaces 'b' with 2, and during expansion, expand_builtin_args is happy. But at -O0, it results in the error - "argument 2 must be a constant immediate". So I guess we need some mechanism to mark a parameter as a constant ? Thanks, Prathamesh > > Thanks, > Andrew Pinski > > > > > The constraint here is that, vshl_n<type> intrinsics require that the > > second arg (__b), > > should be an immediate value. > > Currently, this check is performed by arm_expand_builtin_args, and if > > a non-constant > > value gets passed, it emits the following diagnostic: > > > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > > be a constant immediate > > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > However, we're trying to replace builtin calls with gcc's C vector > > extensions where > > possible (PR66791), because the builtins are opaque to the optimizers. > > > > Unfortunately, we lose type checking of immediate value if we replace > > the builtin > > with << operator: > > > > __extension__ extern __inline int32x2_t > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > vshl_n_s32 (int32x2_t __a, const int __b) > > { > > return __a << __b; > > } > > > > So, I was wondering if we should have an attribute for a parameter to > > specifically > > mark it as a constant value with optional range value info ? > > As Richard suggested, sth like: > > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > > > Thanks, > > Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-07-26 9:04 ` Prathamesh Kulkarni @ 2021-07-27 8:19 ` Richard Biener 2021-08-03 10:11 ` Prathamesh Kulkarni 0 siblings, 1 reply; 31+ messages in thread From: Richard Biener @ 2021-07-27 8:19 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Andrew Pinski, GCC Development, Richard Earnshaw On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc <gcc@gcc.gnu.org> wrote: > > On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: > > > > On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > > <gcc@gcc.gnu.org> wrote: > > > > > > Hi, > > > Continuing from this thread, > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > > > The proposal is to provide a mechanism to mark a parameter in a > > > function as a literal constant. > > > > > > Motivation: > > > Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > > > > > > __extension__ extern __inline int32x2_t > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > { > > > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > } > > > > > > and it's caller: > > > > > > int32x2_t f (int32x2_t x) > > > { > > > return vshl_n_s32 (x, 1); > > > } > > > > Can't you do similar to what is done already in the aarch64 back-end: > > #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > > #define __AARCH64_LANE_CHECK(__vec, __idx) \ > > __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > > sizeof(__vec[0]), __idx) > > > > ? > > Yes this is about lanes but you could even add one for min/max which > > is generic and such; add an argument to say the intrinsics name even. > > You could do this as a non-target builtin if you want and reuse it > > also for the aarch64 backend. > Hi Andrew, > Thanks for the suggestions. IIUC, we could use this approach to check > if the argument > falls within a certain range (min / max), but I am not sure how it > will help to determine > if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require > that the 2nd arg is immediate ? > > Even the current RTL builtin checking is not consistent across > optimization levels: > For eg: > int32x2_t f(int32_t *restrict a) > { > int32x2_t v = vld1_s32 (a); > int b = 2; > return vshl_n_s32 (v, b); > } > > With pristine trunk, compiling with -O2 results in no errors because > constant propagation replaces 'b' with 2, and during expansion, > expand_builtin_args is happy. But at -O0, it results in the error - > "argument 2 must be a constant immediate". > > So I guess we need some mechanism to mark a parameter as a constant ? I guess you want to mark it in a way that the frontend should force constant evaluation and error if that's not possible? C++ doesn't allow to declare a parameter as 'constexpr' but something like void foo (consteval int i); since I guess you do want to allow passing constexpr arguments in C++ or in C extended forms of constants like static const int a[4]; foo (a[1]); ? But yes, this looks useful to me. Richard. > > Thanks, > Prathamesh > > > > Thanks, > > Andrew Pinski > > > > > > > > The constraint here is that, vshl_n<type> intrinsics require that the > > > second arg (__b), > > > should be an immediate value. > > > Currently, this check is performed by arm_expand_builtin_args, and if > > > a non-constant > > > value gets passed, it emits the following diagnostic: > > > > > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > > > be a constant immediate > > > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > However, we're trying to replace builtin calls with gcc's C vector > > > extensions where > > > possible (PR66791), because the builtins are opaque to the optimizers. > > > > > > Unfortunately, we lose type checking of immediate value if we replace > > > the builtin > > > with << operator: > > > > > > __extension__ extern __inline int32x2_t > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > { > > > return __a << __b; > > > } > > > > > > So, I was wondering if we should have an attribute for a parameter to > > > specifically > > > mark it as a constant value with optional range value info ? > > > As Richard suggested, sth like: > > > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > > > > > Thanks, > > > Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-07-27 8:19 ` Richard Biener @ 2021-08-03 10:11 ` Prathamesh Kulkarni 2021-08-03 10:13 ` Prathamesh Kulkarni 2021-08-03 17:44 ` Martin Sebor 0 siblings, 2 replies; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-03 10:11 UTC (permalink / raw) To: Richard Biener; +Cc: Andrew Pinski, GCC Development, Richard Earnshaw [-- Attachment #1: Type: text/plain, Size: 6248 bytes --] On Tue, 27 Jul 2021 at 13:49, Richard Biener <richard.guenther@gmail.com> wrote: > > On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc > <gcc@gcc.gnu.org> wrote: > > > > On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: > > > > > > On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > > > <gcc@gcc.gnu.org> wrote: > > > > > > > > Hi, > > > > Continuing from this thread, > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > > > > The proposal is to provide a mechanism to mark a parameter in a > > > > function as a literal constant. > > > > > > > > Motivation: > > > > Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > > > > > > > > __extension__ extern __inline int32x2_t > > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > > { > > > > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > > } > > > > > > > > and it's caller: > > > > > > > > int32x2_t f (int32x2_t x) > > > > { > > > > return vshl_n_s32 (x, 1); > > > > } > > > > > > Can't you do similar to what is done already in the aarch64 back-end: > > > #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > > > #define __AARCH64_LANE_CHECK(__vec, __idx) \ > > > __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > > > sizeof(__vec[0]), __idx) > > > > > > ? > > > Yes this is about lanes but you could even add one for min/max which > > > is generic and such; add an argument to say the intrinsics name even. > > > You could do this as a non-target builtin if you want and reuse it > > > also for the aarch64 backend. > > Hi Andrew, > > Thanks for the suggestions. IIUC, we could use this approach to check > > if the argument > > falls within a certain range (min / max), but I am not sure how it > > will help to determine > > if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require > > that the 2nd arg is immediate ? > > > > Even the current RTL builtin checking is not consistent across > > optimization levels: > > For eg: > > int32x2_t f(int32_t *restrict a) > > { > > int32x2_t v = vld1_s32 (a); > > int b = 2; > > return vshl_n_s32 (v, b); > > } > > > > With pristine trunk, compiling with -O2 results in no errors because > > constant propagation replaces 'b' with 2, and during expansion, > > expand_builtin_args is happy. But at -O0, it results in the error - > > "argument 2 must be a constant immediate". > > > > So I guess we need some mechanism to mark a parameter as a constant ? > > I guess you want to mark it in a way that the frontend should force > constant evaluation and error if that's not possible? C++ doesn't > allow to declare a parameter as 'constexpr' but something like > > void foo (consteval int i); > > since I guess you do want to allow passing constexpr arguments > in C++ or in C extended forms of constants like > > static const int a[4]; > > foo (a[1]); > > ? But yes, this looks useful to me. Hi Richard, Thanks for the suggestions and sorry for late response. I have attached a prototype patch that implements consteval attribute. As implemented, the attribute takes at least one argument(s), which refer to parameter position, and the corresponding parameter must be const qualified, failing which, the attribute is ignored. The patch does type-checking for arguments in check_function_consteval_attr, which simply does a linear search to see if the corresponding argument number is included in consteval attribute, and if yes, it checks if the argument is CONSTANT_CLASS_P. This works for simple constants and the intrinsics use-case, but rejects "extended constants" as in your above example. I guess we can special case to detect cases like a[i] where 'a' is const and 'i' is an immediate, but I am not sure if there's a way to force constant evaluation in FE ? I tried using c_fully_fold (argarray[i], false, &maybe_const) but that didn't seem to work. Do you have any suggestions on how to detect those in the front-end ? Example: __attribute__((consteval(1, 2))) void f(const int x, int *p) {} Calling it with: 1) f(0, (int *) 0) is accepted. 2) void g(int *p) { f (0, p); } emits the following error: test.c: In function ‘g’: test.c:7:9: error: argument 2 is not a constant. 7 | f (0, p); | ^ test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. 2 | void f(const int x, int *const p) | ^ Thanks, Prathamesh > > Richard. > > > > > Thanks, > > Prathamesh > > > > > > Thanks, > > > Andrew Pinski > > > > > > > > > > > The constraint here is that, vshl_n<type> intrinsics require that the > > > > second arg (__b), > > > > should be an immediate value. > > > > Currently, this check is performed by arm_expand_builtin_args, and if > > > > a non-constant > > > > value gets passed, it emits the following diagnostic: > > > > > > > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > > > > be a constant immediate > > > > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > However, we're trying to replace builtin calls with gcc's C vector > > > > extensions where > > > > possible (PR66791), because the builtins are opaque to the optimizers. > > > > > > > > Unfortunately, we lose type checking of immediate value if we replace > > > > the builtin > > > > with << operator: > > > > > > > > __extension__ extern __inline int32x2_t > > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > > { > > > > return __a << __b; > > > > } > > > > > > > > So, I was wondering if we should have an attribute for a parameter to > > > > specifically > > > > mark it as a constant value with optional range value info ? > > > > As Richard suggested, sth like: > > > > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > > > > > > > Thanks, > > > > Prathamesh [-- Attachment #2: gnu-708-1.diff --] [-- Type: application/octet-stream, Size: 4643 bytes --] diff --git a/gcc/c-family/c-attribs.c b/gcc/c-family/c-attribs.c index e60fb31d8c8..354112e96cc 100644 --- a/gcc/c-family/c-attribs.c +++ b/gcc/c-family/c-attribs.c @@ -121,6 +121,7 @@ static tree handle_no_limit_stack_attribute (tree *, tree, tree, int, bool *); static tree handle_pure_attribute (tree *, tree, tree, int, bool *); static tree handle_tm_attribute (tree *, tree, tree, int, bool *); +static tree handle_consteval_attribute (tree *, tree, tree, int, bool *); static tree handle_tm_wrap_attribute (tree *, tree, tree, int, bool *); static tree handle_novops_attribute (tree *, tree, tree, int, bool *); static tree handle_vector_size_attribute (tree *, tree, tree, int, @@ -394,6 +395,8 @@ const struct attribute_spec c_common_attribute_table[] = handle_tm_attribute, NULL }, { "transaction_may_cancel_outer", 0, 0, false, true, false, false, handle_tm_attribute, NULL }, + { "consteval", 1, -1, false, true, true, false, + handle_consteval_attribute, NULL }, /* ??? These two attributes didn't make the transition from the Intel language document to the multi-vendor language document. */ { "transaction_pure", 0, 0, false, true, false, false, @@ -4032,6 +4035,56 @@ handle_tm_wrap_attribute (tree *node, tree name, tree args, return NULL_TREE; } +/* Handle consteval(argno1, [argno2, ...]). +The attrbute expects at least one arg which is INTEGER_CST and denotes the +position of parameter starting from 1. */ + +static tree +handle_consteval_attribute (tree *node, tree name, tree args, + int ARG_UNUSED (flags), bool *no_add_attrs) +{ + tree fntype = *node; + int nargs = type_num_arguments (fntype); + int argno = 0; + + for (tree ap = args; ap; ap = TREE_CHAIN (ap), argno++) + { + tree val = TREE_VALUE (ap); + if (TREE_CODE (val) != INTEGER_CST) + { + warning (OPT_Wattributes, + "%qE attribute %i value %qE is not an integer constant " + "ignoing attribute.", + name, argno, val); + *no_add_attrs = true; + return NULL_TREE; + } + int pos = TREE_INT_CST_LOW (val); + if (pos < 1 || pos > nargs) + { + warning (OPT_Wattributes, + "%qE attribute %i value %qE does not refer to " + "a function parameter, ignoring attribute.", + name, argno, val); + *no_add_attrs = true; + return NULL_TREE; + } + + tree param_type = type_argument_type (fntype, pos); + if (!TYPE_READONLY (param_type)) + { + warning (OPT_Wattributes, + "%qE attribute has value %qE, but parameter %i is not const " + "qualified, ignoring attribute. ", + name, val, pos); + *no_add_attrs = true; + return NULL_TREE; + } + } + + return NULL_TREE; +} + /* Ignore the given attribute. Used when this attribute may be usefully overridden by the target, but is not used generically. */ diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 00ac3c5278b..365a8001ab0 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -5953,6 +5953,43 @@ attribute_fallthrough_p (tree attr) } \f +static void +check_function_consteval_attr (const_tree fndecl, const_tree fntype, + int nargs, tree *argarray, + vec<location_t> *arglocs) +{ + tree consteval_params + = lookup_attribute ("consteval", TYPE_ATTRIBUTES (fntype)); + + if (!consteval_params) + return; + + for (int i = 0; i < nargs; i++) + { + tree ap; + for (ap = TREE_VALUE (consteval_params); ap; ap = TREE_CHAIN (ap)) + { + tree val = TREE_VALUE (ap); + gcc_assert (TREE_CODE (val) == INTEGER_CST); + int pos = TREE_INT_CST_LOW (val); + if ((i + 1) == pos) + break; + } + if (ap) /* Expected constant. */ + { + tree arg = argarray[i]; + if (!CONSTANT_CLASS_P (arg)) + { + error_at ((*arglocs)[i], + "argument %d is not a constant.", i + 1); + inform (DECL_SOURCE_LOCATION (fndecl), + "Function %qE has parameter %i with consteval attribute.", + DECL_NAME (fndecl), i + 1); + } + } + } +} + /* Check for valid arguments being passed to a function with FNTYPE. There are NARGS arguments in the array ARGARRAY. LOC should be used for diagnostics. Return true if either -Wnonnull or -Wrestrict has @@ -6002,6 +6039,8 @@ check_function_arguments (location_t loc, const_tree fndecl, const_tree fntype, } } + check_function_consteval_attr (fndecl, fntype, nargs, argarray, arglocs); + /* check_function_restrict sets the DECL_READ_P for arguments so it must be called unconditionally. */ warned_p |= check_function_restrict (fndecl, fntype, nargs, argarray); ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-03 10:11 ` Prathamesh Kulkarni @ 2021-08-03 10:13 ` Prathamesh Kulkarni 2021-08-03 17:44 ` Martin Sebor 1 sibling, 0 replies; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-03 10:13 UTC (permalink / raw) To: Richard Biener; +Cc: Andrew Pinski, GCC Development, Richard Earnshaw On Tue, 3 Aug 2021 at 15:41, Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> wrote: > > On Tue, 27 Jul 2021 at 13:49, Richard Biener <richard.guenther@gmail.com> wrote: > > > > On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc > > <gcc@gcc.gnu.org> wrote: > > > > > > On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: > > > > > > > > On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > > > > <gcc@gcc.gnu.org> wrote: > > > > > > > > > > Hi, > > > > > Continuing from this thread, > > > > > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > > > > > The proposal is to provide a mechanism to mark a parameter in a > > > > > function as a literal constant. > > > > > > > > > > Motivation: > > > > > Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > > > > > > > > > > __extension__ extern __inline int32x2_t > > > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > > > { > > > > > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > > > } > > > > > > > > > > and it's caller: > > > > > > > > > > int32x2_t f (int32x2_t x) > > > > > { > > > > > return vshl_n_s32 (x, 1); > > > > > } > > > > > > > > Can't you do similar to what is done already in the aarch64 back-end: > > > > #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > > > > #define __AARCH64_LANE_CHECK(__vec, __idx) \ > > > > __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > > > > sizeof(__vec[0]), __idx) > > > > > > > > ? > > > > Yes this is about lanes but you could even add one for min/max which > > > > is generic and such; add an argument to say the intrinsics name even. > > > > You could do this as a non-target builtin if you want and reuse it > > > > also for the aarch64 backend. > > > Hi Andrew, > > > Thanks for the suggestions. IIUC, we could use this approach to check > > > if the argument > > > falls within a certain range (min / max), but I am not sure how it > > > will help to determine > > > if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require > > > that the 2nd arg is immediate ? > > > > > > Even the current RTL builtin checking is not consistent across > > > optimization levels: > > > For eg: > > > int32x2_t f(int32_t *restrict a) > > > { > > > int32x2_t v = vld1_s32 (a); > > > int b = 2; > > > return vshl_n_s32 (v, b); > > > } > > > > > > With pristine trunk, compiling with -O2 results in no errors because > > > constant propagation replaces 'b' with 2, and during expansion, > > > expand_builtin_args is happy. But at -O0, it results in the error - > > > "argument 2 must be a constant immediate". > > > > > > So I guess we need some mechanism to mark a parameter as a constant ? > > > > I guess you want to mark it in a way that the frontend should force > > constant evaluation and error if that's not possible? C++ doesn't > > allow to declare a parameter as 'constexpr' but something like > > > > void foo (consteval int i); > > > > since I guess you do want to allow passing constexpr arguments > > in C++ or in C extended forms of constants like > > > > static const int a[4]; > > > > foo (a[1]); > > > > ? But yes, this looks useful to me. > Hi Richard, > Thanks for the suggestions and sorry for late response. > I have attached a prototype patch that implements consteval attribute. > As implemented, the attribute takes at least one argument(s), which > refer to parameter position, > and the corresponding parameter must be const qualified, failing > which, the attribute is ignored. > > The patch does type-checking for arguments in > check_function_consteval_attr, which > simply does a linear search to see if the corresponding argument > number is included in consteval attribute, > and if yes, it checks if the argument is CONSTANT_CLASS_P. > > This works for simple constants and the intrinsics use-case, but > rejects "extended constants" as in your above example. > I guess we can special case to detect cases like a[i] where 'a' is > const and 'i' is an immediate, > but I am not sure if there's a way to force constant evaluation in FE ? > I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > didn't seem to work. > Do you have any suggestions on how to detect those in the front-end ? > > Example: > > __attribute__((consteval(1, 2))) > void f(const int x, int *p) Oops typo, this should have been int *const p; > {} > > Calling it with: > 1) f(0, (int *) 0) is accepted. > > 2) > void g(int *p) > { > f (0, p); > } > > emits the following error: > test.c: In function ‘g’: > test.c:7:9: error: argument 2 is not a constant. > 7 | f (0, p); > | ^ > test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. > 2 | void f(const int x, int *const p) > | ^ > > Thanks, > Prathamesh > > > > Richard. > > > > > > > > Thanks, > > > Prathamesh > > > > > > > > Thanks, > > > > Andrew Pinski > > > > > > > > > > > > > > The constraint here is that, vshl_n<type> intrinsics require that the > > > > > second arg (__b), > > > > > should be an immediate value. > > > > > Currently, this check is performed by arm_expand_builtin_args, and if > > > > > a non-constant > > > > > value gets passed, it emits the following diagnostic: > > > > > > > > > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > > > > > be a constant immediate > > > > > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > > > > > However, we're trying to replace builtin calls with gcc's C vector > > > > > extensions where > > > > > possible (PR66791), because the builtins are opaque to the optimizers. > > > > > > > > > > Unfortunately, we lose type checking of immediate value if we replace > > > > > the builtin > > > > > with << operator: > > > > > > > > > > __extension__ extern __inline int32x2_t > > > > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > > > > vshl_n_s32 (int32x2_t __a, const int __b) > > > > > { > > > > > return __a << __b; > > > > > } > > > > > > > > > > So, I was wondering if we should have an attribute for a parameter to > > > > > specifically > > > > > mark it as a constant value with optional range value info ? > > > > > As Richard suggested, sth like: > > > > > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > > > > > > > > > Thanks, > > > > > Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-03 10:11 ` Prathamesh Kulkarni 2021-08-03 10:13 ` Prathamesh Kulkarni @ 2021-08-03 17:44 ` Martin Sebor 2021-08-04 9:46 ` Richard Earnshaw 1 sibling, 1 reply; 31+ messages in thread From: Martin Sebor @ 2021-08-03 17:44 UTC (permalink / raw) To: Prathamesh Kulkarni, Richard Biener; +Cc: GCC Development, Richard Earnshaw On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: > On Tue, 27 Jul 2021 at 13:49, Richard Biener <richard.guenther@gmail.com> wrote: >> >> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >> <gcc@gcc.gnu.org> wrote: >>> >>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: >>>> >>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>> <gcc@gcc.gnu.org> wrote: >>>>> >>>>> Hi, >>>>> Continuing from this thread, >>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>> function as a literal constant. >>>>> >>>>> Motivation: >>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>> >>>>> __extension__ extern __inline int32x2_t >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>> { >>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>> } >>>>> >>>>> and it's caller: >>>>> >>>>> int32x2_t f (int32x2_t x) >>>>> { >>>>> return vshl_n_s32 (x, 1); >>>>> } >>>> >>>> Can't you do similar to what is done already in the aarch64 back-end: >>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>> sizeof(__vec[0]), __idx) >>>> >>>> ? >>>> Yes this is about lanes but you could even add one for min/max which >>>> is generic and such; add an argument to say the intrinsics name even. >>>> You could do this as a non-target builtin if you want and reuse it >>>> also for the aarch64 backend. >>> Hi Andrew, >>> Thanks for the suggestions. IIUC, we could use this approach to check >>> if the argument >>> falls within a certain range (min / max), but I am not sure how it >>> will help to determine >>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require >>> that the 2nd arg is immediate ? >>> >>> Even the current RTL builtin checking is not consistent across >>> optimization levels: >>> For eg: >>> int32x2_t f(int32_t *restrict a) >>> { >>> int32x2_t v = vld1_s32 (a); >>> int b = 2; >>> return vshl_n_s32 (v, b); >>> } >>> >>> With pristine trunk, compiling with -O2 results in no errors because >>> constant propagation replaces 'b' with 2, and during expansion, >>> expand_builtin_args is happy. But at -O0, it results in the error - >>> "argument 2 must be a constant immediate". >>> >>> So I guess we need some mechanism to mark a parameter as a constant ? >> >> I guess you want to mark it in a way that the frontend should force >> constant evaluation and error if that's not possible? C++ doesn't >> allow to declare a parameter as 'constexpr' but something like >> >> void foo (consteval int i); >> >> since I guess you do want to allow passing constexpr arguments >> in C++ or in C extended forms of constants like >> >> static const int a[4]; >> >> foo (a[1]); >> >> ? But yes, this looks useful to me. > Hi Richard, > Thanks for the suggestions and sorry for late response. > I have attached a prototype patch that implements consteval attribute. > As implemented, the attribute takes at least one argument(s), which > refer to parameter position, > and the corresponding parameter must be const qualified, failing > which, the attribute is ignored. I'm curious why the argument must be const-qualified. If it's to keep it from being changed in ways that would prevent it from being evaluated at compile-time in the body of the function then to be effective, the enforcement of the constraint should be on the definition of the function. Otherwise, the const qualifier could be used in a declaration of a function but left out from a subsequent definition of it, letting it modify it, like so: __attribute__ ((consteval (1))) void f (const int); inline __attribute__ ((always_inline)) void f (int i) { ++i; } That said, if compile-time function evaluation is the goal then a fully general solution is an attribute that applies to the whole function, not just a subset of its arguments. That way arguments can also be assigned to local variables within the function that can then be modified while still evaluated at compile time and used where constant expressions are expected. I.e., the design goal is [a subset of] C++ constexpr. (Obviously a much bigger project.) A few notes on the prototype patch: conventionally GCC warnings about attributes do not mention when an attribute is ignored. It may be a nice touch to add to all of them but I'd recommend against doing that in individual handlers. Since the attribute allows pointer constants the warning issued when an argument is not one should be generalized (i.e., not refer to just integer constants). (Other than that, C/C++ warnings should start in lowercase and not end in a period). Martin > > The patch does type-checking for arguments in > check_function_consteval_attr, which > simply does a linear search to see if the corresponding argument > number is included in consteval attribute, > and if yes, it checks if the argument is CONSTANT_CLASS_P. > > This works for simple constants and the intrinsics use-case, but > rejects "extended constants" as in your above example. > I guess we can special case to detect cases like a[i] where 'a' is > const and 'i' is an immediate, > but I am not sure if there's a way to force constant evaluation in FE ? > I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > didn't seem to work. > Do you have any suggestions on how to detect those in the front-end ? > > Example: > > __attribute__((consteval(1, 2))) > void f(const int x, int *p) > {} > > Calling it with: > 1) f(0, (int *) 0) is accepted. > > 2) > void g(int *p) > { > f (0, p); > } > > emits the following error: > test.c: In function ‘g’: > test.c:7:9: error: argument 2 is not a constant. > 7 | f (0, p); > | ^ > test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. > 2 | void f(const int x, int *const p) > | ^ > > Thanks, > Prathamesh >> >> Richard. >> >>> >>> Thanks, >>> Prathamesh >>>> >>>> Thanks, >>>> Andrew Pinski >>>> >>>>> >>>>> The constraint here is that, vshl_n<type> intrinsics require that the >>>>> second arg (__b), >>>>> should be an immediate value. >>>>> Currently, this check is performed by arm_expand_builtin_args, and if >>>>> a non-constant >>>>> value gets passed, it emits the following diagnostic: >>>>> >>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must >>>>> be a constant immediate >>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>> >>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>> extensions where >>>>> possible (PR66791), because the builtins are opaque to the optimizers. >>>>> >>>>> Unfortunately, we lose type checking of immediate value if we replace >>>>> the builtin >>>>> with << operator: >>>>> >>>>> __extension__ extern __inline int32x2_t >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>> { >>>>> return __a << __b; >>>>> } >>>>> >>>>> So, I was wondering if we should have an attribute for a parameter to >>>>> specifically >>>>> mark it as a constant value with optional range value info ? >>>>> As Richard suggested, sth like: >>>>> void foo(int x __attribute__((literal_constant (min_val, max_val))); >>>>> >>>>> Thanks, >>>>> Prathamesh ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-03 17:44 ` Martin Sebor @ 2021-08-04 9:46 ` Richard Earnshaw 2021-08-06 0:06 ` Martin Sebor 0 siblings, 1 reply; 31+ messages in thread From: Richard Earnshaw @ 2021-08-04 9:46 UTC (permalink / raw) To: Martin Sebor, Prathamesh Kulkarni, Richard Biener; +Cc: GCC Development On 03/08/2021 18:44, Martin Sebor wrote: > On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >> On Tue, 27 Jul 2021 at 13:49, Richard Biener >> <richard.guenther@gmail.com> wrote: >>> >>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>> <gcc@gcc.gnu.org> wrote: >>>> >>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: >>>>> >>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>> <gcc@gcc.gnu.org> wrote: >>>>>> >>>>>> Hi, >>>>>> Continuing from this thread, >>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>> function as a literal constant. >>>>>> >>>>>> Motivation: >>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>> >>>>>> __extension__ extern __inline int32x2_t >>>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>> { >>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>> } >>>>>> >>>>>> and it's caller: >>>>>> >>>>>> int32x2_t f (int32x2_t x) >>>>>> { >>>>>> return vshl_n_s32 (x, 1); >>>>>> } >>>>> >>>>> Can't you do similar to what is done already in the aarch64 back-end: >>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>> sizeof(__vec[0]), __idx) >>>>> >>>>> ? >>>>> Yes this is about lanes but you could even add one for min/max which >>>>> is generic and such; add an argument to say the intrinsics name even. >>>>> You could do this as a non-target builtin if you want and reuse it >>>>> also for the aarch64 backend. >>>> Hi Andrew, >>>> Thanks for the suggestions. IIUC, we could use this approach to check >>>> if the argument >>>> falls within a certain range (min / max), but I am not sure how it >>>> will help to determine >>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require >>>> that the 2nd arg is immediate ? >>>> >>>> Even the current RTL builtin checking is not consistent across >>>> optimization levels: >>>> For eg: >>>> int32x2_t f(int32_t *restrict a) >>>> { >>>> int32x2_t v = vld1_s32 (a); >>>> int b = 2; >>>> return vshl_n_s32 (v, b); >>>> } >>>> >>>> With pristine trunk, compiling with -O2 results in no errors because >>>> constant propagation replaces 'b' with 2, and during expansion, >>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>> "argument 2 must be a constant immediate". >>>> >>>> So I guess we need some mechanism to mark a parameter as a constant ? >>> >>> I guess you want to mark it in a way that the frontend should force >>> constant evaluation and error if that's not possible? C++ doesn't >>> allow to declare a parameter as 'constexpr' but something like >>> >>> void foo (consteval int i); >>> >>> since I guess you do want to allow passing constexpr arguments >>> in C++ or in C extended forms of constants like >>> >>> static const int a[4]; >>> >>> foo (a[1]); >>> >>> ? But yes, this looks useful to me. >> Hi Richard, >> Thanks for the suggestions and sorry for late response. >> I have attached a prototype patch that implements consteval attribute. >> As implemented, the attribute takes at least one argument(s), which >> refer to parameter position, >> and the corresponding parameter must be const qualified, failing >> which, the attribute is ignored. > > I'm curious why the argument must be const-qualified. If it's > to keep it from being changed in ways that would prevent it from > being evaluated at compile-time in the body of the function then > to be effective, the enforcement of the constraint should be on > the definition of the function. Otherwise, the const qualifier > could be used in a declaration of a function but left out from > a subsequent definition of it, letting it modify it, like so: > > __attribute__ ((consteval (1))) void f (const int); > > inline __attribute__ ((always_inline)) void f (int i) { ++i; } In this particular case it's because the inline function is implementing an intrinsic operation in the architecture and the instruction only supports a literal constant value. At present we catch this while trying to expand the intrinsic, but that can lead to poor diagnostics because we really want to report against the line of code calling the intrinsic. R. > > That said, if compile-time function evaluation is the goal then > a fully general solution is an attribute that applies to the whole > function, not just a subset of its arguments. That way arguments > can also be assigned to local variables within the function that > can then be modified while still evaluated at compile time and > used where constant expressions are expected. I.e., the design > goal is [a subset of] C++ constexpr. (Obviously a much bigger > project.) > > A few notes on the prototype patch: conventionally GCC warnings > about attributes do not mention when an attribute is ignored. > It may be a nice touch to add to all of them but I'd recommend > against doing that in individual handlers. Since the attribute > allows pointer constants the warning issued when an argument is > not one should be generalized (i.e., not refer to just integer > constants). > > (Other than that, C/C++ warnings should start in lowercase and > not end in a period). > > Martin > >> >> The patch does type-checking for arguments in >> check_function_consteval_attr, which >> simply does a linear search to see if the corresponding argument >> number is included in consteval attribute, >> and if yes, it checks if the argument is CONSTANT_CLASS_P. >> >> This works for simple constants and the intrinsics use-case, but >> rejects "extended constants" as in your above example. >> I guess we can special case to detect cases like a[i] where 'a' is >> const and 'i' is an immediate, >> but I am not sure if there's a way to force constant evaluation in FE ? >> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >> didn't seem to work. >> Do you have any suggestions on how to detect those in the front-end ? >> >> Example: >> >> __attribute__((consteval(1, 2))) >> void f(const int x, int *p) >> {} >> >> Calling it with: >> 1) f(0, (int *) 0) is accepted. >> >> 2) >> void g(int *p) >> { >> f (0, p); >> } >> >> emits the following error: >> test.c: In function ‘g’: >> test.c:7:9: error: argument 2 is not a constant. >> 7 | f (0, p); >> | ^ >> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. >> 2 | void f(const int x, int *const p) >> | ^ >> >> Thanks, >> Prathamesh >>> >>> Richard. >>> >>>> >>>> Thanks, >>>> Prathamesh >>>>> >>>>> Thanks, >>>>> Andrew Pinski >>>>> >>>>>> >>>>>> The constraint here is that, vshl_n<type> intrinsics require that the >>>>>> second arg (__b), >>>>>> should be an immediate value. >>>>>> Currently, this check is performed by arm_expand_builtin_args, and if >>>>>> a non-constant >>>>>> value gets passed, it emits the following diagnostic: >>>>>> >>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must >>>>>> be a constant immediate >>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>> >>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>> extensions where >>>>>> possible (PR66791), because the builtins are opaque to the >>>>>> optimizers. >>>>>> >>>>>> Unfortunately, we lose type checking of immediate value if we replace >>>>>> the builtin >>>>>> with << operator: >>>>>> >>>>>> __extension__ extern __inline int32x2_t >>>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>> { >>>>>> return __a << __b; >>>>>> } >>>>>> >>>>>> So, I was wondering if we should have an attribute for a parameter to >>>>>> specifically >>>>>> mark it as a constant value with optional range value info ? >>>>>> As Richard suggested, sth like: >>>>>> void foo(int x __attribute__((literal_constant (min_val, max_val))); >>>>>> >>>>>> Thanks, >>>>>> Prathamesh > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 9:46 ` Richard Earnshaw @ 2021-08-06 0:06 ` Martin Sebor 2021-08-06 10:51 ` Richard Earnshaw 0 siblings, 1 reply; 31+ messages in thread From: Martin Sebor @ 2021-08-06 0:06 UTC (permalink / raw) To: Richard Earnshaw, Prathamesh Kulkarni, Richard Biener; +Cc: GCC Development On 8/4/21 3:46 AM, Richard Earnshaw wrote: > > > On 03/08/2021 18:44, Martin Sebor wrote: >> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >>> On Tue, 27 Jul 2021 at 13:49, Richard Biener >>> <richard.guenther@gmail.com> wrote: >>>> >>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>>> <gcc@gcc.gnu.org> wrote: >>>>> >>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> wrote: >>>>>> >>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>> >>>>>>> Hi, >>>>>>> Continuing from this thread, >>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>>> function as a literal constant. >>>>>>> >>>>>>> Motivation: >>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>>> >>>>>>> __extension__ extern __inline int32x2_t >>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>> { >>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>> } >>>>>>> >>>>>>> and it's caller: >>>>>>> >>>>>>> int32x2_t f (int32x2_t x) >>>>>>> { >>>>>>> return vshl_n_s32 (x, 1); >>>>>>> } >>>>>> >>>>>> Can't you do similar to what is done already in the aarch64 back-end: >>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>>> sizeof(__vec[0]), __idx) >>>>>> >>>>>> ? >>>>>> Yes this is about lanes but you could even add one for min/max which >>>>>> is generic and such; add an argument to say the intrinsics name even. >>>>>> You could do this as a non-target builtin if you want and reuse it >>>>>> also for the aarch64 backend. >>>>> Hi Andrew, >>>>> Thanks for the suggestions. IIUC, we could use this approach to check >>>>> if the argument >>>>> falls within a certain range (min / max), but I am not sure how it >>>>> will help to determine >>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require >>>>> that the 2nd arg is immediate ? >>>>> >>>>> Even the current RTL builtin checking is not consistent across >>>>> optimization levels: >>>>> For eg: >>>>> int32x2_t f(int32_t *restrict a) >>>>> { >>>>> int32x2_t v = vld1_s32 (a); >>>>> int b = 2; >>>>> return vshl_n_s32 (v, b); >>>>> } >>>>> >>>>> With pristine trunk, compiling with -O2 results in no errors because >>>>> constant propagation replaces 'b' with 2, and during expansion, >>>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>>> "argument 2 must be a constant immediate". >>>>> >>>>> So I guess we need some mechanism to mark a parameter as a constant ? >>>> >>>> I guess you want to mark it in a way that the frontend should force >>>> constant evaluation and error if that's not possible? C++ doesn't >>>> allow to declare a parameter as 'constexpr' but something like >>>> >>>> void foo (consteval int i); >>>> >>>> since I guess you do want to allow passing constexpr arguments >>>> in C++ or in C extended forms of constants like >>>> >>>> static const int a[4]; >>>> >>>> foo (a[1]); >>>> >>>> ? But yes, this looks useful to me. >>> Hi Richard, >>> Thanks for the suggestions and sorry for late response. >>> I have attached a prototype patch that implements consteval attribute. >>> As implemented, the attribute takes at least one argument(s), which >>> refer to parameter position, >>> and the corresponding parameter must be const qualified, failing >>> which, the attribute is ignored. >> >> I'm curious why the argument must be const-qualified. If it's >> to keep it from being changed in ways that would prevent it from >> being evaluated at compile-time in the body of the function then >> to be effective, the enforcement of the constraint should be on >> the definition of the function. Otherwise, the const qualifier >> could be used in a declaration of a function but left out from >> a subsequent definition of it, letting it modify it, like so: >> >> __attribute__ ((consteval (1))) void f (const int); >> >> inline __attribute__ ((always_inline)) void f (int i) { ++i; } > > In this particular case it's because the inline function is implementing > an intrinsic operation in the architecture and the instruction only > supports a literal constant value. At present we catch this while > trying to expand the intrinsic, but that can lead to poor diagnostics > because we really want to report against the line of code calling the > intrinsic. Presumably the intrinsics can accept (or can be made to accept) any constant integer expressions, not just literals. E.g., the aarch64 builtin below accepts them. For example, this is accepted in C++: __Int64x2_t void f (__Int32x2_t a) { constexpr int n = 2; return __builtin_aarch64_vshll_nv2si (a, n + 1); } Making the intrinscis accept constant arguments in constexpr-like functions and introducing a constexpr-lite attribute (for C code) was what I was suggesting bythe constexpr comment below. I'd find that a much more general and more powerful design. But my comment above was to highlight that if requiring the function argument referenced by the proposed consteval attribute to be const is necessary to prevent it from being modified then the requirement needs to be enforced not on the declaration but on the definition. You may rightly say: "but we get to define the inline arm function wrappers so we'll make sure to never declare them that way." I don't have a problem with that. What I am saying is that a consteval attribute that required the function argument to be declared const to be effective would be flawed as a general-purpose feature without enforcing the requirement on the definition. Martin > > R. >> >> That said, if compile-time function evaluation is the goal then >> a fully general solution is an attribute that applies to the whole >> function, not just a subset of its arguments. That way arguments >> can also be assigned to local variables within the function that >> can then be modified while still evaluated at compile time and >> used where constant expressions are expected. I.e., the design >> goal is [a subset of] C++ constexpr. (Obviously a much bigger >> project.) >> >> A few notes on the prototype patch: conventionally GCC warnings >> about attributes do not mention when an attribute is ignored. >> It may be a nice touch to add to all of them but I'd recommend >> against doing that in individual handlers. Since the attribute >> allows pointer constants the warning issued when an argument is >> not one should be generalized (i.e., not refer to just integer >> constants). >> >> (Other than that, C/C++ warnings should start in lowercase and >> not end in a period). >> >> Martin >> >>> >>> The patch does type-checking for arguments in >>> check_function_consteval_attr, which >>> simply does a linear search to see if the corresponding argument >>> number is included in consteval attribute, >>> and if yes, it checks if the argument is CONSTANT_CLASS_P. >>> >>> This works for simple constants and the intrinsics use-case, but >>> rejects "extended constants" as in your above example. >>> I guess we can special case to detect cases like a[i] where 'a' is >>> const and 'i' is an immediate, >>> but I am not sure if there's a way to force constant evaluation in FE ? >>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >>> didn't seem to work. >>> Do you have any suggestions on how to detect those in the front-end ? >>> >>> Example: >>> >>> __attribute__((consteval(1, 2))) >>> void f(const int x, int *p) >>> {} >>> >>> Calling it with: >>> 1) f(0, (int *) 0) is accepted. >>> >>> 2) >>> void g(int *p) >>> { >>> f (0, p); >>> } >>> >>> emits the following error: >>> test.c: In function ‘g’: >>> test.c:7:9: error: argument 2 is not a constant. >>> 7 | f (0, p); >>> | ^ >>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval attribute. >>> 2 | void f(const int x, int *const p) >>> | ^ >>> >>> Thanks, >>> Prathamesh >>>> >>>> Richard. >>>> >>>>> >>>>> Thanks, >>>>> Prathamesh >>>>>> >>>>>> Thanks, >>>>>> Andrew Pinski >>>>>> >>>>>>> >>>>>>> The constraint here is that, vshl_n<type> intrinsics require that >>>>>>> the >>>>>>> second arg (__b), >>>>>>> should be an immediate value. >>>>>>> Currently, this check is performed by arm_expand_builtin_args, >>>>>>> and if >>>>>>> a non-constant >>>>>>> value gets passed, it emits the following diagnostic: >>>>>>> >>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 >>>>>>> must >>>>>>> be a constant immediate >>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>> >>>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>>> extensions where >>>>>>> possible (PR66791), because the builtins are opaque to the >>>>>>> optimizers. >>>>>>> >>>>>>> Unfortunately, we lose type checking of immediate value if we >>>>>>> replace >>>>>>> the builtin >>>>>>> with << operator: >>>>>>> >>>>>>> __extension__ extern __inline int32x2_t >>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>> { >>>>>>> return __a << __b; >>>>>>> } >>>>>>> >>>>>>> So, I was wondering if we should have an attribute for a >>>>>>> parameter to >>>>>>> specifically >>>>>>> mark it as a constant value with optional range value info ? >>>>>>> As Richard suggested, sth like: >>>>>>> void foo(int x __attribute__((literal_constant (min_val, max_val))); >>>>>>> >>>>>>> Thanks, >>>>>>> Prathamesh >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-06 0:06 ` Martin Sebor @ 2021-08-06 10:51 ` Richard Earnshaw 2021-08-06 20:39 ` Martin Sebor 0 siblings, 1 reply; 31+ messages in thread From: Richard Earnshaw @ 2021-08-06 10:51 UTC (permalink / raw) To: Martin Sebor, Prathamesh Kulkarni, Richard Biener; +Cc: GCC Development On 06/08/2021 01:06, Martin Sebor via Gcc wrote: > On 8/4/21 3:46 AM, Richard Earnshaw wrote: >> >> >> On 03/08/2021 18:44, Martin Sebor wrote: >>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener >>>> <richard.guenther@gmail.com> wrote: >>>>> >>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>>>> <gcc@gcc.gnu.org> wrote: >>>>>> >>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> >>>>>> wrote: >>>>>>> >>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> Continuing from this thread, >>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>>>> function as a literal constant. >>>>>>>> >>>>>>>> Motivation: >>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>>>> >>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>> __artificial__)) >>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>> { >>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>> } >>>>>>>> >>>>>>>> and it's caller: >>>>>>>> >>>>>>>> int32x2_t f (int32x2_t x) >>>>>>>> { >>>>>>>> return vshl_n_s32 (x, 1); >>>>>>>> } >>>>>>> >>>>>>> Can't you do similar to what is done already in the aarch64 >>>>>>> back-end: >>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>>>> sizeof(__vec[0]), __idx) >>>>>>> >>>>>>> ? >>>>>>> Yes this is about lanes but you could even add one for min/max which >>>>>>> is generic and such; add an argument to say the intrinsics name >>>>>>> even. >>>>>>> You could do this as a non-target builtin if you want and reuse it >>>>>>> also for the aarch64 backend. >>>>>> Hi Andrew, >>>>>> Thanks for the suggestions. IIUC, we could use this approach to check >>>>>> if the argument >>>>>> falls within a certain range (min / max), but I am not sure how it >>>>>> will help to determine >>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics require >>>>>> that the 2nd arg is immediate ? >>>>>> >>>>>> Even the current RTL builtin checking is not consistent across >>>>>> optimization levels: >>>>>> For eg: >>>>>> int32x2_t f(int32_t *restrict a) >>>>>> { >>>>>> int32x2_t v = vld1_s32 (a); >>>>>> int b = 2; >>>>>> return vshl_n_s32 (v, b); >>>>>> } >>>>>> >>>>>> With pristine trunk, compiling with -O2 results in no errors because >>>>>> constant propagation replaces 'b' with 2, and during expansion, >>>>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>>>> "argument 2 must be a constant immediate". >>>>>> >>>>>> So I guess we need some mechanism to mark a parameter as a constant ? >>>>> >>>>> I guess you want to mark it in a way that the frontend should force >>>>> constant evaluation and error if that's not possible? C++ doesn't >>>>> allow to declare a parameter as 'constexpr' but something like >>>>> >>>>> void foo (consteval int i); >>>>> >>>>> since I guess you do want to allow passing constexpr arguments >>>>> in C++ or in C extended forms of constants like >>>>> >>>>> static const int a[4]; >>>>> >>>>> foo (a[1]); >>>>> >>>>> ? But yes, this looks useful to me. >>>> Hi Richard, >>>> Thanks for the suggestions and sorry for late response. >>>> I have attached a prototype patch that implements consteval attribute. >>>> As implemented, the attribute takes at least one argument(s), which >>>> refer to parameter position, >>>> and the corresponding parameter must be const qualified, failing >>>> which, the attribute is ignored. >>> >>> I'm curious why the argument must be const-qualified. If it's >>> to keep it from being changed in ways that would prevent it from >>> being evaluated at compile-time in the body of the function then >>> to be effective, the enforcement of the constraint should be on >>> the definition of the function. Otherwise, the const qualifier >>> could be used in a declaration of a function but left out from >>> a subsequent definition of it, letting it modify it, like so: >>> >>> __attribute__ ((consteval (1))) void f (const int); >>> >>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } >> >> In this particular case it's because the inline function is >> implementing an intrinsic operation in the architecture and the >> instruction only supports a literal constant value. At present we >> catch this while trying to expand the intrinsic, but that can lead to >> poor diagnostics because we really want to report against the line of >> code calling the intrinsic. > > Presumably the intrinsics can accept (or can be made to accept) any > constant integer expressions, not just literals. E.g., the aarch64 > builtin below accepts them. For example, this is accepted in C++: > > __Int64x2_t void f (__Int32x2_t a) > { > constexpr int n = 2; > return __builtin_aarch64_vshll_nv2si (a, n + 1); > } > > Making the intrinscis accept constant arguments in constexpr-like > functions and introducing a constexpr-lite attribute (for C code) > was what I was suggesting bythe constexpr comment below. I'd find > that a much more general and more powerful design. > Yes, it would be unfortunate if the rule made it impossible to avoid idiomatic const-exprs like those you would write in C++, or even those you'd write naturally in C: #define foo (1u << 5) > But my comment above was to highlight that if requiring the function > argument referenced by the proposed consteval attribute to be const > is necessary to prevent it from being modified then the requirement > needs to be enforced not on the declaration but on the definition. > > You may rightly say: "but we get to define the inline arm function > wrappers so we'll make sure to never declare them that way." I don't > have a problem with that. What I am saying is that a consteval > attribute that required the function argument to be declared const > to be effective would be flawed as a general-purpose feature without > enforcing the requirement on the definition. I'm not totally sure I understand what you're saying. But the point of putting the attribute on the declaration is to allow for reporting errors at the point of invocation, so if I call a function with an invalid 'must-be-const-expr' value, I'll get a diagnostic at the point in the source where that is done, not at the point in the (presumably inlined) function where the value ends up being used. Most of our builtins are wrapped into slightly more user-friendly function names so that they conform to the ACLE specification, yet ultimately map onto names into __builtin namespace per gcc's internal standards. If I have: __extension__ extern __inline int8x8_t __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) vshr_n_s8 (int8x8_t __a, const int __b) { return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); } and then use that with extern int8x8_t x; extern int y; f() { int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. ... } I want the error to be reported on the line in f() where the problem really lies, not on the line in the inline function where the problem eventually manifests itself. R. > > Martin > > >> >> R. >>> >>> That said, if compile-time function evaluation is the goal then >>> a fully general solution is an attribute that applies to the whole >>> function, not just a subset of its arguments. That way arguments >>> can also be assigned to local variables within the function that >>> can then be modified while still evaluated at compile time and >>> used where constant expressions are expected. I.e., the design >>> goal is [a subset of] C++ constexpr. (Obviously a much bigger >>> project.) >>> >>> A few notes on the prototype patch: conventionally GCC warnings >>> about attributes do not mention when an attribute is ignored. >>> It may be a nice touch to add to all of them but I'd recommend >>> against doing that in individual handlers. Since the attribute >>> allows pointer constants the warning issued when an argument is >>> not one should be generalized (i.e., not refer to just integer >>> constants). >>> >>> (Other than that, C/C++ warnings should start in lowercase and >>> not end in a period). >>> >>> Martin >>> >>>> >>>> The patch does type-checking for arguments in >>>> check_function_consteval_attr, which >>>> simply does a linear search to see if the corresponding argument >>>> number is included in consteval attribute, >>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. >>>> >>>> This works for simple constants and the intrinsics use-case, but >>>> rejects "extended constants" as in your above example. >>>> I guess we can special case to detect cases like a[i] where 'a' is >>>> const and 'i' is an immediate, >>>> but I am not sure if there's a way to force constant evaluation in FE ? >>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >>>> didn't seem to work. >>>> Do you have any suggestions on how to detect those in the front-end ? >>>> >>>> Example: >>>> >>>> __attribute__((consteval(1, 2))) >>>> void f(const int x, int *p) >>>> {} >>>> >>>> Calling it with: >>>> 1) f(0, (int *) 0) is accepted. >>>> >>>> 2) >>>> void g(int *p) >>>> { >>>> f (0, p); >>>> } >>>> >>>> emits the following error: >>>> test.c: In function ‘g’: >>>> test.c:7:9: error: argument 2 is not a constant. >>>> 7 | f (0, p); >>>> | ^ >>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval >>>> attribute. >>>> 2 | void f(const int x, int *const p) >>>> | ^ >>>> >>>> Thanks, >>>> Prathamesh >>>>> >>>>> Richard. >>>>> >>>>>> >>>>>> Thanks, >>>>>> Prathamesh >>>>>>> >>>>>>> Thanks, >>>>>>> Andrew Pinski >>>>>>> >>>>>>>> >>>>>>>> The constraint here is that, vshl_n<type> intrinsics require >>>>>>>> that the >>>>>>>> second arg (__b), >>>>>>>> should be an immediate value. >>>>>>>> Currently, this check is performed by arm_expand_builtin_args, >>>>>>>> and if >>>>>>>> a non-constant >>>>>>>> value gets passed, it emits the following diagnostic: >>>>>>>> >>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 >>>>>>>> must >>>>>>>> be a constant immediate >>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>>> >>>>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>>>> extensions where >>>>>>>> possible (PR66791), because the builtins are opaque to the >>>>>>>> optimizers. >>>>>>>> >>>>>>>> Unfortunately, we lose type checking of immediate value if we >>>>>>>> replace >>>>>>>> the builtin >>>>>>>> with << operator: >>>>>>>> >>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>> __artificial__)) >>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>> { >>>>>>>> return __a << __b; >>>>>>>> } >>>>>>>> >>>>>>>> So, I was wondering if we should have an attribute for a >>>>>>>> parameter to >>>>>>>> specifically >>>>>>>> mark it as a constant value with optional range value info ? >>>>>>>> As Richard suggested, sth like: >>>>>>>> void foo(int x __attribute__((literal_constant (min_val, >>>>>>>> max_val))); >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Prathamesh >>> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-06 10:51 ` Richard Earnshaw @ 2021-08-06 20:39 ` Martin Sebor 2021-08-12 8:32 ` Prathamesh Kulkarni 0 siblings, 1 reply; 31+ messages in thread From: Martin Sebor @ 2021-08-06 20:39 UTC (permalink / raw) To: Richard Earnshaw, Prathamesh Kulkarni, Richard Biener; +Cc: GCC Development On 8/6/21 4:51 AM, Richard Earnshaw wrote: > > > On 06/08/2021 01:06, Martin Sebor via Gcc wrote: >> On 8/4/21 3:46 AM, Richard Earnshaw wrote: >>> >>> >>> On 03/08/2021 18:44, Martin Sebor wrote: >>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener >>>>> <richard.guenther@gmail.com> wrote: >>>>>> >>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>> >>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> >>>>>>> wrote: >>>>>>>> >>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>>> >>>>>>>>> Hi, >>>>>>>>> Continuing from this thread, >>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>>>>> function as a literal constant. >>>>>>>>> >>>>>>>>> Motivation: >>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>>>>> >>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>> __artificial__)) >>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>> { >>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>> } >>>>>>>>> >>>>>>>>> and it's caller: >>>>>>>>> >>>>>>>>> int32x2_t f (int32x2_t x) >>>>>>>>> { >>>>>>>>> return vshl_n_s32 (x, 1); >>>>>>>>> } >>>>>>>> >>>>>>>> Can't you do similar to what is done already in the aarch64 >>>>>>>> back-end: >>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>>>>> sizeof(__vec[0]), __idx) >>>>>>>> >>>>>>>> ? >>>>>>>> Yes this is about lanes but you could even add one for min/max >>>>>>>> which >>>>>>>> is generic and such; add an argument to say the intrinsics name >>>>>>>> even. >>>>>>>> You could do this as a non-target builtin if you want and reuse it >>>>>>>> also for the aarch64 backend. >>>>>>> Hi Andrew, >>>>>>> Thanks for the suggestions. IIUC, we could use this approach to >>>>>>> check >>>>>>> if the argument >>>>>>> falls within a certain range (min / max), but I am not sure how it >>>>>>> will help to determine >>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics >>>>>>> require >>>>>>> that the 2nd arg is immediate ? >>>>>>> >>>>>>> Even the current RTL builtin checking is not consistent across >>>>>>> optimization levels: >>>>>>> For eg: >>>>>>> int32x2_t f(int32_t *restrict a) >>>>>>> { >>>>>>> int32x2_t v = vld1_s32 (a); >>>>>>> int b = 2; >>>>>>> return vshl_n_s32 (v, b); >>>>>>> } >>>>>>> >>>>>>> With pristine trunk, compiling with -O2 results in no errors because >>>>>>> constant propagation replaces 'b' with 2, and during expansion, >>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>>>>> "argument 2 must be a constant immediate". >>>>>>> >>>>>>> So I guess we need some mechanism to mark a parameter as a >>>>>>> constant ? >>>>>> >>>>>> I guess you want to mark it in a way that the frontend should force >>>>>> constant evaluation and error if that's not possible? C++ doesn't >>>>>> allow to declare a parameter as 'constexpr' but something like >>>>>> >>>>>> void foo (consteval int i); >>>>>> >>>>>> since I guess you do want to allow passing constexpr arguments >>>>>> in C++ or in C extended forms of constants like >>>>>> >>>>>> static const int a[4]; >>>>>> >>>>>> foo (a[1]); >>>>>> >>>>>> ? But yes, this looks useful to me. >>>>> Hi Richard, >>>>> Thanks for the suggestions and sorry for late response. >>>>> I have attached a prototype patch that implements consteval attribute. >>>>> As implemented, the attribute takes at least one argument(s), which >>>>> refer to parameter position, >>>>> and the corresponding parameter must be const qualified, failing >>>>> which, the attribute is ignored. >>>> >>>> I'm curious why the argument must be const-qualified. If it's >>>> to keep it from being changed in ways that would prevent it from >>>> being evaluated at compile-time in the body of the function then >>>> to be effective, the enforcement of the constraint should be on >>>> the definition of the function. Otherwise, the const qualifier >>>> could be used in a declaration of a function but left out from >>>> a subsequent definition of it, letting it modify it, like so: >>>> >>>> __attribute__ ((consteval (1))) void f (const int); >>>> >>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } >>> >>> In this particular case it's because the inline function is >>> implementing an intrinsic operation in the architecture and the >>> instruction only supports a literal constant value. At present we >>> catch this while trying to expand the intrinsic, but that can lead to >>> poor diagnostics because we really want to report against the line of >>> code calling the intrinsic. >> >> Presumably the intrinsics can accept (or can be made to accept) any >> constant integer expressions, not just literals. E.g., the aarch64 >> builtin below accepts them. For example, this is accepted in C++: >> >> __Int64x2_t void f (__Int32x2_t a) >> { >> constexpr int n = 2; >> return __builtin_aarch64_vshll_nv2si (a, n + 1); >> } >> >> Making the intrinscis accept constant arguments in constexpr-like >> functions and introducing a constexpr-lite attribute (for C code) >> was what I was suggesting bythe constexpr comment below. I'd find >> that a much more general and more powerful design. >> > > Yes, it would be unfortunate if the rule made it impossible to avoid > idiomatic const-exprs like those you would write in C++, or even those > you'd write naturally in C: > > #define foo (1u << 5) > > >> But my comment above was to highlight that if requiring the function >> argument referenced by the proposed consteval attribute to be const >> is necessary to prevent it from being modified then the requirement >> needs to be enforced not on the declaration but on the definition. >> >> You may rightly say: "but we get to define the inline arm function >> wrappers so we'll make sure to never declare them that way." I don't >> have a problem with that. What I am saying is that a consteval >> attribute that required the function argument to be declared const >> to be effective would be flawed as a general-purpose feature without >> enforcing the requirement on the definition. > > I'm not totally sure I understand what you're saying. But the point of > putting the attribute on the declaration is to allow for reporting > errors at the point of invocation, so if I call a function with an > invalid 'must-be-const-expr' value, I'll get a diagnostic at the point > in the source where that is done, not at the point in the (presumably > inlined) function where the value ends up being used. Most of our > builtins are wrapped into slightly more user-friendly function names so > that they conform to the ACLE specification, yet ultimately map onto > names into __builtin namespace per gcc's internal standards. If I have: > > __extension__ extern __inline int8x8_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshr_n_s8 (int8x8_t __a, const int __b) > { > return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); > } > > > and then use that with > > extern int8x8_t x; > extern int y; > > f() { > int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. > ... > } > > I want the error to be reported on the line in f() where the problem > really lies, not on the line in the inline function where the problem > eventually manifests itself. Yes, I understand that. What I'm pointing out is that the description of the attribute above "the attribute takes at least one argument(s), which refer to parameter position, and the corresponding parameter must be const qualified, failing which, the attribute is ignored." doesn't match the POC patch because it silently accepts the following two declarations: int __attribute__ ((consteval (1))) f (const int); // attribute consteval accepted... int f (int a) // ...even though the argument is not const { return ++a; } The attribute handler should check that the const qualifier on the argument is in the definition of the function. I'm also pointing out that from the POV of the user of the attribute, the const shouldn't be necessary: the attribute consteval alone should be sufficient. Hence my original question: why must the argument be const-qualified? (Why can't you print equally good error messages without it?) Martin > > R. > >> >> Martin >> >> >>> >>> R. >>>> >>>> That said, if compile-time function evaluation is the goal then >>>> a fully general solution is an attribute that applies to the whole >>>> function, not just a subset of its arguments. That way arguments >>>> can also be assigned to local variables within the function that >>>> can then be modified while still evaluated at compile time and >>>> used where constant expressions are expected. I.e., the design >>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger >>>> project.) >>>> >>>> A few notes on the prototype patch: conventionally GCC warnings >>>> about attributes do not mention when an attribute is ignored. >>>> It may be a nice touch to add to all of them but I'd recommend >>>> against doing that in individual handlers. Since the attribute >>>> allows pointer constants the warning issued when an argument is >>>> not one should be generalized (i.e., not refer to just integer >>>> constants). >>>> >>>> (Other than that, C/C++ warnings should start in lowercase and >>>> not end in a period). >>>> >>>> Martin >>>> >>>>> >>>>> The patch does type-checking for arguments in >>>>> check_function_consteval_attr, which >>>>> simply does a linear search to see if the corresponding argument >>>>> number is included in consteval attribute, >>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. >>>>> >>>>> This works for simple constants and the intrinsics use-case, but >>>>> rejects "extended constants" as in your above example. >>>>> I guess we can special case to detect cases like a[i] where 'a' is >>>>> const and 'i' is an immediate, >>>>> but I am not sure if there's a way to force constant evaluation in >>>>> FE ? >>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >>>>> didn't seem to work. >>>>> Do you have any suggestions on how to detect those in the front-end ? >>>>> >>>>> Example: >>>>> >>>>> __attribute__((consteval(1, 2))) >>>>> void f(const int x, int *p) >>>>> {} >>>>> >>>>> Calling it with: >>>>> 1) f(0, (int *) 0) is accepted. >>>>> >>>>> 2) >>>>> void g(int *p) >>>>> { >>>>> f (0, p); >>>>> } >>>>> >>>>> emits the following error: >>>>> test.c: In function ‘g’: >>>>> test.c:7:9: error: argument 2 is not a constant. >>>>> 7 | f (0, p); >>>>> | ^ >>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval >>>>> attribute. >>>>> 2 | void f(const int x, int *const p) >>>>> | ^ >>>>> >>>>> Thanks, >>>>> Prathamesh >>>>>> >>>>>> Richard. >>>>>> >>>>>>> >>>>>>> Thanks, >>>>>>> Prathamesh >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Andrew Pinski >>>>>>>> >>>>>>>>> >>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require >>>>>>>>> that the >>>>>>>>> second arg (__b), >>>>>>>>> should be an immediate value. >>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, >>>>>>>>> and if >>>>>>>>> a non-constant >>>>>>>>> value gets passed, it emits the following diagnostic: >>>>>>>>> >>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument >>>>>>>>> 2 must >>>>>>>>> be a constant immediate >>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>>>> >>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>>>>> extensions where >>>>>>>>> possible (PR66791), because the builtins are opaque to the >>>>>>>>> optimizers. >>>>>>>>> >>>>>>>>> Unfortunately, we lose type checking of immediate value if we >>>>>>>>> replace >>>>>>>>> the builtin >>>>>>>>> with << operator: >>>>>>>>> >>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>> __artificial__)) >>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>> { >>>>>>>>> return __a << __b; >>>>>>>>> } >>>>>>>>> >>>>>>>>> So, I was wondering if we should have an attribute for a >>>>>>>>> parameter to >>>>>>>>> specifically >>>>>>>>> mark it as a constant value with optional range value info ? >>>>>>>>> As Richard suggested, sth like: >>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, >>>>>>>>> max_val))); >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Prathamesh >>>> >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-06 20:39 ` Martin Sebor @ 2021-08-12 8:32 ` Prathamesh Kulkarni 2021-08-13 17:14 ` Martin Sebor 0 siblings, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-12 8:32 UTC (permalink / raw) To: Martin Sebor; +Cc: Richard Earnshaw, Richard Biener, GCC Development On Sat, 7 Aug 2021 at 02:09, Martin Sebor <msebor@gmail.com> wrote: > > On 8/6/21 4:51 AM, Richard Earnshaw wrote: > > > > > > On 06/08/2021 01:06, Martin Sebor via Gcc wrote: > >> On 8/4/21 3:46 AM, Richard Earnshaw wrote: > >>> > >>> > >>> On 03/08/2021 18:44, Martin Sebor wrote: > >>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: > >>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener > >>>>> <richard.guenther@gmail.com> wrote: > >>>>>> > >>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc > >>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>> > >>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> > >>>>>>> wrote: > >>>>>>>> > >>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > >>>>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>>>> > >>>>>>>>> Hi, > >>>>>>>>> Continuing from this thread, > >>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > >>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a > >>>>>>>>> function as a literal constant. > >>>>>>>>> > >>>>>>>>> Motivation: > >>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > >>>>>>>>> > >>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>> __artificial__)) > >>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>> { > >>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> and it's caller: > >>>>>>>>> > >>>>>>>>> int32x2_t f (int32x2_t x) > >>>>>>>>> { > >>>>>>>>> return vshl_n_s32 (x, 1); > >>>>>>>>> } > >>>>>>>> > >>>>>>>> Can't you do similar to what is done already in the aarch64 > >>>>>>>> back-end: > >>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > >>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ > >>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > >>>>>>>> sizeof(__vec[0]), __idx) > >>>>>>>> > >>>>>>>> ? > >>>>>>>> Yes this is about lanes but you could even add one for min/max > >>>>>>>> which > >>>>>>>> is generic and such; add an argument to say the intrinsics name > >>>>>>>> even. > >>>>>>>> You could do this as a non-target builtin if you want and reuse it > >>>>>>>> also for the aarch64 backend. > >>>>>>> Hi Andrew, > >>>>>>> Thanks for the suggestions. IIUC, we could use this approach to > >>>>>>> check > >>>>>>> if the argument > >>>>>>> falls within a certain range (min / max), but I am not sure how it > >>>>>>> will help to determine > >>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics > >>>>>>> require > >>>>>>> that the 2nd arg is immediate ? > >>>>>>> > >>>>>>> Even the current RTL builtin checking is not consistent across > >>>>>>> optimization levels: > >>>>>>> For eg: > >>>>>>> int32x2_t f(int32_t *restrict a) > >>>>>>> { > >>>>>>> int32x2_t v = vld1_s32 (a); > >>>>>>> int b = 2; > >>>>>>> return vshl_n_s32 (v, b); > >>>>>>> } > >>>>>>> > >>>>>>> With pristine trunk, compiling with -O2 results in no errors because > >>>>>>> constant propagation replaces 'b' with 2, and during expansion, > >>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - > >>>>>>> "argument 2 must be a constant immediate". > >>>>>>> > >>>>>>> So I guess we need some mechanism to mark a parameter as a > >>>>>>> constant ? > >>>>>> > >>>>>> I guess you want to mark it in a way that the frontend should force > >>>>>> constant evaluation and error if that's not possible? C++ doesn't > >>>>>> allow to declare a parameter as 'constexpr' but something like > >>>>>> > >>>>>> void foo (consteval int i); > >>>>>> > >>>>>> since I guess you do want to allow passing constexpr arguments > >>>>>> in C++ or in C extended forms of constants like > >>>>>> > >>>>>> static const int a[4]; > >>>>>> > >>>>>> foo (a[1]); > >>>>>> > >>>>>> ? But yes, this looks useful to me. > >>>>> Hi Richard, > >>>>> Thanks for the suggestions and sorry for late response. > >>>>> I have attached a prototype patch that implements consteval attribute. > >>>>> As implemented, the attribute takes at least one argument(s), which > >>>>> refer to parameter position, > >>>>> and the corresponding parameter must be const qualified, failing > >>>>> which, the attribute is ignored. > >>>> > >>>> I'm curious why the argument must be const-qualified. If it's > >>>> to keep it from being changed in ways that would prevent it from > >>>> being evaluated at compile-time in the body of the function then > >>>> to be effective, the enforcement of the constraint should be on > >>>> the definition of the function. Otherwise, the const qualifier > >>>> could be used in a declaration of a function but left out from > >>>> a subsequent definition of it, letting it modify it, like so: > >>>> > >>>> __attribute__ ((consteval (1))) void f (const int); > >>>> > >>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } > >>> > >>> In this particular case it's because the inline function is > >>> implementing an intrinsic operation in the architecture and the > >>> instruction only supports a literal constant value. At present we > >>> catch this while trying to expand the intrinsic, but that can lead to > >>> poor diagnostics because we really want to report against the line of > >>> code calling the intrinsic. > >> > >> Presumably the intrinsics can accept (or can be made to accept) any > >> constant integer expressions, not just literals. E.g., the aarch64 > >> builtin below accepts them. For example, this is accepted in C++: > >> > >> __Int64x2_t void f (__Int32x2_t a) > >> { > >> constexpr int n = 2; > >> return __builtin_aarch64_vshll_nv2si (a, n + 1); > >> } > >> > >> Making the intrinscis accept constant arguments in constexpr-like > >> functions and introducing a constexpr-lite attribute (for C code) > >> was what I was suggesting bythe constexpr comment below. I'd find > >> that a much more general and more powerful design. > >> > > > > Yes, it would be unfortunate if the rule made it impossible to avoid > > idiomatic const-exprs like those you would write in C++, or even those > > you'd write naturally in C: > > > > #define foo (1u << 5) > > > > > >> But my comment above was to highlight that if requiring the function > >> argument referenced by the proposed consteval attribute to be const > >> is necessary to prevent it from being modified then the requirement > >> needs to be enforced not on the declaration but on the definition. > >> > >> You may rightly say: "but we get to define the inline arm function > >> wrappers so we'll make sure to never declare them that way." I don't > >> have a problem with that. What I am saying is that a consteval > >> attribute that required the function argument to be declared const > >> to be effective would be flawed as a general-purpose feature without > >> enforcing the requirement on the definition. > > > > I'm not totally sure I understand what you're saying. But the point of > > putting the attribute on the declaration is to allow for reporting > > errors at the point of invocation, so if I call a function with an > > invalid 'must-be-const-expr' value, I'll get a diagnostic at the point > > in the source where that is done, not at the point in the (presumably > > inlined) function where the value ends up being used. Most of our > > builtins are wrapped into slightly more user-friendly function names so > > that they conform to the ACLE specification, yet ultimately map onto > > names into __builtin namespace per gcc's internal standards. If I have: > > > > __extension__ extern __inline int8x8_t > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > > vshr_n_s8 (int8x8_t __a, const int __b) > > { > > return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); > > } > > > > > > and then use that with > > > > extern int8x8_t x; > > extern int y; > > > > f() { > > int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. > > ... > > } > > > > I want the error to be reported on the line in f() where the problem > > really lies, not on the line in the inline function where the problem > > eventually manifests itself. > > Yes, I understand that. What I'm pointing out is that > the description of the attribute above > > "the attribute takes at least one argument(s), which refer to > parameter position, and the corresponding parameter must be const > qualified, failing which, the attribute is ignored." > > doesn't match the POC patch because it silently accepts the following > two declarations: > > int __attribute__ ((consteval (1))) > f (const int); // attribute consteval accepted... > > int f (int a) // ...even though the argument is not const > { > return ++a; > } > > The attribute handler should check that the const qualifier on > the argument is in the definition of the function. > > I'm also pointing out that from the POV of the user of the attribute, > the const shouldn't be necessary: the attribute consteval alone should > be sufficient. Hence my original question: why must the argument be > const-qualified? (Why can't you print equally good error messages > without it?) Hi Martin, While writing the POC patch, my expectation was to treat consteval param as a more "restricted version" of const param -- in addition to being readonly, it should only be passed compile-time constant values, and so I thought it probably made sense to require it to be const qualified. But if that doesn't seem like a good idea, we can drop it to be const qualified. I was wondering how do we take this forward ? For a start would this be OK ? 1] Since we also need range info, define consteval attribute to take 3 arguments -- param-pos, min-val, max-val. and requiring the parameter to be an integral type (unlike the one implemented in POC patch). 2] Diagnose a function call, if the corresponding argument (after invoking c_fully_fold) is not INTEGER_CST. My intent is to currently accept ICE's that are also acceptable in other language features like case statement. As you suggest above, we could add "constexpr-lite" attribute to C, for accepting more complicated integral constant expressions, that can also be used in other language features that need ICE's. My concern with this approach tho, is that currently we accept ICE's for intrinsics and rely on the optimizer to resolve them into constants before expansion. So code that passes ICE to vshl_n, would get compiled at higher optimization levels but fail to compile with -O0. I am wondering if there's any existing code that relies on this behavior and will fail to compile if we impose the above restriction ? Thanks, Prathamesh > > Martin > > > > > R. > > > >> > >> Martin > >> > >> > >>> > >>> R. > >>>> > >>>> That said, if compile-time function evaluation is the goal then > >>>> a fully general solution is an attribute that applies to the whole > >>>> function, not just a subset of its arguments. That way arguments > >>>> can also be assigned to local variables within the function that > >>>> can then be modified while still evaluated at compile time and > >>>> used where constant expressions are expected. I.e., the design > >>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger > >>>> project.) > >>>> > >>>> A few notes on the prototype patch: conventionally GCC warnings > >>>> about attributes do not mention when an attribute is ignored. > >>>> It may be a nice touch to add to all of them but I'd recommend > >>>> against doing that in individual handlers. Since the attribute > >>>> allows pointer constants the warning issued when an argument is > >>>> not one should be generalized (i.e., not refer to just integer > >>>> constants). > >>>> > >>>> (Other than that, C/C++ warnings should start in lowercase and > >>>> not end in a period). > >>>> > >>>> Martin > >>>> > >>>>> > >>>>> The patch does type-checking for arguments in > >>>>> check_function_consteval_attr, which > >>>>> simply does a linear search to see if the corresponding argument > >>>>> number is included in consteval attribute, > >>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. > >>>>> > >>>>> This works for simple constants and the intrinsics use-case, but > >>>>> rejects "extended constants" as in your above example. > >>>>> I guess we can special case to detect cases like a[i] where 'a' is > >>>>> const and 'i' is an immediate, > >>>>> but I am not sure if there's a way to force constant evaluation in > >>>>> FE ? > >>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > >>>>> didn't seem to work. > >>>>> Do you have any suggestions on how to detect those in the front-end ? > >>>>> > >>>>> Example: > >>>>> > >>>>> __attribute__((consteval(1, 2))) > >>>>> void f(const int x, int *p) > >>>>> {} > >>>>> > >>>>> Calling it with: > >>>>> 1) f(0, (int *) 0) is accepted. > >>>>> > >>>>> 2) > >>>>> void g(int *p) > >>>>> { > >>>>> f (0, p); > >>>>> } > >>>>> > >>>>> emits the following error: > >>>>> test.c: In function ‘g’: > >>>>> test.c:7:9: error: argument 2 is not a constant. > >>>>> 7 | f (0, p); > >>>>> | ^ > >>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval > >>>>> attribute. > >>>>> 2 | void f(const int x, int *const p) > >>>>> | ^ > >>>>> > >>>>> Thanks, > >>>>> Prathamesh > >>>>>> > >>>>>> Richard. > >>>>>> > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Prathamesh > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Andrew Pinski > >>>>>>>> > >>>>>>>>> > >>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require > >>>>>>>>> that the > >>>>>>>>> second arg (__b), > >>>>>>>>> should be an immediate value. > >>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, > >>>>>>>>> and if > >>>>>>>>> a non-constant > >>>>>>>>> value gets passed, it emits the following diagnostic: > >>>>>>>>> > >>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument > >>>>>>>>> 2 must > >>>>>>>>> be a constant immediate > >>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>>>>>>>> > >>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector > >>>>>>>>> extensions where > >>>>>>>>> possible (PR66791), because the builtins are opaque to the > >>>>>>>>> optimizers. > >>>>>>>>> > >>>>>>>>> Unfortunately, we lose type checking of immediate value if we > >>>>>>>>> replace > >>>>>>>>> the builtin > >>>>>>>>> with << operator: > >>>>>>>>> > >>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>> __artificial__)) > >>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>> { > >>>>>>>>> return __a << __b; > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> So, I was wondering if we should have an attribute for a > >>>>>>>>> parameter to > >>>>>>>>> specifically > >>>>>>>>> mark it as a constant value with optional range value info ? > >>>>>>>>> As Richard suggested, sth like: > >>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, > >>>>>>>>> max_val))); > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> Prathamesh > >>>> > >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-12 8:32 ` Prathamesh Kulkarni @ 2021-08-13 17:14 ` Martin Sebor 2021-08-18 6:52 ` Prathamesh Kulkarni 0 siblings, 1 reply; 31+ messages in thread From: Martin Sebor @ 2021-08-13 17:14 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Earnshaw, Richard Biener, GCC Development On 8/12/21 2:32 AM, Prathamesh Kulkarni wrote: > On Sat, 7 Aug 2021 at 02:09, Martin Sebor <msebor@gmail.com> wrote: >> >> On 8/6/21 4:51 AM, Richard Earnshaw wrote: >>> >>> >>> On 06/08/2021 01:06, Martin Sebor via Gcc wrote: >>>> On 8/4/21 3:46 AM, Richard Earnshaw wrote: >>>>> >>>>> >>>>> On 03/08/2021 18:44, Martin Sebor wrote: >>>>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >>>>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener >>>>>>> <richard.guenther@gmail.com> wrote: >>>>>>>> >>>>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>>> >>>>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>>>>> >>>>>>>>>>> Hi, >>>>>>>>>>> Continuing from this thread, >>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>>>>>>> function as a literal constant. >>>>>>>>>>> >>>>>>>>>>> Motivation: >>>>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>>>>>>> >>>>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>>>> __artificial__)) >>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>>>> { >>>>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> and it's caller: >>>>>>>>>>> >>>>>>>>>>> int32x2_t f (int32x2_t x) >>>>>>>>>>> { >>>>>>>>>>> return vshl_n_s32 (x, 1); >>>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> Can't you do similar to what is done already in the aarch64 >>>>>>>>>> back-end: >>>>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>>>>>>> sizeof(__vec[0]), __idx) >>>>>>>>>> >>>>>>>>>> ? >>>>>>>>>> Yes this is about lanes but you could even add one for min/max >>>>>>>>>> which >>>>>>>>>> is generic and such; add an argument to say the intrinsics name >>>>>>>>>> even. >>>>>>>>>> You could do this as a non-target builtin if you want and reuse it >>>>>>>>>> also for the aarch64 backend. >>>>>>>>> Hi Andrew, >>>>>>>>> Thanks for the suggestions. IIUC, we could use this approach to >>>>>>>>> check >>>>>>>>> if the argument >>>>>>>>> falls within a certain range (min / max), but I am not sure how it >>>>>>>>> will help to determine >>>>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics >>>>>>>>> require >>>>>>>>> that the 2nd arg is immediate ? >>>>>>>>> >>>>>>>>> Even the current RTL builtin checking is not consistent across >>>>>>>>> optimization levels: >>>>>>>>> For eg: >>>>>>>>> int32x2_t f(int32_t *restrict a) >>>>>>>>> { >>>>>>>>> int32x2_t v = vld1_s32 (a); >>>>>>>>> int b = 2; >>>>>>>>> return vshl_n_s32 (v, b); >>>>>>>>> } >>>>>>>>> >>>>>>>>> With pristine trunk, compiling with -O2 results in no errors because >>>>>>>>> constant propagation replaces 'b' with 2, and during expansion, >>>>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>>>>>>> "argument 2 must be a constant immediate". >>>>>>>>> >>>>>>>>> So I guess we need some mechanism to mark a parameter as a >>>>>>>>> constant ? >>>>>>>> >>>>>>>> I guess you want to mark it in a way that the frontend should force >>>>>>>> constant evaluation and error if that's not possible? C++ doesn't >>>>>>>> allow to declare a parameter as 'constexpr' but something like >>>>>>>> >>>>>>>> void foo (consteval int i); >>>>>>>> >>>>>>>> since I guess you do want to allow passing constexpr arguments >>>>>>>> in C++ or in C extended forms of constants like >>>>>>>> >>>>>>>> static const int a[4]; >>>>>>>> >>>>>>>> foo (a[1]); >>>>>>>> >>>>>>>> ? But yes, this looks useful to me. >>>>>>> Hi Richard, >>>>>>> Thanks for the suggestions and sorry for late response. >>>>>>> I have attached a prototype patch that implements consteval attribute. >>>>>>> As implemented, the attribute takes at least one argument(s), which >>>>>>> refer to parameter position, >>>>>>> and the corresponding parameter must be const qualified, failing >>>>>>> which, the attribute is ignored. >>>>>> >>>>>> I'm curious why the argument must be const-qualified. If it's >>>>>> to keep it from being changed in ways that would prevent it from >>>>>> being evaluated at compile-time in the body of the function then >>>>>> to be effective, the enforcement of the constraint should be on >>>>>> the definition of the function. Otherwise, the const qualifier >>>>>> could be used in a declaration of a function but left out from >>>>>> a subsequent definition of it, letting it modify it, like so: >>>>>> >>>>>> __attribute__ ((consteval (1))) void f (const int); >>>>>> >>>>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } >>>>> >>>>> In this particular case it's because the inline function is >>>>> implementing an intrinsic operation in the architecture and the >>>>> instruction only supports a literal constant value. At present we >>>>> catch this while trying to expand the intrinsic, but that can lead to >>>>> poor diagnostics because we really want to report against the line of >>>>> code calling the intrinsic. >>>> >>>> Presumably the intrinsics can accept (or can be made to accept) any >>>> constant integer expressions, not just literals. E.g., the aarch64 >>>> builtin below accepts them. For example, this is accepted in C++: >>>> >>>> __Int64x2_t void f (__Int32x2_t a) >>>> { >>>> constexpr int n = 2; >>>> return __builtin_aarch64_vshll_nv2si (a, n + 1); >>>> } >>>> >>>> Making the intrinscis accept constant arguments in constexpr-like >>>> functions and introducing a constexpr-lite attribute (for C code) >>>> was what I was suggesting bythe constexpr comment below. I'd find >>>> that a much more general and more powerful design. >>>> >>> >>> Yes, it would be unfortunate if the rule made it impossible to avoid >>> idiomatic const-exprs like those you would write in C++, or even those >>> you'd write naturally in C: >>> >>> #define foo (1u << 5) >>> >>> >>>> But my comment above was to highlight that if requiring the function >>>> argument referenced by the proposed consteval attribute to be const >>>> is necessary to prevent it from being modified then the requirement >>>> needs to be enforced not on the declaration but on the definition. >>>> >>>> You may rightly say: "but we get to define the inline arm function >>>> wrappers so we'll make sure to never declare them that way." I don't >>>> have a problem with that. What I am saying is that a consteval >>>> attribute that required the function argument to be declared const >>>> to be effective would be flawed as a general-purpose feature without >>>> enforcing the requirement on the definition. >>> >>> I'm not totally sure I understand what you're saying. But the point of >>> putting the attribute on the declaration is to allow for reporting >>> errors at the point of invocation, so if I call a function with an >>> invalid 'must-be-const-expr' value, I'll get a diagnostic at the point >>> in the source where that is done, not at the point in the (presumably >>> inlined) function where the value ends up being used. Most of our >>> builtins are wrapped into slightly more user-friendly function names so >>> that they conform to the ACLE specification, yet ultimately map onto >>> names into __builtin namespace per gcc's internal standards. If I have: >>> >>> __extension__ extern __inline int8x8_t >>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>> vshr_n_s8 (int8x8_t __a, const int __b) >>> { >>> return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); >>> } >>> >>> >>> and then use that with >>> >>> extern int8x8_t x; >>> extern int y; >>> >>> f() { >>> int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. >>> ... >>> } >>> >>> I want the error to be reported on the line in f() where the problem >>> really lies, not on the line in the inline function where the problem >>> eventually manifests itself. >> >> Yes, I understand that. What I'm pointing out is that >> the description of the attribute above >> >> "the attribute takes at least one argument(s), which refer to >> parameter position, and the corresponding parameter must be const >> qualified, failing which, the attribute is ignored." >> >> doesn't match the POC patch because it silently accepts the following >> two declarations: >> >> int __attribute__ ((consteval (1))) >> f (const int); // attribute consteval accepted... >> >> int f (int a) // ...even though the argument is not const >> { >> return ++a; >> } >> >> The attribute handler should check that the const qualifier on >> the argument is in the definition of the function. >> >> I'm also pointing out that from the POV of the user of the attribute, >> the const shouldn't be necessary: the attribute consteval alone should >> be sufficient. Hence my original question: why must the argument be >> const-qualified? (Why can't you print equally good error messages >> without it?) > Hi Martin, > While writing the POC patch, my expectation was to treat consteval > param as a more "restricted version" > of const param -- in addition to being readonly, it should only be > passed compile-time constant > values, and so I thought it probably made sense to require it to be > const qualified. > But if that doesn't seem like a good idea, we can drop it to be const qualified. I don't think depending on the const qualifier is necessary or a good idea. > > I was wondering how do we take this forward ? > For a start would this be OK ? > 1] Since we also need range info, define consteval attribute to take 3 > arguments -- param-pos, min-val, max-val. > and requiring the parameter to be an integral type (unlike the one > implemented in POC patch). Specifying a range for a variable of any kind (including globals and members) would be useful in general, independent of this enhancement. I would suggest to introduce a separate attribute for that. (With the effects of assigning out-of-range values being undefined and a warning issued if/when detected.) > 2] Diagnose a function call, if the corresponding argument (after > invoking c_fully_fold) is not INTEGER_CST. > > My intent is to currently accept ICE's that are also acceptable in > other language features like case statement. So something like this: __attribute__ ((always_inline, artificial, consteval (1))) inline int f (int i) { switch (i) case i: return i; // or a built-in that expects return 0; // a constant expression argument } int g (void) { return f (7); } // valid but: int h (int i) { return f (i); } // invalid and a nice error for the call to h (as opposed to f): In function 'h': error: argument 1 to 'f' is not a constant integer note: 'f' declared with attribute 'consteval (1)' > As you suggest above, we could add "constexpr-lite" attribute to C, > for accepting more complicated integral constant expressions, > that can also be used in other language features that need ICE's. > > My concern with this approach tho, is that currently we accept ICE's > for intrinsics and rely on the optimizer to resolve them into > constants > before expansion. So calls to the intrinsics don't compile without optimization? (That's not what I saw in my very limited testing but I know almost nothing about the ARM builtins so I could be missing something.) > So code that passes ICE to vshl_n, would get > compiled at higher optimization levels but fail to compile with -O0. > I am wondering if there's any existing code that relies on this > behavior and will fail to compile if we impose the above restriction ? I don't know. But as it turns out, an argument to a C++ constexpr function isn't considered a constant expression within the body of the function even if it is one at the call site. So the extension you describe would also apply to constexpr functions: __attribute__ ((consteval (1))) constexpr int f (int i) { switch (i) case i: return i; // potentially valid with consteval return 0; } constexpr int x = f (1); // valid with consteval (not without) int y = f (y); // not valid Martin > > Thanks, > Prathamesh > > >> >> Martin >> >>> >>> R. >>> >>>> >>>> Martin >>>> >>>> >>>>> >>>>> R. >>>>>> >>>>>> That said, if compile-time function evaluation is the goal then >>>>>> a fully general solution is an attribute that applies to the whole >>>>>> function, not just a subset of its arguments. That way arguments >>>>>> can also be assigned to local variables within the function that >>>>>> can then be modified while still evaluated at compile time and >>>>>> used where constant expressions are expected. I.e., the design >>>>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger >>>>>> project.) >>>>>> >>>>>> A few notes on the prototype patch: conventionally GCC warnings >>>>>> about attributes do not mention when an attribute is ignored. >>>>>> It may be a nice touch to add to all of them but I'd recommend >>>>>> against doing that in individual handlers. Since the attribute >>>>>> allows pointer constants the warning issued when an argument is >>>>>> not one should be generalized (i.e., not refer to just integer >>>>>> constants). >>>>>> >>>>>> (Other than that, C/C++ warnings should start in lowercase and >>>>>> not end in a period). >>>>>> >>>>>> Martin >>>>>> >>>>>>> >>>>>>> The patch does type-checking for arguments in >>>>>>> check_function_consteval_attr, which >>>>>>> simply does a linear search to see if the corresponding argument >>>>>>> number is included in consteval attribute, >>>>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. >>>>>>> >>>>>>> This works for simple constants and the intrinsics use-case, but >>>>>>> rejects "extended constants" as in your above example. >>>>>>> I guess we can special case to detect cases like a[i] where 'a' is >>>>>>> const and 'i' is an immediate, >>>>>>> but I am not sure if there's a way to force constant evaluation in >>>>>>> FE ? >>>>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >>>>>>> didn't seem to work. >>>>>>> Do you have any suggestions on how to detect those in the front-end ? >>>>>>> >>>>>>> Example: >>>>>>> >>>>>>> __attribute__((consteval(1, 2))) >>>>>>> void f(const int x, int *p) >>>>>>> {} >>>>>>> >>>>>>> Calling it with: >>>>>>> 1) f(0, (int *) 0) is accepted. >>>>>>> >>>>>>> 2) >>>>>>> void g(int *p) >>>>>>> { >>>>>>> f (0, p); >>>>>>> } >>>>>>> >>>>>>> emits the following error: >>>>>>> test.c: In function ‘g’: >>>>>>> test.c:7:9: error: argument 2 is not a constant. >>>>>>> 7 | f (0, p); >>>>>>> | ^ >>>>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval >>>>>>> attribute. >>>>>>> 2 | void f(const int x, int *const p) >>>>>>> | ^ >>>>>>> >>>>>>> Thanks, >>>>>>> Prathamesh >>>>>>>> >>>>>>>> Richard. >>>>>>>> >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Prathamesh >>>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> Andrew Pinski >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require >>>>>>>>>>> that the >>>>>>>>>>> second arg (__b), >>>>>>>>>>> should be an immediate value. >>>>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, >>>>>>>>>>> and if >>>>>>>>>>> a non-constant >>>>>>>>>>> value gets passed, it emits the following diagnostic: >>>>>>>>>>> >>>>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument >>>>>>>>>>> 2 must >>>>>>>>>>> be a constant immediate >>>>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>>>>>> >>>>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>>>>>>> extensions where >>>>>>>>>>> possible (PR66791), because the builtins are opaque to the >>>>>>>>>>> optimizers. >>>>>>>>>>> >>>>>>>>>>> Unfortunately, we lose type checking of immediate value if we >>>>>>>>>>> replace >>>>>>>>>>> the builtin >>>>>>>>>>> with << operator: >>>>>>>>>>> >>>>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>>>> __artificial__)) >>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>>>> { >>>>>>>>>>> return __a << __b; >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> So, I was wondering if we should have an attribute for a >>>>>>>>>>> parameter to >>>>>>>>>>> specifically >>>>>>>>>>> mark it as a constant value with optional range value info ? >>>>>>>>>>> As Richard suggested, sth like: >>>>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, >>>>>>>>>>> max_val))); >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Prathamesh >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-13 17:14 ` Martin Sebor @ 2021-08-18 6:52 ` Prathamesh Kulkarni 2021-08-18 14:40 ` Martin Sebor 0 siblings, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-18 6:52 UTC (permalink / raw) To: Martin Sebor; +Cc: Richard Earnshaw, Richard Biener, GCC Development On Fri, 13 Aug 2021 at 22:44, Martin Sebor <msebor@gmail.com> wrote: > > On 8/12/21 2:32 AM, Prathamesh Kulkarni wrote: > > On Sat, 7 Aug 2021 at 02:09, Martin Sebor <msebor@gmail.com> wrote: > >> > >> On 8/6/21 4:51 AM, Richard Earnshaw wrote: > >>> > >>> > >>> On 06/08/2021 01:06, Martin Sebor via Gcc wrote: > >>>> On 8/4/21 3:46 AM, Richard Earnshaw wrote: > >>>>> > >>>>> > >>>>> On 03/08/2021 18:44, Martin Sebor wrote: > >>>>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: > >>>>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener > >>>>>>> <richard.guenther@gmail.com> wrote: > >>>>>>>> > >>>>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc > >>>>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>>>> > >>>>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > >>>>>>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>>>>>> > >>>>>>>>>>> Hi, > >>>>>>>>>>> Continuing from this thread, > >>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > >>>>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a > >>>>>>>>>>> function as a literal constant. > >>>>>>>>>>> > >>>>>>>>>>> Motivation: > >>>>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > >>>>>>>>>>> > >>>>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>>>> __artificial__)) > >>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>>>> { > >>>>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>>>> } > >>>>>>>>>>> > >>>>>>>>>>> and it's caller: > >>>>>>>>>>> > >>>>>>>>>>> int32x2_t f (int32x2_t x) > >>>>>>>>>>> { > >>>>>>>>>>> return vshl_n_s32 (x, 1); > >>>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> Can't you do similar to what is done already in the aarch64 > >>>>>>>>>> back-end: > >>>>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > >>>>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ > >>>>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > >>>>>>>>>> sizeof(__vec[0]), __idx) > >>>>>>>>>> > >>>>>>>>>> ? > >>>>>>>>>> Yes this is about lanes but you could even add one for min/max > >>>>>>>>>> which > >>>>>>>>>> is generic and such; add an argument to say the intrinsics name > >>>>>>>>>> even. > >>>>>>>>>> You could do this as a non-target builtin if you want and reuse it > >>>>>>>>>> also for the aarch64 backend. > >>>>>>>>> Hi Andrew, > >>>>>>>>> Thanks for the suggestions. IIUC, we could use this approach to > >>>>>>>>> check > >>>>>>>>> if the argument > >>>>>>>>> falls within a certain range (min / max), but I am not sure how it > >>>>>>>>> will help to determine > >>>>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics > >>>>>>>>> require > >>>>>>>>> that the 2nd arg is immediate ? > >>>>>>>>> > >>>>>>>>> Even the current RTL builtin checking is not consistent across > >>>>>>>>> optimization levels: > >>>>>>>>> For eg: > >>>>>>>>> int32x2_t f(int32_t *restrict a) > >>>>>>>>> { > >>>>>>>>> int32x2_t v = vld1_s32 (a); > >>>>>>>>> int b = 2; > >>>>>>>>> return vshl_n_s32 (v, b); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> With pristine trunk, compiling with -O2 results in no errors because > >>>>>>>>> constant propagation replaces 'b' with 2, and during expansion, > >>>>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - > >>>>>>>>> "argument 2 must be a constant immediate". > >>>>>>>>> > >>>>>>>>> So I guess we need some mechanism to mark a parameter as a > >>>>>>>>> constant ? > >>>>>>>> > >>>>>>>> I guess you want to mark it in a way that the frontend should force > >>>>>>>> constant evaluation and error if that's not possible? C++ doesn't > >>>>>>>> allow to declare a parameter as 'constexpr' but something like > >>>>>>>> > >>>>>>>> void foo (consteval int i); > >>>>>>>> > >>>>>>>> since I guess you do want to allow passing constexpr arguments > >>>>>>>> in C++ or in C extended forms of constants like > >>>>>>>> > >>>>>>>> static const int a[4]; > >>>>>>>> > >>>>>>>> foo (a[1]); > >>>>>>>> > >>>>>>>> ? But yes, this looks useful to me. > >>>>>>> Hi Richard, > >>>>>>> Thanks for the suggestions and sorry for late response. > >>>>>>> I have attached a prototype patch that implements consteval attribute. > >>>>>>> As implemented, the attribute takes at least one argument(s), which > >>>>>>> refer to parameter position, > >>>>>>> and the corresponding parameter must be const qualified, failing > >>>>>>> which, the attribute is ignored. > >>>>>> > >>>>>> I'm curious why the argument must be const-qualified. If it's > >>>>>> to keep it from being changed in ways that would prevent it from > >>>>>> being evaluated at compile-time in the body of the function then > >>>>>> to be effective, the enforcement of the constraint should be on > >>>>>> the definition of the function. Otherwise, the const qualifier > >>>>>> could be used in a declaration of a function but left out from > >>>>>> a subsequent definition of it, letting it modify it, like so: > >>>>>> > >>>>>> __attribute__ ((consteval (1))) void f (const int); > >>>>>> > >>>>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } > >>>>> > >>>>> In this particular case it's because the inline function is > >>>>> implementing an intrinsic operation in the architecture and the > >>>>> instruction only supports a literal constant value. At present we > >>>>> catch this while trying to expand the intrinsic, but that can lead to > >>>>> poor diagnostics because we really want to report against the line of > >>>>> code calling the intrinsic. > >>>> > >>>> Presumably the intrinsics can accept (or can be made to accept) any > >>>> constant integer expressions, not just literals. E.g., the aarch64 > >>>> builtin below accepts them. For example, this is accepted in C++: > >>>> > >>>> __Int64x2_t void f (__Int32x2_t a) > >>>> { > >>>> constexpr int n = 2; > >>>> return __builtin_aarch64_vshll_nv2si (a, n + 1); > >>>> } > >>>> > >>>> Making the intrinscis accept constant arguments in constexpr-like > >>>> functions and introducing a constexpr-lite attribute (for C code) > >>>> was what I was suggesting bythe constexpr comment below. I'd find > >>>> that a much more general and more powerful design. > >>>> > >>> > >>> Yes, it would be unfortunate if the rule made it impossible to avoid > >>> idiomatic const-exprs like those you would write in C++, or even those > >>> you'd write naturally in C: > >>> > >>> #define foo (1u << 5) > >>> > >>> > >>>> But my comment above was to highlight that if requiring the function > >>>> argument referenced by the proposed consteval attribute to be const > >>>> is necessary to prevent it from being modified then the requirement > >>>> needs to be enforced not on the declaration but on the definition. > >>>> > >>>> You may rightly say: "but we get to define the inline arm function > >>>> wrappers so we'll make sure to never declare them that way." I don't > >>>> have a problem with that. What I am saying is that a consteval > >>>> attribute that required the function argument to be declared const > >>>> to be effective would be flawed as a general-purpose feature without > >>>> enforcing the requirement on the definition. > >>> > >>> I'm not totally sure I understand what you're saying. But the point of > >>> putting the attribute on the declaration is to allow for reporting > >>> errors at the point of invocation, so if I call a function with an > >>> invalid 'must-be-const-expr' value, I'll get a diagnostic at the point > >>> in the source where that is done, not at the point in the (presumably > >>> inlined) function where the value ends up being used. Most of our > >>> builtins are wrapped into slightly more user-friendly function names so > >>> that they conform to the ACLE specification, yet ultimately map onto > >>> names into __builtin namespace per gcc's internal standards. If I have: > >>> > >>> __extension__ extern __inline int8x8_t > >>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > >>> vshr_n_s8 (int8x8_t __a, const int __b) > >>> { > >>> return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); > >>> } > >>> > >>> > >>> and then use that with > >>> > >>> extern int8x8_t x; > >>> extern int y; > >>> > >>> f() { > >>> int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. > >>> ... > >>> } > >>> > >>> I want the error to be reported on the line in f() where the problem > >>> really lies, not on the line in the inline function where the problem > >>> eventually manifests itself. > >> > >> Yes, I understand that. What I'm pointing out is that > >> the description of the attribute above > >> > >> "the attribute takes at least one argument(s), which refer to > >> parameter position, and the corresponding parameter must be const > >> qualified, failing which, the attribute is ignored." > >> > >> doesn't match the POC patch because it silently accepts the following > >> two declarations: > >> > >> int __attribute__ ((consteval (1))) > >> f (const int); // attribute consteval accepted... > >> > >> int f (int a) // ...even though the argument is not const > >> { > >> return ++a; > >> } > >> > >> The attribute handler should check that the const qualifier on > >> the argument is in the definition of the function. > >> > >> I'm also pointing out that from the POV of the user of the attribute, > >> the const shouldn't be necessary: the attribute consteval alone should > >> be sufficient. Hence my original question: why must the argument be > >> const-qualified? (Why can't you print equally good error messages > >> without it?) > > Hi Martin, > > While writing the POC patch, my expectation was to treat consteval > > param as a more "restricted version" > > of const param -- in addition to being readonly, it should only be > > passed compile-time constant > > values, and so I thought it probably made sense to require it to be > > const qualified. > > But if that doesn't seem like a good idea, we can drop it to be const qualified. > > I don't think depending on the const qualifier is necessary or > a good idea. > > > > > I was wondering how do we take this forward ? > > For a start would this be OK ? > > 1] Since we also need range info, define consteval attribute to take 3 > > arguments -- param-pos, min-val, max-val. > > and requiring the parameter to be an integral type (unlike the one > > implemented in POC patch). > > Specifying a range for a variable of any kind (including globals > and members) would be useful in general, independent of this > enhancement. I would suggest to introduce a separate attribute > for that. (With the effects of assigning out-of-range values > being undefined and a warning issued if/when detected.) Hi Martin, Sorry for late response. For vshl_n case, we need two constraints on the argument: (1) It has to be a constant. (2) The constant value has to be within the permissible range corresponding to the width. For eg, vshl_n_s32, accepts a constant value only between [0, 31] range. I agree that the range-info can be more generally useful beyond argument checking. So, do you think we should add two attributes: (1) consteval(param-pos), which just tells the compiler that the corresponding parameter expects an ICE (similar to POC patch). (2) range-info attribute for integral types similar to mode. For eg: typedef int __attribute__((range_info(0, 31))) S32; S32 x; // x is int with [0, 31] range So, vshl_n_s32 could be declared as: __extension__ extern __inline int32x2_t __attribute__ ((__always_inline__, __gnu_inline__, __artificial__, consteval(2))) vshl_n_s32 (int32x2_t __a, const S32 __b) { return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); } which would tell the compiler that 'b' needs to be constant within [0, 31] range, and emit an error otherwise during parsing. > > > 2] Diagnose a function call, if the corresponding argument (after > > invoking c_fully_fold) is not INTEGER_CST. > > > > My intent is to currently accept ICE's that are also acceptable in > > other language features like case statement. > > So something like this: > > __attribute__ ((always_inline, artificial, consteval (1))) > inline int f (int i) > { > switch (i) case i: return i; // or a built-in that expects > return 0; // a constant expression argument > } > > int g (void) { return f (7); } // valid > > but: > > int h (int i) { return f (i); } // invalid > > and a nice error for the call to h (as opposed to f): > > In function 'h': > error: argument 1 to 'f' is not a constant integer > note: 'f' declared with attribute 'consteval (1)' > > > As you suggest above, we could add "constexpr-lite" attribute to C, > > for accepting more complicated integral constant expressions, > > that can also be used in other language features that need ICE's. > > > > My concern with this approach tho, is that currently we accept ICE's > > for intrinsics and rely on the optimizer to resolve them into > > constants > > before expansion. > > So calls to the intrinsics don't compile without optimization? > (That's not what I saw in my very limited testing but I know > almost nothing about the ARM builtins so I could be missing > something.) I think it's only the case for _n intrinsics that require an ICE. The actual checking for constant currently happens in expand_builtin_args, in the backend during expansion, so the following function: int32x2_t f(int32_t *restrict a) { int32x2_t v = vld1_s32 (a); int b = 2; return vshl_n_s32 (v, b); } compiles at -O1+ because ccp turns 'b' into a constant, but fails at -O0 with following diagnostic: In file included from foo.c:1: In function ‘vshl_n_s32’, inlined from ‘f’ at foo.c:9:10: ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must be a constant immediate 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > So code that passes ICE to vshl_n, would get > > compiled at higher optimization levels but fail to compile with -O0. > > I am wondering if there's any existing code that relies on this > > behavior and will fail to compile if we impose the above restriction ? > > I don't know. > > But as it turns out, an argument to a C++ constexpr function isn't > considered a constant expression within the body of the function > even if it is one at the call site. So the extension you describe > would also apply to constexpr functions: > > __attribute__ ((consteval (1))) > constexpr int f (int i) > { > switch (i) case i: return i; // potentially valid with consteval > return 0; > } > > constexpr int x = f (1); // valid with consteval (not without) > int y = f (y); // not valid I guess that would be a more useful extension, but I was merely suggesting, to add constexpr to C for computing more complex ICE's. For eg: int a = 1; int b = a + 1; switch() { case b: ... } // error With constexpr: constexpr int a = 1; constexpr int b = a + 1; switch() { case b: ... } // OK Thanks, Prathamesh > > Martin > > > > > Thanks, > > Prathamesh > > > > > >> > >> Martin > >> > >>> > >>> R. > >>> > >>>> > >>>> Martin > >>>> > >>>> > >>>>> > >>>>> R. > >>>>>> > >>>>>> That said, if compile-time function evaluation is the goal then > >>>>>> a fully general solution is an attribute that applies to the whole > >>>>>> function, not just a subset of its arguments. That way arguments > >>>>>> can also be assigned to local variables within the function that > >>>>>> can then be modified while still evaluated at compile time and > >>>>>> used where constant expressions are expected. I.e., the design > >>>>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger > >>>>>> project.) > >>>>>> > >>>>>> A few notes on the prototype patch: conventionally GCC warnings > >>>>>> about attributes do not mention when an attribute is ignored. > >>>>>> It may be a nice touch to add to all of them but I'd recommend > >>>>>> against doing that in individual handlers. Since the attribute > >>>>>> allows pointer constants the warning issued when an argument is > >>>>>> not one should be generalized (i.e., not refer to just integer > >>>>>> constants). > >>>>>> > >>>>>> (Other than that, C/C++ warnings should start in lowercase and > >>>>>> not end in a period). > >>>>>> > >>>>>> Martin > >>>>>> > >>>>>>> > >>>>>>> The patch does type-checking for arguments in > >>>>>>> check_function_consteval_attr, which > >>>>>>> simply does a linear search to see if the corresponding argument > >>>>>>> number is included in consteval attribute, > >>>>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. > >>>>>>> > >>>>>>> This works for simple constants and the intrinsics use-case, but > >>>>>>> rejects "extended constants" as in your above example. > >>>>>>> I guess we can special case to detect cases like a[i] where 'a' is > >>>>>>> const and 'i' is an immediate, > >>>>>>> but I am not sure if there's a way to force constant evaluation in > >>>>>>> FE ? > >>>>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > >>>>>>> didn't seem to work. > >>>>>>> Do you have any suggestions on how to detect those in the front-end ? > >>>>>>> > >>>>>>> Example: > >>>>>>> > >>>>>>> __attribute__((consteval(1, 2))) > >>>>>>> void f(const int x, int *p) > >>>>>>> {} > >>>>>>> > >>>>>>> Calling it with: > >>>>>>> 1) f(0, (int *) 0) is accepted. > >>>>>>> > >>>>>>> 2) > >>>>>>> void g(int *p) > >>>>>>> { > >>>>>>> f (0, p); > >>>>>>> } > >>>>>>> > >>>>>>> emits the following error: > >>>>>>> test.c: In function ‘g’: > >>>>>>> test.c:7:9: error: argument 2 is not a constant. > >>>>>>> 7 | f (0, p); > >>>>>>> | ^ > >>>>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval > >>>>>>> attribute. > >>>>>>> 2 | void f(const int x, int *const p) > >>>>>>> | ^ > >>>>>>> > >>>>>>> Thanks, > >>>>>>> Prathamesh > >>>>>>>> > >>>>>>>> Richard. > >>>>>>>> > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> Prathamesh > >>>>>>>>>> > >>>>>>>>>> Thanks, > >>>>>>>>>> Andrew Pinski > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require > >>>>>>>>>>> that the > >>>>>>>>>>> second arg (__b), > >>>>>>>>>>> should be an immediate value. > >>>>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, > >>>>>>>>>>> and if > >>>>>>>>>>> a non-constant > >>>>>>>>>>> value gets passed, it emits the following diagnostic: > >>>>>>>>>>> > >>>>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument > >>>>>>>>>>> 2 must > >>>>>>>>>>> be a constant immediate > >>>>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>>>>>>>>>> > >>>>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector > >>>>>>>>>>> extensions where > >>>>>>>>>>> possible (PR66791), because the builtins are opaque to the > >>>>>>>>>>> optimizers. > >>>>>>>>>>> > >>>>>>>>>>> Unfortunately, we lose type checking of immediate value if we > >>>>>>>>>>> replace > >>>>>>>>>>> the builtin > >>>>>>>>>>> with << operator: > >>>>>>>>>>> > >>>>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>>>> __artificial__)) > >>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>>>> { > >>>>>>>>>>> return __a << __b; > >>>>>>>>>>> } > >>>>>>>>>>> > >>>>>>>>>>> So, I was wondering if we should have an attribute for a > >>>>>>>>>>> parameter to > >>>>>>>>>>> specifically > >>>>>>>>>>> mark it as a constant value with optional range value info ? > >>>>>>>>>>> As Richard suggested, sth like: > >>>>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, > >>>>>>>>>>> max_val))); > >>>>>>>>>>> > >>>>>>>>>>> Thanks, > >>>>>>>>>>> Prathamesh > >>>>>> > >>>> > >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-18 6:52 ` Prathamesh Kulkarni @ 2021-08-18 14:40 ` Martin Sebor 2021-08-19 8:10 ` Prathamesh Kulkarni 0 siblings, 1 reply; 31+ messages in thread From: Martin Sebor @ 2021-08-18 14:40 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Earnshaw, Richard Biener, GCC Development On 8/18/21 12:52 AM, Prathamesh Kulkarni wrote: > On Fri, 13 Aug 2021 at 22:44, Martin Sebor <msebor@gmail.com> wrote: >> >> On 8/12/21 2:32 AM, Prathamesh Kulkarni wrote: >>> On Sat, 7 Aug 2021 at 02:09, Martin Sebor <msebor@gmail.com> wrote: >>>> >>>> On 8/6/21 4:51 AM, Richard Earnshaw wrote: >>>>> >>>>> >>>>> On 06/08/2021 01:06, Martin Sebor via Gcc wrote: >>>>>> On 8/4/21 3:46 AM, Richard Earnshaw wrote: >>>>>>> >>>>>>> >>>>>>> On 03/08/2021 18:44, Martin Sebor wrote: >>>>>>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: >>>>>>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener >>>>>>>>> <richard.guenther@gmail.com> wrote: >>>>>>>>>> >>>>>>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc >>>>>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>>>>> >>>>>>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> >>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc >>>>>>>>>>>> <gcc@gcc.gnu.org> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>> Hi, >>>>>>>>>>>>> Continuing from this thread, >>>>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html >>>>>>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a >>>>>>>>>>>>> function as a literal constant. >>>>>>>>>>>>> >>>>>>>>>>>>> Motivation: >>>>>>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: >>>>>>>>>>>>> >>>>>>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>>>>>> __artificial__)) >>>>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>>>>>> { >>>>>>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> and it's caller: >>>>>>>>>>>>> >>>>>>>>>>>>> int32x2_t f (int32x2_t x) >>>>>>>>>>>>> { >>>>>>>>>>>>> return vshl_n_s32 (x, 1); >>>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> Can't you do similar to what is done already in the aarch64 >>>>>>>>>>>> back-end: >>>>>>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) >>>>>>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ >>>>>>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), >>>>>>>>>>>> sizeof(__vec[0]), __idx) >>>>>>>>>>>> >>>>>>>>>>>> ? >>>>>>>>>>>> Yes this is about lanes but you could even add one for min/max >>>>>>>>>>>> which >>>>>>>>>>>> is generic and such; add an argument to say the intrinsics name >>>>>>>>>>>> even. >>>>>>>>>>>> You could do this as a non-target builtin if you want and reuse it >>>>>>>>>>>> also for the aarch64 backend. >>>>>>>>>>> Hi Andrew, >>>>>>>>>>> Thanks for the suggestions. IIUC, we could use this approach to >>>>>>>>>>> check >>>>>>>>>>> if the argument >>>>>>>>>>> falls within a certain range (min / max), but I am not sure how it >>>>>>>>>>> will help to determine >>>>>>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics >>>>>>>>>>> require >>>>>>>>>>> that the 2nd arg is immediate ? >>>>>>>>>>> >>>>>>>>>>> Even the current RTL builtin checking is not consistent across >>>>>>>>>>> optimization levels: >>>>>>>>>>> For eg: >>>>>>>>>>> int32x2_t f(int32_t *restrict a) >>>>>>>>>>> { >>>>>>>>>>> int32x2_t v = vld1_s32 (a); >>>>>>>>>>> int b = 2; >>>>>>>>>>> return vshl_n_s32 (v, b); >>>>>>>>>>> } >>>>>>>>>>> >>>>>>>>>>> With pristine trunk, compiling with -O2 results in no errors because >>>>>>>>>>> constant propagation replaces 'b' with 2, and during expansion, >>>>>>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - >>>>>>>>>>> "argument 2 must be a constant immediate". >>>>>>>>>>> >>>>>>>>>>> So I guess we need some mechanism to mark a parameter as a >>>>>>>>>>> constant ? >>>>>>>>>> >>>>>>>>>> I guess you want to mark it in a way that the frontend should force >>>>>>>>>> constant evaluation and error if that's not possible? C++ doesn't >>>>>>>>>> allow to declare a parameter as 'constexpr' but something like >>>>>>>>>> >>>>>>>>>> void foo (consteval int i); >>>>>>>>>> >>>>>>>>>> since I guess you do want to allow passing constexpr arguments >>>>>>>>>> in C++ or in C extended forms of constants like >>>>>>>>>> >>>>>>>>>> static const int a[4]; >>>>>>>>>> >>>>>>>>>> foo (a[1]); >>>>>>>>>> >>>>>>>>>> ? But yes, this looks useful to me. >>>>>>>>> Hi Richard, >>>>>>>>> Thanks for the suggestions and sorry for late response. >>>>>>>>> I have attached a prototype patch that implements consteval attribute. >>>>>>>>> As implemented, the attribute takes at least one argument(s), which >>>>>>>>> refer to parameter position, >>>>>>>>> and the corresponding parameter must be const qualified, failing >>>>>>>>> which, the attribute is ignored. >>>>>>>> >>>>>>>> I'm curious why the argument must be const-qualified. If it's >>>>>>>> to keep it from being changed in ways that would prevent it from >>>>>>>> being evaluated at compile-time in the body of the function then >>>>>>>> to be effective, the enforcement of the constraint should be on >>>>>>>> the definition of the function. Otherwise, the const qualifier >>>>>>>> could be used in a declaration of a function but left out from >>>>>>>> a subsequent definition of it, letting it modify it, like so: >>>>>>>> >>>>>>>> __attribute__ ((consteval (1))) void f (const int); >>>>>>>> >>>>>>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } >>>>>>> >>>>>>> In this particular case it's because the inline function is >>>>>>> implementing an intrinsic operation in the architecture and the >>>>>>> instruction only supports a literal constant value. At present we >>>>>>> catch this while trying to expand the intrinsic, but that can lead to >>>>>>> poor diagnostics because we really want to report against the line of >>>>>>> code calling the intrinsic. >>>>>> >>>>>> Presumably the intrinsics can accept (or can be made to accept) any >>>>>> constant integer expressions, not just literals. E.g., the aarch64 >>>>>> builtin below accepts them. For example, this is accepted in C++: >>>>>> >>>>>> __Int64x2_t void f (__Int32x2_t a) >>>>>> { >>>>>> constexpr int n = 2; >>>>>> return __builtin_aarch64_vshll_nv2si (a, n + 1); >>>>>> } >>>>>> >>>>>> Making the intrinscis accept constant arguments in constexpr-like >>>>>> functions and introducing a constexpr-lite attribute (for C code) >>>>>> was what I was suggesting bythe constexpr comment below. I'd find >>>>>> that a much more general and more powerful design. >>>>>> >>>>> >>>>> Yes, it would be unfortunate if the rule made it impossible to avoid >>>>> idiomatic const-exprs like those you would write in C++, or even those >>>>> you'd write naturally in C: >>>>> >>>>> #define foo (1u << 5) >>>>> >>>>> >>>>>> But my comment above was to highlight that if requiring the function >>>>>> argument referenced by the proposed consteval attribute to be const >>>>>> is necessary to prevent it from being modified then the requirement >>>>>> needs to be enforced not on the declaration but on the definition. >>>>>> >>>>>> You may rightly say: "but we get to define the inline arm function >>>>>> wrappers so we'll make sure to never declare them that way." I don't >>>>>> have a problem with that. What I am saying is that a consteval >>>>>> attribute that required the function argument to be declared const >>>>>> to be effective would be flawed as a general-purpose feature without >>>>>> enforcing the requirement on the definition. >>>>> >>>>> I'm not totally sure I understand what you're saying. But the point of >>>>> putting the attribute on the declaration is to allow for reporting >>>>> errors at the point of invocation, so if I call a function with an >>>>> invalid 'must-be-const-expr' value, I'll get a diagnostic at the point >>>>> in the source where that is done, not at the point in the (presumably >>>>> inlined) function where the value ends up being used. Most of our >>>>> builtins are wrapped into slightly more user-friendly function names so >>>>> that they conform to the ACLE specification, yet ultimately map onto >>>>> names into __builtin namespace per gcc's internal standards. If I have: >>>>> >>>>> __extension__ extern __inline int8x8_t >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >>>>> vshr_n_s8 (int8x8_t __a, const int __b) >>>>> { >>>>> return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); >>>>> } >>>>> >>>>> >>>>> and then use that with >>>>> >>>>> extern int8x8_t x; >>>>> extern int y; >>>>> >>>>> f() { >>>>> int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. >>>>> ... >>>>> } >>>>> >>>>> I want the error to be reported on the line in f() where the problem >>>>> really lies, not on the line in the inline function where the problem >>>>> eventually manifests itself. >>>> >>>> Yes, I understand that. What I'm pointing out is that >>>> the description of the attribute above >>>> >>>> "the attribute takes at least one argument(s), which refer to >>>> parameter position, and the corresponding parameter must be const >>>> qualified, failing which, the attribute is ignored." >>>> >>>> doesn't match the POC patch because it silently accepts the following >>>> two declarations: >>>> >>>> int __attribute__ ((consteval (1))) >>>> f (const int); // attribute consteval accepted... >>>> >>>> int f (int a) // ...even though the argument is not const >>>> { >>>> return ++a; >>>> } >>>> >>>> The attribute handler should check that the const qualifier on >>>> the argument is in the definition of the function. >>>> >>>> I'm also pointing out that from the POV of the user of the attribute, >>>> the const shouldn't be necessary: the attribute consteval alone should >>>> be sufficient. Hence my original question: why must the argument be >>>> const-qualified? (Why can't you print equally good error messages >>>> without it?) >>> Hi Martin, >>> While writing the POC patch, my expectation was to treat consteval >>> param as a more "restricted version" >>> of const param -- in addition to being readonly, it should only be >>> passed compile-time constant >>> values, and so I thought it probably made sense to require it to be >>> const qualified. >>> But if that doesn't seem like a good idea, we can drop it to be const qualified. >> >> I don't think depending on the const qualifier is necessary or >> a good idea. >> >>> >>> I was wondering how do we take this forward ? >>> For a start would this be OK ? >>> 1] Since we also need range info, define consteval attribute to take 3 >>> arguments -- param-pos, min-val, max-val. >>> and requiring the parameter to be an integral type (unlike the one >>> implemented in POC patch). >> >> Specifying a range for a variable of any kind (including globals >> and members) would be useful in general, independent of this >> enhancement. I would suggest to introduce a separate attribute >> for that. (With the effects of assigning out-of-range values >> being undefined and a warning issued if/when detected.) > Hi Martin, > Sorry for late response. > > For vshl_n case, we need two constraints on the argument: > (1) It has to be a constant. > (2) The constant value has to be within the permissible range > corresponding to the width. > For eg, vshl_n_s32, accepts a constant value only between [0, 31] range. > > I agree that the range-info can be more generally useful beyond > argument checking. > So, do you think we should add two attributes: > (1) consteval(param-pos), which just tells the compiler that the > corresponding parameter > expects an ICE (similar to POC patch). > > (2) range-info attribute for integral types similar to mode. > For eg: > typedef int __attribute__((range_info(0, 31))) S32; > S32 x; // x is int with [0, 31] range > > So, vshl_n_s32 could be declared as: > > __extension__ extern __inline int32x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__, > consteval(2))) > vshl_n_s32 (int32x2_t __a, const S32 __b) > { > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > } > > which would tell the compiler that 'b' needs to be constant within [0, > 31] range, > and emit an error otherwise during parsing. Yes, that's what I was thinking (without the const qualifier). The range_info attribute (I'd suggest naming it value_range) could be used on parameters as well to avoid having to introduce typedefs: __attribute__ ((..., consteval(2))) vshl_n_s32 (int32x2_t a, int b __attribute__ ((value_range (0, 31))) { ... } (I think accepting the attribute on types as in your example would be useful as well, as would be accepting multiple attributes to represent multi-ranges). >> >>> 2] Diagnose a function call, if the corresponding argument (after >>> invoking c_fully_fold) is not INTEGER_CST. >>> >>> My intent is to currently accept ICE's that are also acceptable in >>> other language features like case statement. >> >> So something like this: >> >> __attribute__ ((always_inline, artificial, consteval (1))) >> inline int f (int i) >> { >> switch (i) case i: return i; // or a built-in that expects >> return 0; // a constant expression argument >> } >> >> int g (void) { return f (7); } // valid >> >> but: >> >> int h (int i) { return f (i); } // invalid >> >> and a nice error for the call to h (as opposed to f): >> >> In function 'h': >> error: argument 1 to 'f' is not a constant integer >> note: 'f' declared with attribute 'consteval (1)' >> >>> As you suggest above, we could add "constexpr-lite" attribute to C, >>> for accepting more complicated integral constant expressions, >>> that can also be used in other language features that need ICE's. >>> >>> My concern with this approach tho, is that currently we accept ICE's >>> for intrinsics and rely on the optimizer to resolve them into >>> constants >>> before expansion. >> >> So calls to the intrinsics don't compile without optimization? >> (That's not what I saw in my very limited testing but I know >> almost nothing about the ARM builtins so I could be missing >> something.) > I think it's only the case for _n intrinsics that require an ICE. > The actual checking for constant currently happens in > expand_builtin_args, in the backend during expansion, > so the following function: > > int32x2_t f(int32_t *restrict a) > { > int32x2_t v = vld1_s32 (a); > int b = 2; > return vshl_n_s32 (v, b); > } > > compiles at -O1+ because ccp turns 'b' into a constant, but fails at > -O0 with following diagnostic: > In file included from foo.c:1: > In function ‘vshl_n_s32’, > inlined from ‘f’ at foo.c:9:10: > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > be a constant immediate > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ I meant simply this: vshl_n_s32 (v, 2); With vshl_n_s32() an inline wrapper around the __builtin, I'd expect the call not to compile for the reason we're discussing (i.e., it's not an constant integer expression). >> >>> So code that passes ICE to vshl_n, would get >>> compiled at higher optimization levels but fail to compile with -O0. >>> I am wondering if there's any existing code that relies on this >>> behavior and will fail to compile if we impose the above restriction ? >> >> I don't know. >> >> But as it turns out, an argument to a C++ constexpr function isn't >> considered a constant expression within the body of the function >> even if it is one at the call site. So the extension you describe >> would also apply to constexpr functions: >> >> __attribute__ ((consteval (1))) >> constexpr int f (int i) >> { >> switch (i) case i: return i; // potentially valid with consteval >> return 0; >> } >> >> constexpr int x = f (1); // valid with consteval (not without) >> int y = f (y); // not valid > I guess that would be a more useful extension, but I was merely suggesting, > to add constexpr to C for computing more complex ICE's. > For eg: > int a = 1; > int b = a + 1; > > switch() { case b: ... } // error > > With constexpr: > constexpr int a = 1; > constexpr int b = a + 1; > > switch() { case b: ... } // OK I was just noting that the consteval attribute would be useful even in C++ where a constant integer expression argument of a constexpr function isn't considered constexpr. (Function arguments can't be declared constexpr in C++ as far as I can tell.) A "constexpr" for variables only (or attribute consteval) be useful too in C. That would suggest making it a variable attribute rather than a positional function attribute. FWIW, there was a proposal for C2x along these lines some time ago: http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2067.pdf AFAIK, it didn't go anywhere in WG14 for a variety of reasons but the basic idea behind it was a good one. Martin > > Thanks, > Prathamesh >> >> Martin >> >>> >>> Thanks, >>> Prathamesh >>> >>> >>>> >>>> Martin >>>> >>>>> >>>>> R. >>>>> >>>>>> >>>>>> Martin >>>>>> >>>>>> >>>>>>> >>>>>>> R. >>>>>>>> >>>>>>>> That said, if compile-time function evaluation is the goal then >>>>>>>> a fully general solution is an attribute that applies to the whole >>>>>>>> function, not just a subset of its arguments. That way arguments >>>>>>>> can also be assigned to local variables within the function that >>>>>>>> can then be modified while still evaluated at compile time and >>>>>>>> used where constant expressions are expected. I.e., the design >>>>>>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger >>>>>>>> project.) >>>>>>>> >>>>>>>> A few notes on the prototype patch: conventionally GCC warnings >>>>>>>> about attributes do not mention when an attribute is ignored. >>>>>>>> It may be a nice touch to add to all of them but I'd recommend >>>>>>>> against doing that in individual handlers. Since the attribute >>>>>>>> allows pointer constants the warning issued when an argument is >>>>>>>> not one should be generalized (i.e., not refer to just integer >>>>>>>> constants). >>>>>>>> >>>>>>>> (Other than that, C/C++ warnings should start in lowercase and >>>>>>>> not end in a period). >>>>>>>> >>>>>>>> Martin >>>>>>>> >>>>>>>>> >>>>>>>>> The patch does type-checking for arguments in >>>>>>>>> check_function_consteval_attr, which >>>>>>>>> simply does a linear search to see if the corresponding argument >>>>>>>>> number is included in consteval attribute, >>>>>>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. >>>>>>>>> >>>>>>>>> This works for simple constants and the intrinsics use-case, but >>>>>>>>> rejects "extended constants" as in your above example. >>>>>>>>> I guess we can special case to detect cases like a[i] where 'a' is >>>>>>>>> const and 'i' is an immediate, >>>>>>>>> but I am not sure if there's a way to force constant evaluation in >>>>>>>>> FE ? >>>>>>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that >>>>>>>>> didn't seem to work. >>>>>>>>> Do you have any suggestions on how to detect those in the front-end ? >>>>>>>>> >>>>>>>>> Example: >>>>>>>>> >>>>>>>>> __attribute__((consteval(1, 2))) >>>>>>>>> void f(const int x, int *p) >>>>>>>>> {} >>>>>>>>> >>>>>>>>> Calling it with: >>>>>>>>> 1) f(0, (int *) 0) is accepted. >>>>>>>>> >>>>>>>>> 2) >>>>>>>>> void g(int *p) >>>>>>>>> { >>>>>>>>> f (0, p); >>>>>>>>> } >>>>>>>>> >>>>>>>>> emits the following error: >>>>>>>>> test.c: In function ‘g’: >>>>>>>>> test.c:7:9: error: argument 2 is not a constant. >>>>>>>>> 7 | f (0, p); >>>>>>>>> | ^ >>>>>>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval >>>>>>>>> attribute. >>>>>>>>> 2 | void f(const int x, int *const p) >>>>>>>>> | ^ >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Prathamesh >>>>>>>>>> >>>>>>>>>> Richard. >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Thanks, >>>>>>>>>>> Prathamesh >>>>>>>>>>>> >>>>>>>>>>>> Thanks, >>>>>>>>>>>> Andrew Pinski >>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require >>>>>>>>>>>>> that the >>>>>>>>>>>>> second arg (__b), >>>>>>>>>>>>> should be an immediate value. >>>>>>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, >>>>>>>>>>>>> and if >>>>>>>>>>>>> a non-constant >>>>>>>>>>>>> value gets passed, it emits the following diagnostic: >>>>>>>>>>>>> >>>>>>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument >>>>>>>>>>>>> 2 must >>>>>>>>>>>>> be a constant immediate >>>>>>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); >>>>>>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>>>>>>>>>>> >>>>>>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector >>>>>>>>>>>>> extensions where >>>>>>>>>>>>> possible (PR66791), because the builtins are opaque to the >>>>>>>>>>>>> optimizers. >>>>>>>>>>>>> >>>>>>>>>>>>> Unfortunately, we lose type checking of immediate value if we >>>>>>>>>>>>> replace >>>>>>>>>>>>> the builtin >>>>>>>>>>>>> with << operator: >>>>>>>>>>>>> >>>>>>>>>>>>> __extension__ extern __inline int32x2_t >>>>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, >>>>>>>>>>>>> __artificial__)) >>>>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) >>>>>>>>>>>>> { >>>>>>>>>>>>> return __a << __b; >>>>>>>>>>>>> } >>>>>>>>>>>>> >>>>>>>>>>>>> So, I was wondering if we should have an attribute for a >>>>>>>>>>>>> parameter to >>>>>>>>>>>>> specifically >>>>>>>>>>>>> mark it as a constant value with optional range value info ? >>>>>>>>>>>>> As Richard suggested, sth like: >>>>>>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, >>>>>>>>>>>>> max_val))); >>>>>>>>>>>>> >>>>>>>>>>>>> Thanks, >>>>>>>>>>>>> Prathamesh >>>>>>>> >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-18 14:40 ` Martin Sebor @ 2021-08-19 8:10 ` Prathamesh Kulkarni 0 siblings, 0 replies; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-19 8:10 UTC (permalink / raw) To: Martin Sebor; +Cc: Richard Earnshaw, Richard Biener, GCC Development On Wed, 18 Aug 2021 at 20:10, Martin Sebor <msebor@gmail.com> wrote: > > On 8/18/21 12:52 AM, Prathamesh Kulkarni wrote: > > On Fri, 13 Aug 2021 at 22:44, Martin Sebor <msebor@gmail.com> wrote: > >> > >> On 8/12/21 2:32 AM, Prathamesh Kulkarni wrote: > >>> On Sat, 7 Aug 2021 at 02:09, Martin Sebor <msebor@gmail.com> wrote: > >>>> > >>>> On 8/6/21 4:51 AM, Richard Earnshaw wrote: > >>>>> > >>>>> > >>>>> On 06/08/2021 01:06, Martin Sebor via Gcc wrote: > >>>>>> On 8/4/21 3:46 AM, Richard Earnshaw wrote: > >>>>>>> > >>>>>>> > >>>>>>> On 03/08/2021 18:44, Martin Sebor wrote: > >>>>>>>> On 8/3/21 4:11 AM, Prathamesh Kulkarni via Gcc wrote: > >>>>>>>>> On Tue, 27 Jul 2021 at 13:49, Richard Biener > >>>>>>>>> <richard.guenther@gmail.com> wrote: > >>>>>>>>>> > >>>>>>>>>> On Mon, Jul 26, 2021 at 11:06 AM Prathamesh Kulkarni via Gcc > >>>>>>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>>>>>> > >>>>>>>>>>> On Fri, 23 Jul 2021 at 23:29, Andrew Pinski <pinskia@gmail.com> > >>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>> On Fri, Jul 23, 2021 at 3:55 AM Prathamesh Kulkarni via Gcc > >>>>>>>>>>>> <gcc@gcc.gnu.org> wrote: > >>>>>>>>>>>>> > >>>>>>>>>>>>> Hi, > >>>>>>>>>>>>> Continuing from this thread, > >>>>>>>>>>>>> https://gcc.gnu.org/pipermail/gcc-patches/2021-July/575920.html > >>>>>>>>>>>>> The proposal is to provide a mechanism to mark a parameter in a > >>>>>>>>>>>>> function as a literal constant. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Motivation: > >>>>>>>>>>>>> Consider the following intrinsic vshl_n_s32 from arrm/arm_neon.h: > >>>>>>>>>>>>> > >>>>>>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>>>>>> __artificial__)) > >>>>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>>>>>> { > >>>>>>>>>>>>> return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>>>>>> } > >>>>>>>>>>>>> > >>>>>>>>>>>>> and it's caller: > >>>>>>>>>>>>> > >>>>>>>>>>>>> int32x2_t f (int32x2_t x) > >>>>>>>>>>>>> { > >>>>>>>>>>>>> return vshl_n_s32 (x, 1); > >>>>>>>>>>>>> } > >>>>>>>>>>>> > >>>>>>>>>>>> Can't you do similar to what is done already in the aarch64 > >>>>>>>>>>>> back-end: > >>>>>>>>>>>> #define __AARCH64_NUM_LANES(__v) (sizeof (__v) / sizeof (__v[0])) > >>>>>>>>>>>> #define __AARCH64_LANE_CHECK(__vec, __idx) \ > >>>>>>>>>>>> __builtin_aarch64_im_lane_boundsi (sizeof(__vec), > >>>>>>>>>>>> sizeof(__vec[0]), __idx) > >>>>>>>>>>>> > >>>>>>>>>>>> ? > >>>>>>>>>>>> Yes this is about lanes but you could even add one for min/max > >>>>>>>>>>>> which > >>>>>>>>>>>> is generic and such; add an argument to say the intrinsics name > >>>>>>>>>>>> even. > >>>>>>>>>>>> You could do this as a non-target builtin if you want and reuse it > >>>>>>>>>>>> also for the aarch64 backend. > >>>>>>>>>>> Hi Andrew, > >>>>>>>>>>> Thanks for the suggestions. IIUC, we could use this approach to > >>>>>>>>>>> check > >>>>>>>>>>> if the argument > >>>>>>>>>>> falls within a certain range (min / max), but I am not sure how it > >>>>>>>>>>> will help to determine > >>>>>>>>>>> if the arg is a constant immediate ? AFAIK, vshl_n intrinsics > >>>>>>>>>>> require > >>>>>>>>>>> that the 2nd arg is immediate ? > >>>>>>>>>>> > >>>>>>>>>>> Even the current RTL builtin checking is not consistent across > >>>>>>>>>>> optimization levels: > >>>>>>>>>>> For eg: > >>>>>>>>>>> int32x2_t f(int32_t *restrict a) > >>>>>>>>>>> { > >>>>>>>>>>> int32x2_t v = vld1_s32 (a); > >>>>>>>>>>> int b = 2; > >>>>>>>>>>> return vshl_n_s32 (v, b); > >>>>>>>>>>> } > >>>>>>>>>>> > >>>>>>>>>>> With pristine trunk, compiling with -O2 results in no errors because > >>>>>>>>>>> constant propagation replaces 'b' with 2, and during expansion, > >>>>>>>>>>> expand_builtin_args is happy. But at -O0, it results in the error - > >>>>>>>>>>> "argument 2 must be a constant immediate". > >>>>>>>>>>> > >>>>>>>>>>> So I guess we need some mechanism to mark a parameter as a > >>>>>>>>>>> constant ? > >>>>>>>>>> > >>>>>>>>>> I guess you want to mark it in a way that the frontend should force > >>>>>>>>>> constant evaluation and error if that's not possible? C++ doesn't > >>>>>>>>>> allow to declare a parameter as 'constexpr' but something like > >>>>>>>>>> > >>>>>>>>>> void foo (consteval int i); > >>>>>>>>>> > >>>>>>>>>> since I guess you do want to allow passing constexpr arguments > >>>>>>>>>> in C++ or in C extended forms of constants like > >>>>>>>>>> > >>>>>>>>>> static const int a[4]; > >>>>>>>>>> > >>>>>>>>>> foo (a[1]); > >>>>>>>>>> > >>>>>>>>>> ? But yes, this looks useful to me. > >>>>>>>>> Hi Richard, > >>>>>>>>> Thanks for the suggestions and sorry for late response. > >>>>>>>>> I have attached a prototype patch that implements consteval attribute. > >>>>>>>>> As implemented, the attribute takes at least one argument(s), which > >>>>>>>>> refer to parameter position, > >>>>>>>>> and the corresponding parameter must be const qualified, failing > >>>>>>>>> which, the attribute is ignored. > >>>>>>>> > >>>>>>>> I'm curious why the argument must be const-qualified. If it's > >>>>>>>> to keep it from being changed in ways that would prevent it from > >>>>>>>> being evaluated at compile-time in the body of the function then > >>>>>>>> to be effective, the enforcement of the constraint should be on > >>>>>>>> the definition of the function. Otherwise, the const qualifier > >>>>>>>> could be used in a declaration of a function but left out from > >>>>>>>> a subsequent definition of it, letting it modify it, like so: > >>>>>>>> > >>>>>>>> __attribute__ ((consteval (1))) void f (const int); > >>>>>>>> > >>>>>>>> inline __attribute__ ((always_inline)) void f (int i) { ++i; } > >>>>>>> > >>>>>>> In this particular case it's because the inline function is > >>>>>>> implementing an intrinsic operation in the architecture and the > >>>>>>> instruction only supports a literal constant value. At present we > >>>>>>> catch this while trying to expand the intrinsic, but that can lead to > >>>>>>> poor diagnostics because we really want to report against the line of > >>>>>>> code calling the intrinsic. > >>>>>> > >>>>>> Presumably the intrinsics can accept (or can be made to accept) any > >>>>>> constant integer expressions, not just literals. E.g., the aarch64 > >>>>>> builtin below accepts them. For example, this is accepted in C++: > >>>>>> > >>>>>> __Int64x2_t void f (__Int32x2_t a) > >>>>>> { > >>>>>> constexpr int n = 2; > >>>>>> return __builtin_aarch64_vshll_nv2si (a, n + 1); > >>>>>> } > >>>>>> > >>>>>> Making the intrinscis accept constant arguments in constexpr-like > >>>>>> functions and introducing a constexpr-lite attribute (for C code) > >>>>>> was what I was suggesting bythe constexpr comment below. I'd find > >>>>>> that a much more general and more powerful design. > >>>>>> > >>>>> > >>>>> Yes, it would be unfortunate if the rule made it impossible to avoid > >>>>> idiomatic const-exprs like those you would write in C++, or even those > >>>>> you'd write naturally in C: > >>>>> > >>>>> #define foo (1u << 5) > >>>>> > >>>>> > >>>>>> But my comment above was to highlight that if requiring the function > >>>>>> argument referenced by the proposed consteval attribute to be const > >>>>>> is necessary to prevent it from being modified then the requirement > >>>>>> needs to be enforced not on the declaration but on the definition. > >>>>>> > >>>>>> You may rightly say: "but we get to define the inline arm function > >>>>>> wrappers so we'll make sure to never declare them that way." I don't > >>>>>> have a problem with that. What I am saying is that a consteval > >>>>>> attribute that required the function argument to be declared const > >>>>>> to be effective would be flawed as a general-purpose feature without > >>>>>> enforcing the requirement on the definition. > >>>>> > >>>>> I'm not totally sure I understand what you're saying. But the point of > >>>>> putting the attribute on the declaration is to allow for reporting > >>>>> errors at the point of invocation, so if I call a function with an > >>>>> invalid 'must-be-const-expr' value, I'll get a diagnostic at the point > >>>>> in the source where that is done, not at the point in the (presumably > >>>>> inlined) function where the value ends up being used. Most of our > >>>>> builtins are wrapped into slightly more user-friendly function names so > >>>>> that they conform to the ACLE specification, yet ultimately map onto > >>>>> names into __builtin namespace per gcc's internal standards. If I have: > >>>>> > >>>>> __extension__ extern __inline int8x8_t > >>>>> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > >>>>> vshr_n_s8 (int8x8_t __a, const int __b) > >>>>> { > >>>>> return (int8x8_t)__builtin_neon_vshrs_nv8qi (__a, __b); > >>>>> } > >>>>> > >>>>> > >>>>> and then use that with > >>>>> > >>>>> extern int8x8_t x; > >>>>> extern int y; > >>>>> > >>>>> f() { > >>>>> int8x8_t z = vshr_n_s8 (x, y); // Error y must be a const int expr. > >>>>> ... > >>>>> } > >>>>> > >>>>> I want the error to be reported on the line in f() where the problem > >>>>> really lies, not on the line in the inline function where the problem > >>>>> eventually manifests itself. > >>>> > >>>> Yes, I understand that. What I'm pointing out is that > >>>> the description of the attribute above > >>>> > >>>> "the attribute takes at least one argument(s), which refer to > >>>> parameter position, and the corresponding parameter must be const > >>>> qualified, failing which, the attribute is ignored." > >>>> > >>>> doesn't match the POC patch because it silently accepts the following > >>>> two declarations: > >>>> > >>>> int __attribute__ ((consteval (1))) > >>>> f (const int); // attribute consteval accepted... > >>>> > >>>> int f (int a) // ...even though the argument is not const > >>>> { > >>>> return ++a; > >>>> } > >>>> > >>>> The attribute handler should check that the const qualifier on > >>>> the argument is in the definition of the function. > >>>> > >>>> I'm also pointing out that from the POV of the user of the attribute, > >>>> the const shouldn't be necessary: the attribute consteval alone should > >>>> be sufficient. Hence my original question: why must the argument be > >>>> const-qualified? (Why can't you print equally good error messages > >>>> without it?) > >>> Hi Martin, > >>> While writing the POC patch, my expectation was to treat consteval > >>> param as a more "restricted version" > >>> of const param -- in addition to being readonly, it should only be > >>> passed compile-time constant > >>> values, and so I thought it probably made sense to require it to be > >>> const qualified. > >>> But if that doesn't seem like a good idea, we can drop it to be const qualified. > >> > >> I don't think depending on the const qualifier is necessary or > >> a good idea. > >> > >>> > >>> I was wondering how do we take this forward ? > >>> For a start would this be OK ? > >>> 1] Since we also need range info, define consteval attribute to take 3 > >>> arguments -- param-pos, min-val, max-val. > >>> and requiring the parameter to be an integral type (unlike the one > >>> implemented in POC patch). > >> > >> Specifying a range for a variable of any kind (including globals > >> and members) would be useful in general, independent of this > >> enhancement. I would suggest to introduce a separate attribute > >> for that. (With the effects of assigning out-of-range values > >> being undefined and a warning issued if/when detected.) > > Hi Martin, > > Sorry for late response. > > > > For vshl_n case, we need two constraints on the argument: > > (1) It has to be a constant. > > (2) The constant value has to be within the permissible range > > corresponding to the width. > > For eg, vshl_n_s32, accepts a constant value only between [0, 31] range. > > > > I agree that the range-info can be more generally useful beyond > > argument checking. > > So, do you think we should add two attributes: > > (1) consteval(param-pos), which just tells the compiler that the > > corresponding parameter > > expects an ICE (similar to POC patch). > > > > (2) range-info attribute for integral types similar to mode. > > For eg: > > typedef int __attribute__((range_info(0, 31))) S32; > > S32 x; // x is int with [0, 31] range > > > > So, vshl_n_s32 could be declared as: > > > > __extension__ extern __inline int32x2_t > > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__, > > consteval(2))) > > vshl_n_s32 (int32x2_t __a, const S32 __b) > > { > > return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > } > > > > which would tell the compiler that 'b' needs to be constant within [0, > > 31] range, > > and emit an error otherwise during parsing. > > Yes, that's what I was thinking (without the const qualifier). > The range_info attribute (I'd suggest naming it value_range) > could be used on parameters as well to avoid having to > introduce typedefs: > > __attribute__ ((..., consteval(2))) > vshl_n_s32 (int32x2_t a, int b __attribute__ ((value_range (0, 31))) > { ... } > > (I think accepting the attribute on types as in your example would > be useful as well, as would be accepting multiple attributes to > represent multi-ranges). > > >> > >>> 2] Diagnose a function call, if the corresponding argument (after > >>> invoking c_fully_fold) is not INTEGER_CST. > >>> > >>> My intent is to currently accept ICE's that are also acceptable in > >>> other language features like case statement. > >> > >> So something like this: > >> > >> __attribute__ ((always_inline, artificial, consteval (1))) > >> inline int f (int i) > >> { > >> switch (i) case i: return i; // or a built-in that expects > >> return 0; // a constant expression argument > >> } > >> > >> int g (void) { return f (7); } // valid > >> > >> but: > >> > >> int h (int i) { return f (i); } // invalid > >> > >> and a nice error for the call to h (as opposed to f): > >> > >> In function 'h': > >> error: argument 1 to 'f' is not a constant integer > >> note: 'f' declared with attribute 'consteval (1)' > >> > >>> As you suggest above, we could add "constexpr-lite" attribute to C, > >>> for accepting more complicated integral constant expressions, > >>> that can also be used in other language features that need ICE's. > >>> > >>> My concern with this approach tho, is that currently we accept ICE's > >>> for intrinsics and rely on the optimizer to resolve them into > >>> constants > >>> before expansion. > >> > >> So calls to the intrinsics don't compile without optimization? > >> (That's not what I saw in my very limited testing but I know > >> almost nothing about the ARM builtins so I could be missing > >> something.) > > I think it's only the case for _n intrinsics that require an ICE. > > The actual checking for constant currently happens in > > expand_builtin_args, in the backend during expansion, > > so the following function: > > > > int32x2_t f(int32_t *restrict a) > > { > > int32x2_t v = vld1_s32 (a); > > int b = 2; > > return vshl_n_s32 (v, b); > > } > > > > compiles at -O1+ because ccp turns 'b' into a constant, but fails at > > -O0 with following diagnostic: > > In file included from foo.c:1: > > In function ‘vshl_n_s32’, > > inlined from ‘f’ at foo.c:9:10: > > ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument 2 must > > be a constant immediate > > 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > I meant simply this: > > vshl_n_s32 (v, 2); > > With vshl_n_s32() an inline wrapper around the __builtin, I'd expect > the call not to compile for the reason we're discussing (i.e., it's > not an constant integer expression). > > >> > >>> So code that passes ICE to vshl_n, would get > >>> compiled at higher optimization levels but fail to compile with -O0. > >>> I am wondering if there's any existing code that relies on this > >>> behavior and will fail to compile if we impose the above restriction ? > >> > >> I don't know. > >> > >> But as it turns out, an argument to a C++ constexpr function isn't > >> considered a constant expression within the body of the function > >> even if it is one at the call site. So the extension you describe > >> would also apply to constexpr functions: > >> > >> __attribute__ ((consteval (1))) > >> constexpr int f (int i) > >> { > >> switch (i) case i: return i; // potentially valid with consteval > >> return 0; > >> } > >> > >> constexpr int x = f (1); // valid with consteval (not without) > >> int y = f (y); // not valid > > I guess that would be a more useful extension, but I was merely suggesting, > > to add constexpr to C for computing more complex ICE's. > > For eg: > > int a = 1; > > int b = a + 1; > > > > switch() { case b: ... } // error > > > > With constexpr: > > constexpr int a = 1; > > constexpr int b = a + 1; > > > > switch() { case b: ... } // OK > > I was just noting that the consteval attribute would be useful even > in C++ where a constant integer expression argument of a constexpr > function isn't considered constexpr. (Function arguments can't > be declared constexpr in C++ as far as I can tell.) > > A "constexpr" for variables only (or attribute consteval) be useful > too in C. That would suggest making it a variable attribute rather > than a positional function attribute. > > FWIW, there was a proposal for C2x along these lines some time ago: > http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2067.pdf > AFAIK, it didn't go anywhere in WG14 for a variety of reasons but > the basic idea behind it was a good one. Thanks for the suggestions. To summarize we'd need (1) consteval (let's just call it constexpr?) attribute on function params. (2) value_range attribute on variables and/or types. For extending a consteval/constexpr param to be accepted as ICE like in case statement, won't it be necessary tho that the function be always inlined ? As mentioned above, my concern with using the attribute(s) on the _n intrinsics, is that it will break code that relies on the optimizer to resolve expressions to constants like: int a = 1; vshl_n_s32 (x, a); @Richard Earnshaw would that be OK ? Thanks, Prathamesh Thanks, Prathamesh > > Martin > > > > > Thanks, > > Prathamesh > >> > >> Martin > >> > >>> > >>> Thanks, > >>> Prathamesh > >>> > >>> > >>>> > >>>> Martin > >>>> > >>>>> > >>>>> R. > >>>>> > >>>>>> > >>>>>> Martin > >>>>>> > >>>>>> > >>>>>>> > >>>>>>> R. > >>>>>>>> > >>>>>>>> That said, if compile-time function evaluation is the goal then > >>>>>>>> a fully general solution is an attribute that applies to the whole > >>>>>>>> function, not just a subset of its arguments. That way arguments > >>>>>>>> can also be assigned to local variables within the function that > >>>>>>>> can then be modified while still evaluated at compile time and > >>>>>>>> used where constant expressions are expected. I.e., the design > >>>>>>>> goal is [a subset of] C++ constexpr. (Obviously a much bigger > >>>>>>>> project.) > >>>>>>>> > >>>>>>>> A few notes on the prototype patch: conventionally GCC warnings > >>>>>>>> about attributes do not mention when an attribute is ignored. > >>>>>>>> It may be a nice touch to add to all of them but I'd recommend > >>>>>>>> against doing that in individual handlers. Since the attribute > >>>>>>>> allows pointer constants the warning issued when an argument is > >>>>>>>> not one should be generalized (i.e., not refer to just integer > >>>>>>>> constants). > >>>>>>>> > >>>>>>>> (Other than that, C/C++ warnings should start in lowercase and > >>>>>>>> not end in a period). > >>>>>>>> > >>>>>>>> Martin > >>>>>>>> > >>>>>>>>> > >>>>>>>>> The patch does type-checking for arguments in > >>>>>>>>> check_function_consteval_attr, which > >>>>>>>>> simply does a linear search to see if the corresponding argument > >>>>>>>>> number is included in consteval attribute, > >>>>>>>>> and if yes, it checks if the argument is CONSTANT_CLASS_P. > >>>>>>>>> > >>>>>>>>> This works for simple constants and the intrinsics use-case, but > >>>>>>>>> rejects "extended constants" as in your above example. > >>>>>>>>> I guess we can special case to detect cases like a[i] where 'a' is > >>>>>>>>> const and 'i' is an immediate, > >>>>>>>>> but I am not sure if there's a way to force constant evaluation in > >>>>>>>>> FE ? > >>>>>>>>> I tried using c_fully_fold (argarray[i], false, &maybe_const) but that > >>>>>>>>> didn't seem to work. > >>>>>>>>> Do you have any suggestions on how to detect those in the front-end ? > >>>>>>>>> > >>>>>>>>> Example: > >>>>>>>>> > >>>>>>>>> __attribute__((consteval(1, 2))) > >>>>>>>>> void f(const int x, int *p) > >>>>>>>>> {} > >>>>>>>>> > >>>>>>>>> Calling it with: > >>>>>>>>> 1) f(0, (int *) 0) is accepted. > >>>>>>>>> > >>>>>>>>> 2) > >>>>>>>>> void g(int *p) > >>>>>>>>> { > >>>>>>>>> f (0, p); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> emits the following error: > >>>>>>>>> test.c: In function ‘g’: > >>>>>>>>> test.c:7:9: error: argument 2 is not a constant. > >>>>>>>>> 7 | f (0, p); > >>>>>>>>> | ^ > >>>>>>>>> test.c:2:6: note: Function ‘f’ has parameter 2 with consteval > >>>>>>>>> attribute. > >>>>>>>>> 2 | void f(const int x, int *const p) > >>>>>>>>> | ^ > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> Prathamesh > >>>>>>>>>> > >>>>>>>>>> Richard. > >>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Thanks, > >>>>>>>>>>> Prathamesh > >>>>>>>>>>>> > >>>>>>>>>>>> Thanks, > >>>>>>>>>>>> Andrew Pinski > >>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>>> The constraint here is that, vshl_n<type> intrinsics require > >>>>>>>>>>>>> that the > >>>>>>>>>>>>> second arg (__b), > >>>>>>>>>>>>> should be an immediate value. > >>>>>>>>>>>>> Currently, this check is performed by arm_expand_builtin_args, > >>>>>>>>>>>>> and if > >>>>>>>>>>>>> a non-constant > >>>>>>>>>>>>> value gets passed, it emits the following diagnostic: > >>>>>>>>>>>>> > >>>>>>>>>>>>> ../armhf-build/gcc/include/arm_neon.h:4904:10: error: argument > >>>>>>>>>>>>> 2 must > >>>>>>>>>>>>> be a constant immediate > >>>>>>>>>>>>> 4904 | return (int32x2_t)__builtin_neon_vshl_nv2si (__a, __b); > >>>>>>>>>>>>> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > >>>>>>>>>>>>> > >>>>>>>>>>>>> However, we're trying to replace builtin calls with gcc's C vector > >>>>>>>>>>>>> extensions where > >>>>>>>>>>>>> possible (PR66791), because the builtins are opaque to the > >>>>>>>>>>>>> optimizers. > >>>>>>>>>>>>> > >>>>>>>>>>>>> Unfortunately, we lose type checking of immediate value if we > >>>>>>>>>>>>> replace > >>>>>>>>>>>>> the builtin > >>>>>>>>>>>>> with << operator: > >>>>>>>>>>>>> > >>>>>>>>>>>>> __extension__ extern __inline int32x2_t > >>>>>>>>>>>>> __attribute__ ((__always_inline__, __gnu_inline__, > >>>>>>>>>>>>> __artificial__)) > >>>>>>>>>>>>> vshl_n_s32 (int32x2_t __a, const int __b) > >>>>>>>>>>>>> { > >>>>>>>>>>>>> return __a << __b; > >>>>>>>>>>>>> } > >>>>>>>>>>>>> > >>>>>>>>>>>>> So, I was wondering if we should have an attribute for a > >>>>>>>>>>>>> parameter to > >>>>>>>>>>>>> specifically > >>>>>>>>>>>>> mark it as a constant value with optional range value info ? > >>>>>>>>>>>>> As Richard suggested, sth like: > >>>>>>>>>>>>> void foo(int x __attribute__((literal_constant (min_val, > >>>>>>>>>>>>> max_val))); > >>>>>>>>>>>>> > >>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>> Prathamesh > >>>>>>>> > >>>>>> > >>>> > >> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-07-23 10:53 [RFC] Adding a new attribute to function param to mark it as constant Prathamesh Kulkarni 2021-07-23 17:59 ` Andrew Pinski @ 2021-08-03 21:55 ` Segher Boessenkool 2021-08-04 9:50 ` Prathamesh Kulkarni 1 sibling, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-03 21:55 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: GCC Development, Richard Earnshaw Hi! On Fri, Jul 23, 2021 at 04:23:42PM +0530, Prathamesh Kulkarni via Gcc wrote: > The constraint here is that, vshl_n<type> intrinsics require that the > second arg (__b), > should be an immediate value. Something that matches the "n" constraint, not necessarily a literal, but stricter than just "immediate". It probably is a good idea to allow only "integer constant expression"s, so that the validity of the source code does not depend on what the optimisers do with the code. > As Richard suggested, sth like: > void foo(int x __attribute__((literal_constant (min_val, max_val))); The Linux kernel has a macro __is_constexpr to test if something is an integer constant expression, see <linux/const.h> . That is a much better idea imo. There could be a builtin for that of course, but an attribute is less powerful, less usable, less useful. Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-03 21:55 ` Segher Boessenkool @ 2021-08-04 9:50 ` Prathamesh Kulkarni 2021-08-04 10:17 ` Segher Boessenkool 0 siblings, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-04 9:50 UTC (permalink / raw) To: Segher Boessenkool; +Cc: GCC Development, Richard Earnshaw On Wed, 4 Aug 2021 at 03:27, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > Hi! > > On Fri, Jul 23, 2021 at 04:23:42PM +0530, Prathamesh Kulkarni via Gcc wrote: > > The constraint here is that, vshl_n<type> intrinsics require that the > > second arg (__b), > > should be an immediate value. > > Something that matches the "n" constraint, not necessarily a literal, > but stricter than just "immediate". It probably is a good idea to allow > only "integer constant expression"s, so that the validity of the source > code does not depend on what the optimisers do with the code. > > > As Richard suggested, sth like: > > void foo(int x __attribute__((literal_constant (min_val, max_val))); > > The Linux kernel has a macro __is_constexpr to test if something is an > integer constant expression, see <linux/const.h> . That is a much > better idea imo. There could be a builtin for that of course, but an > attribute is less powerful, less usable, less useful. Hi Segher, Thanks for the suggestions. I am not sure tho if we could use a macro similar to __is_constexpr to check if parameter is constant inside an inline function (which is the case for intrinsics) ? For eg: #define __is_constexpr(x) \ (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8))) inline int foo(const int x) { _Static_assert (__is_constexpr (x)); return x; } int main() { return foo (1); } results in: foo.c: In function ‘foo’: foo.c:8:3: error: static assertion failed 8 | _Static_assert (__is_constexpr (x)); Initially we tried to use __Static_assert (__builtin_constant_p (arg)) for the same purpose but that did not work because while parsing the intrinsic function, the FE cannot determine if the arg is indeed a constant. I guess the static assertion or __is_constexpr would work only if the intrinsic were defined as a macro instead of an inline function ? Or am I misunderstanding ? Thanks, Prathamesh > > > Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 9:50 ` Prathamesh Kulkarni @ 2021-08-04 10:17 ` Segher Boessenkool 2021-08-04 11:50 ` Prathamesh Kulkarni 0 siblings, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-04 10:17 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: GCC Development, Richard Earnshaw On Wed, Aug 04, 2021 at 03:20:45PM +0530, Prathamesh Kulkarni wrote: > On Wed, 4 Aug 2021 at 03:27, Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > The Linux kernel has a macro __is_constexpr to test if something is an > > integer constant expression, see <linux/const.h> . That is a much > > better idea imo. There could be a builtin for that of course, but an > > attribute is less powerful, less usable, less useful. > Hi Segher, > Thanks for the suggestions. I am not sure tho if we could use a macro > similar to __is_constexpr > to check if parameter is constant inside an inline function (which is > the case for intrinsics) ? I said we can make a builtin that returns if its arg is an ICE -- we do not have to do tricky tricks :-) The macro would work fine in an inline function though, or, where do you see potential problems? > For eg: > #define __is_constexpr(x) \ > (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8))) > > inline int foo(const int x) > { > _Static_assert (__is_constexpr (x)); > return x; > } > > int main() > { > return foo (1); > } > > results in: > foo.c: In function ‘foo’: > foo.c:8:3: error: static assertion failed > 8 | _Static_assert (__is_constexpr (x)); And that is correct, x is *not* an integer constant expression here. Because it is a variable, instead :-) If you do this in a macro it should work though? > Initially we tried to use __Static_assert (__builtin_constant_p (arg)) > for the same purpose but that did not work > because while parsing the intrinsic function, the FE cannot determine > if the arg is indeed a constant. Yes. If you want something like that you need to test very late during compilation whether something is a constant then: it will not be earlier. > I guess the static assertion or __is_constexpr would work only if the > intrinsic were defined as a macro instead of an inline function ? > Or am I misunderstanding ? Both __builtin_constant_p and __is_constexpr will not work in your use case (since a function argument is not a constant, let alone an ICE). It only becomes a constant value later on. The manual (for the former) says: You may use this built-in function in either a macro or an inline function. However, if you use it in an inlined function and pass an argument of the function as the argument to the built-in, GCC never returns 1 when you call the inline function with a string constant or compound literal (see Compound Literals) and does not return 1 when you pass a constant numeric value to the inline function unless you specify the -O option. An integer constant expression is well-defined whatever the optimisation level is, it is a feature of the language. If some x is an ICE you can do asm ("" :: "n"(x)); and if it is a constant you can do asm ("" :: "i"(x)); (not that that gets you much further, but it might help explorng this). Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 10:17 ` Segher Boessenkool @ 2021-08-04 11:50 ` Prathamesh Kulkarni 2021-08-04 12:46 ` Segher Boessenkool 0 siblings, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-04 11:50 UTC (permalink / raw) To: Segher Boessenkool; +Cc: GCC Development, Richard Earnshaw On Wed, 4 Aug 2021 at 15:49, Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Wed, Aug 04, 2021 at 03:20:45PM +0530, Prathamesh Kulkarni wrote: > > On Wed, 4 Aug 2021 at 03:27, Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > The Linux kernel has a macro __is_constexpr to test if something is an > > > integer constant expression, see <linux/const.h> . That is a much > > > better idea imo. There could be a builtin for that of course, but an > > > attribute is less powerful, less usable, less useful. > > Hi Segher, > > Thanks for the suggestions. I am not sure tho if we could use a macro > > similar to __is_constexpr > > to check if parameter is constant inside an inline function (which is > > the case for intrinsics) ? > > I said we can make a builtin that returns if its arg is an ICE -- we do > not have to do tricky tricks :-) > > The macro would work fine in an inline function though, or, where do you > see potential problems? > > > For eg: > > #define __is_constexpr(x) \ > > (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8))) > > > > inline int foo(const int x) > > { > > _Static_assert (__is_constexpr (x)); > > return x; > > } > > > > int main() > > { > > return foo (1); > > } > > > > results in: > > foo.c: In function ‘foo’: > > foo.c:8:3: error: static assertion failed > > 8 | _Static_assert (__is_constexpr (x)); > > And that is correct, x is *not* an integer constant expression here. > Because it is a variable, instead :-) > > If you do this in a macro it should work though? > > > Initially we tried to use __Static_assert (__builtin_constant_p (arg)) > > for the same purpose but that did not work > > because while parsing the intrinsic function, the FE cannot determine > > if the arg is indeed a constant. > > Yes. If you want something like that you need to test very late during > compilation whether something is a constant then: it will not be earlier. > > > I guess the static assertion or __is_constexpr would work only if the > > intrinsic were defined as a macro instead of an inline function ? > > Or am I misunderstanding ? > > Both __builtin_constant_p and __is_constexpr will not work in your use > case (since a function argument is not a constant, let alone an ICE). > It only becomes a constant value later on. The manual (for the former) > says: > You may use this built-in function in either a macro or an inline > function. However, if you use it in an inlined function and pass an > argument of the function as the argument to the built-in, GCC never > returns 1 when you call the inline function with a string constant or > compound literal (see Compound Literals) and does not return 1 when you > pass a constant numeric value to the inline function unless you specify > the -O option. Indeed, that's why I was thinking if we should use an attribute to mark param as a constant, so during type-checking the function call, the compiler can emit a diagnostic if the passed arg is not a constant. Alternatively -- as you suggest, we could define a new builtin, say __builtin_ice(x) that returns true if 'x' is an ICE. And wrap the intrinsic inside a macro that would check if the arg is an ICE ? For eg: __extension__ extern __inline int32x2_t __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) vshl_n_s32_1 (int32x2_t __a, const int __b) { return __builtin_neon_vshl_nv2si (__a, __b); } #define vshl_n_s32(__a, __b) \ ({ typeof (__a) a = (__a); \ _Static_assert (__builtin_constant_p ((__b)), #__b " is not an integer constant"); \ vshl_n_s32_1 (a, (__b)); }) void f(int32x2_t x, const int y) { vshl_n_s32 (x, 2); vshl_n_s32 (x, y); int z = 1; vshl_n_s32 (x, z); } With this, the compiler rejects vshl_n_s32 (x, y) and vshl_n_s32 (x, z) at all optimization levels since neither 'y' nor 'z' is an ICE. Instead of __builtin_constant_p, we could use __builtin_ice. Would that be a reasonable approach ? But this changes the semantics of intrinsic from being an inline function to a macro, and I am not sure if that's a good idea. Thanks, Prathamesh > An integer constant expression is well-defined whatever the optimisation > level is, it is a feature of the language. > > If some x is an ICE you can do > asm ("" :: "n"(x)); > and if it is a constant you can do > asm ("" :: "i"(x)); > (not that that gets you much further, but it might help explorng this). > > > Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 11:50 ` Prathamesh Kulkarni @ 2021-08-04 12:46 ` Segher Boessenkool 2021-08-04 13:00 ` Richard Earnshaw 0 siblings, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-04 12:46 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: GCC Development, Richard Earnshaw On Wed, Aug 04, 2021 at 05:20:58PM +0530, Prathamesh Kulkarni wrote: > On Wed, 4 Aug 2021 at 15:49, Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > Both __builtin_constant_p and __is_constexpr will not work in your use > > case (since a function argument is not a constant, let alone an ICE). > > It only becomes a constant value later on. The manual (for the former) > > says: > > You may use this built-in function in either a macro or an inline > > function. However, if you use it in an inlined function and pass an > > argument of the function as the argument to the built-in, GCC never > > returns 1 when you call the inline function with a string constant or > > compound literal (see Compound Literals) and does not return 1 when you > > pass a constant numeric value to the inline function unless you specify > > the -O option. > Indeed, that's why I was thinking if we should use an attribute to mark param as > a constant, so during type-checking the function call, the compiler > can emit a diagnostic if the passed arg > is not a constant. That will depend on the vagaries of what optimisations the compiler managed to do :-( > Alternatively -- as you suggest, we could define a new builtin, say > __builtin_ice(x) that returns true if 'x' is an ICE. (That is a terrible name, it's not clear at all to the reader, just write it out? It is fun if you know what it means, but infuriating otherwise.) > And wrap the intrinsic inside a macro that would check if the arg is an ICE ? That will work yeah. Maybe not as elegant as you'd like, but not all that bad, and it *works*. Well, hopefully it does :-) > For eg: > > __extension__ extern __inline int32x2_t > __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > vshl_n_s32_1 (int32x2_t __a, const int __b) > { > return __builtin_neon_vshl_nv2si (__a, __b); > } > > #define vshl_n_s32(__a, __b) \ > ({ typeof (__a) a = (__a); \ > _Static_assert (__builtin_constant_p ((__b)), #__b " is not an > integer constant"); \ > vshl_n_s32_1 (a, (__b)); }) > > void f(int32x2_t x, const int y) > { > vshl_n_s32 (x, 2); > vshl_n_s32 (x, y); > > int z = 1; > vshl_n_s32 (x, z); > } > > With this, the compiler rejects vshl_n_s32 (x, y) and vshl_n_s32 (x, > z) at all optimization levels since neither 'y' nor 'z' is an ICE. You used __builtin_constant_p though, which works differently, so the test is not conclusive, might not show what you want to show. > Instead of __builtin_constant_p, we could use __builtin_ice. > Would that be a reasonable approach ? I think it will work, yes. > But this changes the semantics of intrinsic from being an inline > function to a macro, and I am not sure if that's a good idea. Well, what happens if you call the actual builtin directly, with some non-constant parameter? That just fails with a more cryptic error, right? So you can view this as some syntactic sugar to make these intrinsics easier to use. Hrm I now remember a place I could have used this: #define mtspr(n, x) do { asm("mtspr %1,%0" : : "r"(x), "n"(n)); } while (0) #define mfspr(n) ({ \ u32 x; asm volatile("mfspr %0,%1" : "=r"(x) : "n"(n)); x; \ }) It is quite similar to your builtin code really, and I did resort to macros there, for similar reasons :-) Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 12:46 ` Segher Boessenkool @ 2021-08-04 13:00 ` Richard Earnshaw 2021-08-04 13:40 ` Segher Boessenkool 2021-08-05 9:01 ` Prathamesh Kulkarni 0 siblings, 2 replies; 31+ messages in thread From: Richard Earnshaw @ 2021-08-04 13:00 UTC (permalink / raw) To: Segher Boessenkool, Prathamesh Kulkarni; +Cc: GCC Development On 04/08/2021 13:46, Segher Boessenkool wrote: > On Wed, Aug 04, 2021 at 05:20:58PM +0530, Prathamesh Kulkarni wrote: >> On Wed, 4 Aug 2021 at 15:49, Segher Boessenkool >> <segher@kernel.crashing.org> wrote: >>> Both __builtin_constant_p and __is_constexpr will not work in your use >>> case (since a function argument is not a constant, let alone an ICE). >>> It only becomes a constant value later on. The manual (for the former) >>> says: >>> You may use this built-in function in either a macro or an inline >>> function. However, if you use it in an inlined function and pass an >>> argument of the function as the argument to the built-in, GCC never >>> returns 1 when you call the inline function with a string constant or >>> compound literal (see Compound Literals) and does not return 1 when you >>> pass a constant numeric value to the inline function unless you specify >>> the -O option. >> Indeed, that's why I was thinking if we should use an attribute to mark param as >> a constant, so during type-checking the function call, the compiler >> can emit a diagnostic if the passed arg >> is not a constant. > > That will depend on the vagaries of what optimisations the compiler > managed to do :-( > >> Alternatively -- as you suggest, we could define a new builtin, say >> __builtin_ice(x) that returns true if 'x' is an ICE. > > (That is a terrible name, it's not clear at all to the reader, just > write it out? It is fun if you know what it means, but infuriating > otherwise.) > >> And wrap the intrinsic inside a macro that would check if the arg is an ICE ? > > That will work yeah. Maybe not as elegant as you'd like, but not all > that bad, and it *works*. Well, hopefully it does :-) > >> For eg: >> >> __extension__ extern __inline int32x2_t >> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) >> vshl_n_s32_1 (int32x2_t __a, const int __b) >> { >> return __builtin_neon_vshl_nv2si (__a, __b); >> } >> >> #define vshl_n_s32(__a, __b) \ >> ({ typeof (__a) a = (__a); \ >> _Static_assert (__builtin_constant_p ((__b)), #__b " is not an >> integer constant"); \ >> vshl_n_s32_1 (a, (__b)); }) >> >> void f(int32x2_t x, const int y) >> { >> vshl_n_s32 (x, 2); >> vshl_n_s32 (x, y); >> >> int z = 1; >> vshl_n_s32 (x, z); >> } >> >> With this, the compiler rejects vshl_n_s32 (x, y) and vshl_n_s32 (x, >> z) at all optimization levels since neither 'y' nor 'z' is an ICE. > > You used __builtin_constant_p though, which works differently, so the > test is not conclusive, might not show what you want to show. > >> Instead of __builtin_constant_p, we could use __builtin_ice. >> Would that be a reasonable approach ? > > I think it will work, yes. > >> But this changes the semantics of intrinsic from being an inline >> function to a macro, and I am not sure if that's a good idea. > > Well, what happens if you call the actual builtin directly, with some > non-constant parameter? That just fails with a more cryptic error, > right? So you can view this as some syntactic sugar to make these > intrinsics easier to use. > > Hrm I now remember a place I could have used this: > > #define mtspr(n, x) do { asm("mtspr %1,%0" : : "r"(x), "n"(n)); } while (0) > #define mfspr(n) ({ \ > u32 x; asm volatile("mfspr %0,%1" : "=r"(x) : "n"(n)); x; \ > }) > > It is quite similar to your builtin code really, and I did resort to > macros there, for similar reasons :-) > > > Segher > We don't want to have to resort to macros. Not least because at some point we want to replace the content of arm_neon.h with a single #pragma directive to remove all the parsing of the header that's needed. What's more, if we had a suitable pragma we'd stand a fighting chance of being able to extend support to other languages as well that don't use the pre-processor, such as Fortran or Ada (not that that is on the cards right now). R. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 13:00 ` Richard Earnshaw @ 2021-08-04 13:40 ` Segher Boessenkool 2021-08-04 14:27 ` Richard Earnshaw 2021-08-05 9:01 ` Prathamesh Kulkarni 1 sibling, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-04 13:40 UTC (permalink / raw) To: Richard Earnshaw; +Cc: Prathamesh Kulkarni, GCC Development On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: > We don't want to have to resort to macros. Not least because at some > point we want to replace the content of arm_neon.h with a single #pragma > directive to remove all the parsing of the header that's needed. What's > more, if we had a suitable pragma we'd stand a fighting chance of being > able to extend support to other languages as well that don't use the > pre-processor, such as Fortran or Ada (not that that is on the cards > right now). So how do you want to handle constants-that-are-not-yet-constant, say before inlining? And how do you want to deal with those possibly not ever becoming constant, perhaps because you used a too low "n" in -On (but there are very many random other causes)? And, what *is* a constant, anyway? This is even more fuzzy if you consider those other languages as well. (Does skipping parsing of some trivial header save so much time? Huh!) Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 13:40 ` Segher Boessenkool @ 2021-08-04 14:27 ` Richard Earnshaw 2021-08-04 16:16 ` Segher Boessenkool 0 siblings, 1 reply; 31+ messages in thread From: Richard Earnshaw @ 2021-08-04 14:27 UTC (permalink / raw) To: Segher Boessenkool; +Cc: Prathamesh Kulkarni, GCC Development On 04/08/2021 14:40, Segher Boessenkool wrote: > On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: >> We don't want to have to resort to macros. Not least because at some >> point we want to replace the content of arm_neon.h with a single #pragma >> directive to remove all the parsing of the header that's needed. What's >> more, if we had a suitable pragma we'd stand a fighting chance of being >> able to extend support to other languages as well that don't use the >> pre-processor, such as Fortran or Ada (not that that is on the cards >> right now). > > So how do you want to handle constants-that-are-not-yet-constant, say > before inlining? And how do you want to deal with those possibly not > ever becoming constant, perhaps because you used a too low "n" in -On > (but there are very many random other causes)? And, what *is* a > constant, anyway? This is even more fuzzy if you consider those > other languages as well. > > (Does skipping parsing of some trivial header save so much time? Huh!) > Trivial? arm_neon.h is currently 20k lines of source. What's more, it has to support inline functions that might not be available when the header is parsed, but might become available if the user subsequently compiles a function with different attributes enabled. It is very definitely *NOT* trivial. R. > > Segher > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 14:27 ` Richard Earnshaw @ 2021-08-04 16:16 ` Segher Boessenkool 2021-08-04 17:08 ` Florian Weimer 0 siblings, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-04 16:16 UTC (permalink / raw) To: Richard Earnshaw; +Cc: Prathamesh Kulkarni, GCC Development On Wed, Aug 04, 2021 at 03:27:00PM +0100, Richard Earnshaw wrote: > On 04/08/2021 14:40, Segher Boessenkool wrote: > >On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: > >>We don't want to have to resort to macros. Not least because at some > >>point we want to replace the content of arm_neon.h with a single #pragma > >>directive to remove all the parsing of the header that's needed. What's > >>more, if we had a suitable pragma we'd stand a fighting chance of being > >>able to extend support to other languages as well that don't use the > >>pre-processor, such as Fortran or Ada (not that that is on the cards > >>right now). > > > >So how do you want to handle constants-that-are-not-yet-constant, say > >before inlining? And how do you want to deal with those possibly not > >ever becoming constant, perhaps because you used a too low "n" in -On > >(but there are very many random other causes)? And, what *is* a > >constant, anyway? This is even more fuzzy if you consider those > >other languages as well. > > > >(Does skipping parsing of some trivial header save so much time? Huh!) > > Trivial? arm_neon.h is currently 20k lines of source. What's more, it > has to support inline functions that might not be available when the > header is parsed, but might become available if the user subsequently > compiles a function with different attributes enabled. It is very > definitely *NOT* trivial. Ha yes :-) I just assumed without looking that it would be like other architectures' intrinsics headers. Whoops. Now I could ask how this monster came to be this big, but I will wisely back away. Slowly :-) Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 16:16 ` Segher Boessenkool @ 2021-08-04 17:08 ` Florian Weimer 2021-08-04 17:59 ` Segher Boessenkool 0 siblings, 1 reply; 31+ messages in thread From: Florian Weimer @ 2021-08-04 17:08 UTC (permalink / raw) To: Segher Boessenkool; +Cc: Richard Earnshaw, GCC Development * Segher Boessenkool: > On Wed, Aug 04, 2021 at 03:27:00PM +0100, Richard Earnshaw wrote: >> On 04/08/2021 14:40, Segher Boessenkool wrote: >> >On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: >> >>We don't want to have to resort to macros. Not least because at some >> >>point we want to replace the content of arm_neon.h with a single #pragma >> >>directive to remove all the parsing of the header that's needed. What's >> >>more, if we had a suitable pragma we'd stand a fighting chance of being >> >>able to extend support to other languages as well that don't use the >> >>pre-processor, such as Fortran or Ada (not that that is on the cards >> >>right now). >> > >> >So how do you want to handle constants-that-are-not-yet-constant, say >> >before inlining? And how do you want to deal with those possibly not >> >ever becoming constant, perhaps because you used a too low "n" in -On >> >(but there are very many random other causes)? And, what *is* a >> >constant, anyway? This is even more fuzzy if you consider those >> >other languages as well. >> > >> >(Does skipping parsing of some trivial header save so much time? Huh!) >> >> Trivial? arm_neon.h is currently 20k lines of source. What's more, it >> has to support inline functions that might not be available when the >> header is parsed, but might become available if the user subsequently >> compiles a function with different attributes enabled. It is very >> definitely *NOT* trivial. > > Ha yes :-) I just assumed without looking that it would be like other > architectures' intrinsics headers. Whoops. But isn't it? $ echo '#include <x86intrin.h>' | gcc -E - | wc -l 41045 Thanks, Florian ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 17:08 ` Florian Weimer @ 2021-08-04 17:59 ` Segher Boessenkool 2021-08-05 9:32 ` Richard Earnshaw 0 siblings, 1 reply; 31+ messages in thread From: Segher Boessenkool @ 2021-08-04 17:59 UTC (permalink / raw) To: Florian Weimer; +Cc: Richard Earnshaw, GCC Development On Wed, Aug 04, 2021 at 07:08:08PM +0200, Florian Weimer wrote: > * Segher Boessenkool: > > > On Wed, Aug 04, 2021 at 03:27:00PM +0100, Richard Earnshaw wrote: > >> On 04/08/2021 14:40, Segher Boessenkool wrote: > >> >On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: > >> >>We don't want to have to resort to macros. Not least because at some > >> >>point we want to replace the content of arm_neon.h with a single #pragma > >> >>directive to remove all the parsing of the header that's needed. What's > >> >>more, if we had a suitable pragma we'd stand a fighting chance of being > >> >>able to extend support to other languages as well that don't use the > >> >>pre-processor, such as Fortran or Ada (not that that is on the cards > >> >>right now). > >> > > >> >So how do you want to handle constants-that-are-not-yet-constant, say > >> >before inlining? And how do you want to deal with those possibly not > >> >ever becoming constant, perhaps because you used a too low "n" in -On > >> >(but there are very many random other causes)? And, what *is* a > >> >constant, anyway? This is even more fuzzy if you consider those > >> >other languages as well. > >> > > >> >(Does skipping parsing of some trivial header save so much time? Huh!) > >> > >> Trivial? arm_neon.h is currently 20k lines of source. What's more, it > >> has to support inline functions that might not be available when the > >> header is parsed, but might become available if the user subsequently > >> compiles a function with different attributes enabled. It is very > >> definitely *NOT* trivial. > > > > Ha yes :-) I just assumed without looking that it would be like other > > architectures' intrinsics headers. Whoops. > > But isn't it? > > $ echo '#include <x86intrin.h>' | gcc -E - | wc -l > 41045 $ echo '#include <altivec.h>' | gcc -E - -maltivec | wc -l 9 Most of this file (774 lines) is #define's, which take essentially no time at all. And none of the other archs I have looked at have big headers either! Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 17:59 ` Segher Boessenkool @ 2021-08-05 9:32 ` Richard Earnshaw 0 siblings, 0 replies; 31+ messages in thread From: Richard Earnshaw @ 2021-08-05 9:32 UTC (permalink / raw) To: Segher Boessenkool, Florian Weimer; +Cc: GCC Development On 04/08/2021 18:59, Segher Boessenkool wrote: > On Wed, Aug 04, 2021 at 07:08:08PM +0200, Florian Weimer wrote: >> * Segher Boessenkool: >> >>> On Wed, Aug 04, 2021 at 03:27:00PM +0100, Richard Earnshaw wrote: >>>> On 04/08/2021 14:40, Segher Boessenkool wrote: >>>>> On Wed, Aug 04, 2021 at 02:00:42PM +0100, Richard Earnshaw wrote: >>>>>> We don't want to have to resort to macros. Not least because at some >>>>>> point we want to replace the content of arm_neon.h with a single #pragma >>>>>> directive to remove all the parsing of the header that's needed. What's >>>>>> more, if we had a suitable pragma we'd stand a fighting chance of being >>>>>> able to extend support to other languages as well that don't use the >>>>>> pre-processor, such as Fortran or Ada (not that that is on the cards >>>>>> right now). >>>>> >>>>> So how do you want to handle constants-that-are-not-yet-constant, say >>>>> before inlining? And how do you want to deal with those possibly not >>>>> ever becoming constant, perhaps because you used a too low "n" in -On >>>>> (but there are very many random other causes)? And, what *is* a >>>>> constant, anyway? This is even more fuzzy if you consider those >>>>> other languages as well. >>>>> >>>>> (Does skipping parsing of some trivial header save so much time? Huh!) >>>> >>>> Trivial? arm_neon.h is currently 20k lines of source. What's more, it >>>> has to support inline functions that might not be available when the >>>> header is parsed, but might become available if the user subsequently >>>> compiles a function with different attributes enabled. It is very >>>> definitely *NOT* trivial. >>> >>> Ha yes :-) I just assumed without looking that it would be like other >>> architectures' intrinsics headers. Whoops. >> >> But isn't it? >> >> $ echo '#include <x86intrin.h>' | gcc -E - | wc -l >> 41045 > > $ echo '#include <altivec.h>' | gcc -E - -maltivec | wc -l > 9 > > Most of this file (774 lines) is #define's, which take essentially no > time at all. And none of the other archs I have looked at have big > headers either! > > > Segher > arm_sve.h isn't large either, but that's because all it contains (other than a couple of typedefs is #pragma GCC aarch64 "arm_sve.h" :) R. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-04 13:00 ` Richard Earnshaw 2021-08-04 13:40 ` Segher Boessenkool @ 2021-08-05 9:01 ` Prathamesh Kulkarni 2021-08-05 15:06 ` Segher Boessenkool 1 sibling, 1 reply; 31+ messages in thread From: Prathamesh Kulkarni @ 2021-08-05 9:01 UTC (permalink / raw) To: Richard Earnshaw; +Cc: Segher Boessenkool, GCC Development On Wed, 4 Aug 2021 at 18:30, Richard Earnshaw <Richard.Earnshaw@foss.arm.com> wrote: > > On 04/08/2021 13:46, Segher Boessenkool wrote: > > On Wed, Aug 04, 2021 at 05:20:58PM +0530, Prathamesh Kulkarni wrote: > >> On Wed, 4 Aug 2021 at 15:49, Segher Boessenkool > >> <segher@kernel.crashing.org> wrote: > >>> Both __builtin_constant_p and __is_constexpr will not work in your use > >>> case (since a function argument is not a constant, let alone an ICE). > >>> It only becomes a constant value later on. The manual (for the former) > >>> says: > >>> You may use this built-in function in either a macro or an inline > >>> function. However, if you use it in an inlined function and pass an > >>> argument of the function as the argument to the built-in, GCC never > >>> returns 1 when you call the inline function with a string constant or > >>> compound literal (see Compound Literals) and does not return 1 when you > >>> pass a constant numeric value to the inline function unless you specify > >>> the -O option. > >> Indeed, that's why I was thinking if we should use an attribute to mark param as > >> a constant, so during type-checking the function call, the compiler > >> can emit a diagnostic if the passed arg > >> is not a constant. > > > > That will depend on the vagaries of what optimisations the compiler > > managed to do :-( > > > >> Alternatively -- as you suggest, we could define a new builtin, say > >> __builtin_ice(x) that returns true if 'x' is an ICE. > > > > (That is a terrible name, it's not clear at all to the reader, just > > write it out? It is fun if you know what it means, but infuriating > > otherwise.) > > > >> And wrap the intrinsic inside a macro that would check if the arg is an ICE ? > > > > That will work yeah. Maybe not as elegant as you'd like, but not all > > that bad, and it *works*. Well, hopefully it does :-) > > > >> For eg: > >> > >> __extension__ extern __inline int32x2_t > >> __attribute__ ((__always_inline__, __gnu_inline__, __artificial__)) > >> vshl_n_s32_1 (int32x2_t __a, const int __b) > >> { > >> return __builtin_neon_vshl_nv2si (__a, __b); > >> } > >> > >> #define vshl_n_s32(__a, __b) \ > >> ({ typeof (__a) a = (__a); \ > >> _Static_assert (__builtin_constant_p ((__b)), #__b " is not an > >> integer constant"); \ > >> vshl_n_s32_1 (a, (__b)); }) > >> > >> void f(int32x2_t x, const int y) > >> { > >> vshl_n_s32 (x, 2); > >> vshl_n_s32 (x, y); > >> > >> int z = 1; > >> vshl_n_s32 (x, z); > >> } > >> > >> With this, the compiler rejects vshl_n_s32 (x, y) and vshl_n_s32 (x, > >> z) at all optimization levels since neither 'y' nor 'z' is an ICE. > > > > You used __builtin_constant_p though, which works differently, so the > > test is not conclusive, might not show what you want to show. > > > >> Instead of __builtin_constant_p, we could use __builtin_ice. > >> Would that be a reasonable approach ? > > > > I think it will work, yes. > > > >> But this changes the semantics of intrinsic from being an inline > >> function to a macro, and I am not sure if that's a good idea. > > > > Well, what happens if you call the actual builtin directly, with some > > non-constant parameter? That just fails with a more cryptic error, > > right? So you can view this as some syntactic sugar to make these > > intrinsics easier to use. > > > > Hrm I now remember a place I could have used this: > > > > #define mtspr(n, x) do { asm("mtspr %1,%0" : : "r"(x), "n"(n)); } while (0) > > #define mfspr(n) ({ \ > > u32 x; asm volatile("mfspr %0,%1" : "=r"(x) : "n"(n)); x; \ > > }) > > > > It is quite similar to your builtin code really, and I did resort to > > macros there, for similar reasons :-) > > > > > > Segher > > > > We don't want to have to resort to macros. Not least because at some > point we want to replace the content of arm_neon.h with a single #pragma > directive to remove all the parsing of the header that's needed. What's > more, if we had a suitable pragma we'd stand a fighting chance of being > able to extend support to other languages as well that don't use the > pre-processor, such as Fortran or Ada (not that that is on the cards > right now). Hi, IIUC, a more general issue here, is that the intrinsics require special type-checking of arguments, beyond what is dictated by the Standard. An argument needing to be an ICE could be seen as one instance. So perhaps, should there be some mechanism to tell the FE to let the target do additional checking for a particular function call, say by explicitly marking it with "intrinsic" attribute ? So while type checking a call to a function marked with "intrinsic" attribute, FE can invoke target handler with name of function and corresponding arguments passed, and then leave it to the target for further checking ? For vshl_n case, the target hook would check that the 2nd arg is an integer constant within the permissible range. I propose to do this only for intrinsics that need special checking and can be entirely implemented with C extensions and won't need pattern definitions / builtins, which I assume won't be a lot. I wonder if there'd be any apart from ones ending with _n ? Does the approach look reasonable ? Thanks, Prathamesh > > R. ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant 2021-08-05 9:01 ` Prathamesh Kulkarni @ 2021-08-05 15:06 ` Segher Boessenkool 0 siblings, 0 replies; 31+ messages in thread From: Segher Boessenkool @ 2021-08-05 15:06 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: Richard Earnshaw, GCC Development On Thu, Aug 05, 2021 at 02:31:02PM +0530, Prathamesh Kulkarni wrote: > On Wed, 4 Aug 2021 at 18:30, Richard Earnshaw > <Richard.Earnshaw@foss.arm.com> wrote: > > We don't want to have to resort to macros. Not least because at some > > point we want to replace the content of arm_neon.h with a single #pragma > > directive to remove all the parsing of the header that's needed. What's > > more, if we had a suitable pragma we'd stand a fighting chance of being > > able to extend support to other languages as well that don't use the > > pre-processor, such as Fortran or Ada (not that that is on the cards > > right now). > Hi, > IIUC, a more general issue here, is that the intrinsics require > special type-checking of arguments, beyond what is dictated by the > Standard. > An argument needing to be an ICE could be seen as one instance. > > So perhaps, should there be some mechanism to tell the FE to let the > target do additional checking for a particular function call, say by An integer constant expression can be checked by the frontend itself, it does not depend on optimisation etc. That is the beauty of it: it is a) more local, and b) a more reliable / less surprising thing to use. But it *is* less powerful than "it is a constant integer after a travel through the bowels of the compiler". Which of course is less reliable and more surprising (think what happens if you use -O0 or -O1 or -Og or -Os or any -fno- etc.) So it will be a lot more maintenance work (answering PRs about it is only the start). Segher ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [RFC] Adding a new attribute to function param to mark it as constant
@ 2021-08-06 20:10 Martin Uecker
0 siblings, 0 replies; 31+ messages in thread
From: Martin Uecker @ 2021-08-06 20:10 UTC (permalink / raw)
To: prathamesh.kulkarni; +Cc: gcc
> On Wed, 4 Aug 2021 at 03:27, Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > Hi!
> >
> > On Fri, Jul 23, 2021 at 04:23:42PM +0530, Prathamesh Kulkarni via Gcc wrote:
> > > The constraint here is that, vshl_n<type> intrinsics require that the
> > > second arg (__b),
> > > should be an immediate value.
> >
> > Something that matches the "n" constraint, not necessarily a literal,
> > but stricter than just "immediate". It probably is a good idea to allow
> > only "integer constant expression"s, so that the validity of the source
> > code does not depend on what the optimisers do with the code.
> >
> > > As Richard suggested, sth like:
> > > void foo(int x __attribute__((literal_constant (min_val, max_val)));
> >
> > The Linux kernel has a macro __is_constexpr to test if something is an
> > integer constant expression, see <linux/const.h> . That is a much
> > better idea imo. There could be a builtin for that of course, but an
> > attribute is less powerful, less usable, less useful.
> Hi Segher,
> Thanks for the suggestions. I am not sure tho if we could use a macro
> similar to __is_constexpr
> to check if parameter is constant inside an inline function (which is
> the case for intrinsics) ?
>
> For eg:
> #define __is_constexpr(x) \
> (sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
>
> inline int foo(const int x)
> {
> _Static_assert (__is_constexpr (x));
> return x;
> }
>
> int main()
> {
> return foo (1);
> }
>
> results in:
> foo.c: In function ‘foo’:
> foo.c:8:3: error: static assertion failed
> 8 | _Static_assert (__is_constexpr (x));
>
> Initially we tried to use __Static_assert (__builtin_constant_p (arg))
> for the same purpose but that did not work
> because while parsing the intrinsic function, the FE cannot determine
> if the arg is indeed a constant.
> I guess the static assertion or __is_constexpr would work only if the
> intrinsic were defined as a macro instead of an inline function ?
> Or am I misunderstanding ?
You have to use it at the call site:
#define __is_constexpr(x) \
(sizeof(int) == sizeof(*(8 ? ((void *)((long)(x) * 0l)) : (int *)8)))
#define foo(x) foo(({ _Static_assert(__is_constexpr(x), "no ICE"); (x); }))
inline int (foo)(const int x)
{
return x;
}
int main()
{
foo(1);
int n = 1;
foo(n); // error
}
--Martin
^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2021-08-19 8:10 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-23 10:53 [RFC] Adding a new attribute to function param to mark it as constant Prathamesh Kulkarni 2021-07-23 17:59 ` Andrew Pinski 2021-07-26 9:04 ` Prathamesh Kulkarni 2021-07-27 8:19 ` Richard Biener 2021-08-03 10:11 ` Prathamesh Kulkarni 2021-08-03 10:13 ` Prathamesh Kulkarni 2021-08-03 17:44 ` Martin Sebor 2021-08-04 9:46 ` Richard Earnshaw 2021-08-06 0:06 ` Martin Sebor 2021-08-06 10:51 ` Richard Earnshaw 2021-08-06 20:39 ` Martin Sebor 2021-08-12 8:32 ` Prathamesh Kulkarni 2021-08-13 17:14 ` Martin Sebor 2021-08-18 6:52 ` Prathamesh Kulkarni 2021-08-18 14:40 ` Martin Sebor 2021-08-19 8:10 ` Prathamesh Kulkarni 2021-08-03 21:55 ` Segher Boessenkool 2021-08-04 9:50 ` Prathamesh Kulkarni 2021-08-04 10:17 ` Segher Boessenkool 2021-08-04 11:50 ` Prathamesh Kulkarni 2021-08-04 12:46 ` Segher Boessenkool 2021-08-04 13:00 ` Richard Earnshaw 2021-08-04 13:40 ` Segher Boessenkool 2021-08-04 14:27 ` Richard Earnshaw 2021-08-04 16:16 ` Segher Boessenkool 2021-08-04 17:08 ` Florian Weimer 2021-08-04 17:59 ` Segher Boessenkool 2021-08-05 9:32 ` Richard Earnshaw 2021-08-05 9:01 ` Prathamesh Kulkarni 2021-08-05 15:06 ` Segher Boessenkool 2021-08-06 20:10 Martin Uecker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).