* [RESEND PATCH v3 0/2] add _statically_true() to simplify GENMASK_INPUT_CHECK()
@ 2024-11-12 19:08 Vincent Mailhol
2024-11-12 19:08 ` [RESEND PATCH v3 1/2] compiler.h: add _static_assert() Vincent Mailhol
2024-11-12 19:08 ` [RESEND PATCH v3 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK() Vincent Mailhol
0 siblings, 2 replies; 7+ messages in thread
From: Vincent Mailhol @ 2024-11-12 19:08 UTC (permalink / raw)
To: Linus Torvalds, Yury Norov, Rasmus Villemoes, Luc Van Oostenryck
Cc: linux-kernel, linux-sparse, Rikard Falkeborn, Vincent Mailhol
The first patch introduces a new variant of statically_true() named
_statically_true() which rely on __is_constexpr() to produce a
constant expression result which can be used in BUILD_BUG_ON_ZERO()
and other macros which expect a constant expression as input.
The second patch applies this newly created _statically_true() to
GENMASK_INPUT_CHECK().
** Changelog **
v3 -> v3 RESEND:
- send email using the smtp.wanadoo.fr gateway. Note that this may
appear as smtp.orange.fr which is an alias (both have the same IP).
v2 -> v3:
- split the single patch into a series of two patches.
- add explanation of why _statically_true() is needed in addition
to the existing statically_true(). Explain the pros and cons of
each.
- use __builtin_choose_expr() in _statically_true(). The
_statically_true() of the v2 works perfectly fine when used in
conjunction with BUILD_BUG_ON_ZERO() but fails if used, for
example, in arrays or in static_assert().
Link: https://lore.kernel.org/all/20241111164743.339117-2-mailhol.vincent@wanadoo.fr/
v1 -> v2:
- introduce _statically_true(), taking inspiration from
statically_true() as introduced in commit 22f546873149 ("minmax:
improve macro expansion and type checking").
Link: https://lore.kernel.org/all/20240609073513.256179-1-mailhol.vincent@wanadoo.fr/
Vincent Mailhol (2):
compiler.h: add _static_assert()
linux/bits.h: simplify GENMASK_INPUT_CHECK()
include/linux/bits.h | 5 ++---
include/linux/compiler.h | 14 ++++++++++++++
2 files changed, 16 insertions(+), 3 deletions(-)
--
2.45.2
^ permalink raw reply [flat|nested] 7+ messages in thread* [RESEND PATCH v3 1/2] compiler.h: add _static_assert() 2024-11-12 19:08 [RESEND PATCH v3 0/2] add _statically_true() to simplify GENMASK_INPUT_CHECK() Vincent Mailhol @ 2024-11-12 19:08 ` Vincent Mailhol 2024-11-12 20:03 ` Rasmus Villemoes 2024-11-12 20:26 ` Yury Norov 2024-11-12 19:08 ` [RESEND PATCH v3 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK() Vincent Mailhol 1 sibling, 2 replies; 7+ messages in thread From: Vincent Mailhol @ 2024-11-12 19:08 UTC (permalink / raw) To: Linus Torvalds, Yury Norov, Rasmus Villemoes, Luc Van Oostenryck Cc: linux-kernel, linux-sparse, Rikard Falkeborn, Vincent Mailhol __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. 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@wanadoo.fr> --- 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. + */ +#define _statically_true(x) __builtin_choose_expr(__is_constexpr(x), x, false) + /* * This is needed in functions which generate the stack canary, see * arch/x86/kernel/smpboot.c::start_secondary() for an example. -- 2.45.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert() 2024-11-12 19:08 ` [RESEND PATCH v3 1/2] compiler.h: add _static_assert() Vincent Mailhol @ 2024-11-12 20:03 ` Rasmus Villemoes 2024-11-13 17:47 ` Vincent Mailhol 2024-11-12 20:26 ` Yury Norov 1 sibling, 1 reply; 7+ messages in thread From: Rasmus Villemoes @ 2024-11-12 20:03 UTC (permalink / raw) To: Vincent Mailhol Cc: Linus Torvalds, Yury Norov, Luc Van Oostenryck, linux-kernel, linux-sparse, Rikard Falkeborn On Wed, Nov 13 2024, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > > 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. Looks sane, but $subject needs s/_static_assert/_statically_true/. And to be completely pedantic, macros are defined, not declared. > > - 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 At least the second of these might be good to refer to in the commit message itself. Rasmus ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert() 2024-11-12 20:03 ` Rasmus Villemoes @ 2024-11-13 17:47 ` Vincent Mailhol 0 siblings, 0 replies; 7+ messages in thread From: Vincent Mailhol @ 2024-11-13 17:47 UTC (permalink / raw) To: Rasmus Villemoes Cc: Linus Torvalds, Yury Norov, Luc Van Oostenryck, linux-kernel, linux-sparse, Rikard Falkeborn On 13/11/2024 at 05:03, Rasmus Villemoes wrote: > On Wed, Nov 13 2024, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > >> 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. > Looks sane, but $subject needs s/_static_assert/_statically_true/. And > to be completely pedantic, macros are defined, not declared. Sorry for the silly mistake in the subject. And agreed that "define" is more appropriate than "declare" when speaking of macros. >> - 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 > At least the second of these might be good to refer to in the commit > message itself. Ack. I rewrote the set of examples in the v4 and added a few samples to the macro comment. Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert() 2024-11-12 19:08 ` [RESEND PATCH v3 1/2] compiler.h: add _static_assert() Vincent Mailhol 2024-11-12 20:03 ` Rasmus Villemoes @ 2024-11-12 20:26 ` Yury Norov 2024-11-13 17:44 ` Vincent Mailhol 1 sibling, 1 reply; 7+ messages in thread From: Yury Norov @ 2024-11-12 20:26 UTC (permalink / raw) To: Vincent Mailhol Cc: Linus Torvalds, Rasmus Villemoes, Luc Van Oostenryck, linux-kernel, linux-sparse, Rikard Falkeborn 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? > 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@wanadoo.fr> > --- > 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. > + */ > +#define _statically_true(x) __builtin_choose_expr(__is_constexpr(x), x, false) > + > /* > * This is needed in functions which generate the stack canary, see > * arch/x86/kernel/smpboot.c::start_secondary() for an example. > -- > 2.45.2 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH v3 1/2] compiler.h: add _static_assert() 2024-11-12 20:26 ` Yury Norov @ 2024-11-13 17:44 ` Vincent Mailhol 0 siblings, 0 replies; 7+ messages in thread From: Vincent Mailhol @ 2024-11-13 17:44 UTC (permalink / raw) To: Yury Norov Cc: Linus Torvalds, Rasmus Villemoes, Luc Van Oostenryck, linux-kernel, linux-sparse, Rikard Falkeborn 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@wanadoo.fr> >> --- >> 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RESEND PATCH v3 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK() 2024-11-12 19:08 [RESEND PATCH v3 0/2] add _statically_true() to simplify GENMASK_INPUT_CHECK() Vincent Mailhol 2024-11-12 19:08 ` [RESEND PATCH v3 1/2] compiler.h: add _static_assert() Vincent Mailhol @ 2024-11-12 19:08 ` Vincent Mailhol 1 sibling, 0 replies; 7+ messages in thread From: Vincent Mailhol @ 2024-11-12 19:08 UTC (permalink / raw) To: Linus Torvalds, Yury Norov, Rasmus Villemoes, Luc Van Oostenryck Cc: linux-kernel, linux-sparse, Rikard Falkeborn, Vincent Mailhol In GENMASK_INPUT_CHECK(), __builtin_choose_expr(__is_constexpr((l) > (h)), (l) > (h), 0) is the exact expansion of: _statically_true((l) > (h)) Apply _statically_true() to simplify GENMASK_INPUT_CHECK(). Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- This change passes the unit tests from CONFIG_BITS_TEST, including the extra negative tests provided under #ifdef TEST_GENMASK_FAILURES [1]. [1] commit 6d511020e13d ("lib/test_bits.c: add tests of GENMASK") Link: https://git.kernel.org/torvalds/c/6d511020e13d ** Changelog ** v2 -> v3: - split the single patch into a series of two patches. - add greater explanation of why _statically_true() is needed in addition of the existing statically_true(). Explain the pros and cons of both. - use __builtin_choose_expr() in _statically_true(). The _statically_true() of the v2 works perfectly fine when used in conjunction with BUILD_BUG_ON_ZERO() but fails if used in arrays or in static_assert() - update the patch description accordingly Link: https://lore.kernel.org/all/20241111164743.339117-2-mailhol.vincent@wanadoo.fr/ v1 -> v2: - introduce _statically_true(), taking inspiration from statically_true() as introduced in commit 22f546873149 ("minmax: improve macro expansion and type checking"). Link: https://lore.kernel.org/all/20240609073513.256179-1-mailhol.vincent@wanadoo.fr/ --- include/linux/bits.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/include/linux/bits.h b/include/linux/bits.h index 60044b608817..01713e1eda56 100644 --- a/include/linux/bits.h +++ b/include/linux/bits.h @@ -20,9 +20,8 @@ */ #if !defined(__ASSEMBLY__) #include <linux/build_bug.h> -#define GENMASK_INPUT_CHECK(h, l) \ - (BUILD_BUG_ON_ZERO(__builtin_choose_expr( \ - __is_constexpr((l) > (h)), (l) > (h), 0))) +#include <linux/compiler.h> +#define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(_statically_true((l) > (h))) #else /* * BUILD_BUG_ON_ZERO is not available in h files included from asm files, -- 2.45.2 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-11-13 17:47 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-12 19:08 [RESEND PATCH v3 0/2] add _statically_true() to simplify GENMASK_INPUT_CHECK() Vincent Mailhol 2024-11-12 19:08 ` [RESEND PATCH v3 1/2] compiler.h: add _static_assert() Vincent Mailhol 2024-11-12 20:03 ` Rasmus Villemoes 2024-11-13 17:47 ` Vincent Mailhol 2024-11-12 20:26 ` Yury Norov 2024-11-13 17:44 ` Vincent Mailhol 2024-11-12 19:08 ` [RESEND PATCH v3 2/2] linux/bits.h: simplify GENMASK_INPUT_CHECK() Vincent Mailhol
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).