Re: [RFC] Adding a new attribute to function param to mark it as constant

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

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