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

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

[Martin Uecker]

> 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

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

[Prathamesh Kulkarni]

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
> >>>>>>>>
> >>>>>>
> >>>>
> >>
>

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

[Martin Sebor]

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
>>>>>>>>
>>>>>>
>>>>
>>

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

[Prathamesh Kulkarni]

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
> >>>>>>
> >>>>
> >>
>

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

[Martin Sebor]

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
>>>>>>
>>>>
>>

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

[Prathamesh Kulkarni]

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
> >>>>
> >>
>

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

[Martin Sebor]

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
>>>>
>>

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

[Richard Earnshaw]

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
>>>
> 

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

[Martin Sebor]

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
>>

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

[Segher Boessenkool]

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

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

[Richard Earnshaw]

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.

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

[Prathamesh Kulkarni]

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.

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

[Segher Boessenkool]

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

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

[Florian Weimer]

* 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

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

[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.  Now I could ask how this
monster came to be this big, but I will wisely back away.  Slowly :-)


Segher

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

[Richard Earnshaw]

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
> 

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

[Segher Boessenkool]

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

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

[Richard Earnshaw]

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.

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

[Segher Boessenkool]

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

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

[Prathamesh Kulkarni]

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

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

[Segher Boessenkool]

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

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

[Prathamesh Kulkarni]

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

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

[Richard Earnshaw]

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
> 

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

[Segher Boessenkool]

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

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

[Martin Sebor]

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

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

[Prathamesh Kulkarni]

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

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

[Prathamesh Kulkarni]

[-- 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);

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

[Richard Biener]

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

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

[Prathamesh Kulkarni]

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

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

[Andrew Pinski]

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

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

[Prathamesh Kulkarni]

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