* [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
* [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
* 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 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
* 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
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).