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

[Richard Earnshaw <Richard.Earnshaw@foss.arm.com>]

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