| Message ID | 74e0b027a908461da879b69b0e12c0de@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | minmax: reduce compilation time | expand |
On Sun, 28 Jul 2024 at 07:21, David Laight <David.Laight@aculab.com> wrote: > > +/* Allow if both x and y are valid for either signed or unsigned compares. */ > +#define __types_ok(x, y) \ > + ((__is_ok_signed(x) && __is_ok_signed(y)) || \ > + (__is_ok_unsigned(x) && __is_ok_unsigned(y))) This seems horrendous, exactly because it expands both x and y twice. And the "expand multiple times" was really the fundamental problem. Why not just change the model to say it's a bitmask of "signedness bits", the bits are "signed ok" and "unsigned ok", and turn it into /* Signedness matches? */ #define __types_ok(x, y) \ (__signedness_bits(x) & __signedness_bits(y)) and __signedness_ok() simply does something like "1 if unsigned type, 2 if signed type, 3 if signed positive integer". Something like (very very handwavy, very very untested): __builtin_choose_expr(is_signed_type(typeof(x)), 2+__if_constexpr(x,(x)>0,0), 1) Actually, I think that "__if_constexpr()" could very well be "if known positive value", ie 'x' itself doesn't have to be constant, but "x>0" has to be a constant (the difference being that the compiler may be able to tell that some variable is always positive, even if it's a variable): #define statically_true(x) __builtin_constant_p((x),(x),0) #define is_positive_value(x) statically_true((x)>=0) and then use __builtin_choose_expr(is_signed_type(typeof(x)), 2+is_positive_value(x), 1) and yes, I realize I count zero as a positive value, but writing out "nonnegative()" is annoying and we never care. I guess we could say "is_unsigned_value()"? Linus
From: Linus Torvalds > Sent: 28 July 2024 17:57 > > On Sun, 28 Jul 2024 at 07:21, David Laight <David.Laight@aculab.com> wrote: > > > > +/* Allow if both x and y are valid for either signed or unsigned compares. */ > > +#define __types_ok(x, y) \ > > + ((__is_ok_signed(x) && __is_ok_signed(y)) || \ > > + (__is_ok_unsigned(x) && __is_ok_unsigned(y))) > > This seems horrendous, exactly because it expands both x and y twice. > And the "expand multiple times" was really the fundamental problem. This version is better than the previous one ;-) > Why not just change the model to say it's a bitmask of "signedness > bits", the bits are "signed ok" and "unsigned ok", and turn it into > > /* Signedness matches? */ > #define __types_ok(x, y) \ > (__signedness_bits(x) & __signedness_bits(y)) Something like that might work, but it would take some effort to get right. It would be better to remove the 'low hanging fruit' of min(pointer_type) and the places where a constant is needed first. Both those require extra expansions and tend to make it all that much harder. > and __signedness_ok() simply does something like "1 if unsigned type, > 2 if signed type, 3 if signed positive integer". > > Something like (very very handwavy, very very untested): > > __builtin_choose_expr(is_signed_type(typeof(x)), > 2+__if_constexpr(x,(x)>0,0), > 1) You'd want to test '(x) >= 0' and the compiler is going to bleat (with -Wall) if (x) is an unsigned type - even though the code isn't used. Neither __builtin_choose_expr() or _Generic() help with that. Unless you need the types to differ ?: is just as good. > Actually, I think that "__if_constexpr()" could very well be "if known > positive value", ie 'x' itself doesn't have to be constant, but "x>0" > has to be a constant (the difference being that the compiler may be > able to tell that some variable is always positive, even if it's a > variable): > > #define statically_true(x) __builtin_constant_p((x),(x),0) > #define is_positive_value(x) statically_true((x)>=0) I think that test could be done on __x (ie the local copy). But then you can't use static_assert() and get a sane error message. (But don't look at what clang outputs...) > and then use > > __builtin_choose_expr(is_signed_type(typeof(x)), > 2+is_positive_value(x), 1) > > and yes, I realize I count zero as a positive value, but writing out > "nonnegative()" is annoying and we never care. I got annoyed earlier :-) > > I guess we could say "is_unsigned_value()"? David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
From: David Laight > Sent: 28 July 2024 19:15 > > From: Linus Torvalds > > Sent: 28 July 2024 17:57 > > > > On Sun, 28 Jul 2024 at 07:21, David Laight <David.Laight@aculab.com> wrote: > > > > > > +/* Allow if both x and y are valid for either signed or unsigned compares. */ > > > +#define __types_ok(x, y) \ > > > + ((__is_ok_signed(x) && __is_ok_signed(y)) || \ > > > + (__is_ok_unsigned(x) && __is_ok_unsigned(y))) > > > > This seems horrendous, exactly because it expands both x and y twice. > > And the "expand multiple times" was really the fundamental problem. > > This version is better than the previous one ;-) > > > Why not just change the model to say it's a bitmask of "signedness > > bits", the bits are "signed ok" and "unsigned ok", and turn it into > > > > /* Signedness matches? */ > > #define __types_ok(x, y) \ > > (__signedness_bits(x) & __signedness_bits(y)) > > Something like that might work, but it would take some effort to get right. Actually it doesn't work. The checks are is_signed((x) + 0) and is_unsigned((x)) so that 'unsigned char' can be compared against both 'int' and 'unsigned int'. But the signedness tests can use _unique_x which is trivially short. That needs a pre-change to pass __COUNTER__ through (as in min3()). David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/include/linux/minmax.h b/include/linux/minmax.h index ab64b2e73ae5..b9b5348a3879 100644 --- a/include/linux/minmax.h +++ b/include/linux/minmax.h @@ -8,7 +8,7 @@ #include <linux/types.h> /* - * min()/max()/clamp() macros must accomplish three things: + * min()/max()/clamp() macros must accomplish several things: * * - Avoid multiple evaluations of the arguments (so side-effects like * "x++" happen only once) when non-constant. @@ -26,19 +26,20 @@ #define __typecheck(x, y) \ (!!(sizeof((typeof(x) *)1 == (typeof(y) *)1))) -/* is_signed_type() isn't a constexpr for pointer types */ -#define __is_signed(x) \ - __builtin_choose_expr(__is_constexpr(is_signed_type(typeof(x))), \ - is_signed_type(typeof(x)), 0) +#define __is_signed(x) \ + __if_constexpr((typeof(x))1, is_signed_type(typeof(x)), 0) -/* True for a non-negative signed int constant */ -#define __is_noneg_int(x) \ - (__builtin_choose_expr(__is_constexpr(x) && __is_signed(x), x, -1) >= 0) +/* Allow unsigned compares against non-negative signed constants. */ +#define __is_ok_unsigned(x) \ + ((!__is_signed((x)) ? 0 : __if_constexpr(x, x, -1)) >= 0) -#define __types_ok(x, y) \ - (__is_signed(x) == __is_signed(y) || \ - __is_signed((x) + 0) == __is_signed((y) + 0) || \ - __is_noneg_int(x) || __is_noneg_int(y)) +/* Check for signed after promoting unsigned char/short to int */ +#define __is_ok_signed(x) __is_signed((x) + 0) + +/* Allow if both x and y are valid for either signed or unsigned compares. */ +#define __types_ok(x, y) \ + ((__is_ok_signed(x) && __is_ok_signed(y)) || \ + (__is_ok_unsigned(x) && __is_ok_unsigned(y))) #define __cmp_op_min < #define __cmp_op_max >
It is enough to check that both 'x' and 'y' are valid for either a signed compare or an unsigned compare. For unsigned they must be an unsigned type or a positive constant. For signed they must be signed after unsigned char/short are promoted. Order the expressions to avoid warnings about comparisons that are always true. Signed-off-by: David Laight <david.laight@aculab.com> --- Changes for v2: - Wrap is_signed_type() to avoid issues with pointer types because (foo *)1 isn't a compile time constant. - Remove the '+ 0' from __is_ok_unsigned(). This converted 'bool' to 'int' to avoid a compiler warning and is no longer needed because of the implicit conversion dome by ?:. include/linux/minmax.h | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-)