[RFC] Adding a new attribute to function param to mark it as constant
Prathamesh Kulkarni
prathamesh.kulkarni@linaro.org
Wed Aug 18 06:52:06 GMT 2021
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
> >>>>>>
> >>>>
> >>
>
More information about the Gcc
mailing list