Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert()

[Vincent Mailhol]

On 13/11/2024 at 05:26, Yury Norov wrote:
> On Wed, Nov 13, 2024 at 04:08:39AM +0900, Vincent Mailhol wrote:
> > __builtin_constant_p() is known for not always being able to produce
> > constant expression [1] which lead to the introduction of
> > __is_constexpr() [2]. Because of its dependency on
> > __builtin_constant_p(), statically_true() suffers from the same
> > issues.
> >
> > For example:
> >
> >    void foo(int a)
> >    {
> >    	 /* fail on GCC */
> >    	BUILD_BUG_ON_ZERO(statically_true(a));
> >
> >    	 /* fail both clang and GCC */
> >    	static char arr[statically_true(a) ? 1 : 2];
> >    }
> >
> > For the same reasons why __is_constexpr() was created to cover
> > __builtin_constant_p() edge cases, __is_constexpr() can be used to
> > resolve statically_true() limitations.
> >
> > Note that, somehow, GCC is not always able to fold this:
> >
> >    __is_constexpr(x) && (x)
> >
> > It is OK in BUILD_BUG_ON_ZERO() but not in array declarations or in
> > static_assert():
> >
> >    void bar(int a)
> >    {
> >    	/* success */
> >    	BUILD_BUG_ON_ZERO(__is_constexpr(a) && (a));
> >
> >    	/* fail on GCC */
> >    	static char arr[__is_constexpr(a) && (a) ? 1 : 2];
> >
> >    	/* fail on GCC */
> >    	static_assert(__is_constexpr(a) && (a));
> >    }
> >
> > Encapsulating the expression in a __builtin_choose_expr() switch
> > resolves all these failed test.
> >
> > Declare a new _statically_true() macro which, by making use of the
> > __builtin_choose_expr() and __is_constexpr(x) combo, always produces a
> > constant expression.
> So, maybe name it const_true() then?


OK. I pretty like the _statically_true() because the link with 
statically_true() was obvious and the _ underscore prefix hinted that 
this variant was "special". But I have to admit that the const_true() is 
also really nice, and I finally adopted it in the v4.

> > It should be noted that statically_true() still produces better
> > folding:
> >
> >    statically_true(!(var * 8 % 8))
> >
> > always evaluates to true even if var is unknown, whereas
> >
> >    _statically_true(!(var * 8 % 8))
> >
> > fails to fold the expression and return false.
> >
> > For this reason, usage of _statically_true() be should the exception.
> > Reflect in the documentation that _statically_true() is less powerful
> > and that statically_true() is the overall preferred solution.
> >
> > [1] __builtin_constant_p cannot resolve to const when optimizing
> > Link: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=19449
> >
> > [2] commit 3c8ba0d61d04 ("kernel.h: Retain constant expression output for max()/min()")
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx>
> > ---
> > Bonuses:
> >
> >    - above examples, and a bit more:
> >
> >        https://godbolt.org/z/zzqM1ajPj
> >
> >    - a proof that statically_true() does better constant folding than _statically_true()
> >
> >        https://godbolt.org/z/vK6KK4hMG
> > ---
> >   include/linux/compiler.h | 14 ++++++++++++++
> >   1 file changed, 14 insertions(+)
> >
> > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > index 4d4e23b6e3e7..c76db8b50202 100644
> > --- a/include/linux/compiler.h
> > +++ b/include/linux/compiler.h
> > @@ -308,6 +308,20 @@ static inline void *offset_to_ptr(const int *off)
> >    */
> >   #define statically_true(x) (__builtin_constant_p(x) && (x))
> >   
> > +/*
> > + * Similar to statically_true() but produces a constant expression
> > + *
> > + * To be used in conjunction with macros, such as BUILD_BUG_ON_ZERO(),
> > + * which require their input to be a constant expression and for which
> > + * statically_true() would otherwise fail.
> > + *
> > + * This is a tradeoff: _statically_true() is less efficient at
> > + * constant folding and will fail to optimize any expressions in which
> > + * at least one of the subcomponent is not constant. For the general
> > + * case, statically_true() is better.
> I agree with Rasmus. Would be nice to have examples where should I use
> one vs another right here in the comment.


I rewrote the full set of examples in v4. I added the godbolt link in 
the patch description and I cherry picked what seems to me the two most 
meaningful examples and put them in the macro comment. Let me know what 
you think.

Yours sincerely,
Vincent Mailhol