Message-ID: <424323bd65354ecbad256657d5782bc2@AcuMS.aculab.com>
Date: Tue, 30 Jul 2024 12:03:24 +0000
From: David Laight <David.Laight@...LAB.COM>
To: 'Arnd Bergmann' <arnd@...nel.org>, Linus Torvalds
	<torvalds@...uxfoundation.org>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Jens Axboe
	<axboe@...nel.dk>, Matthew Wilcox <willy@...radead.org>, Christoph Hellwig
	<hch@...radead.org>, Andrew Morton <akpm@...ux-foundation.org>, "Andy
 Shevchenko" <andriy.shevchenko@...ux.intel.com>, Dan Carpenter
	<dan.carpenter@...aro.org>, "Jason A . Donenfeld" <Jason@...c4.com>,
	"pedro.falcato@...il.com" <pedro.falcato@...il.com>, Mateusz Guzik
	<mjguzik@...il.com>, "linux-mm@...ck.org" <linux-mm@...ck.org>, "Lorenzo
 Stoakes" <lorenzo.stoakes@...cle.com>
Subject: RE: [PATCH v2 1/8] minmax: Put all the clamp() definitions together

From: Arnd Bergmann
> Sent: 29 July 2024 23:25
..
> - My macros use __builtin_choose_expr() instead of ?: to
>   ensure that the arguments are constant, this produces a
>   relatively clear compiler warning when they are not.
>   Without that, I would expect random drivers to start
>   using MIN()/MAX() in places where it's not safe.

Yes, I think you either want temporaries or a constant check.
Losing the signedness check is less of a problem.
Since 'constantness' is only important for array sizes and
static initialisers the constant check is unlikely to be a problem.

Just adding __builtin_choose_expr((x)+(y),0,0) to the result
is enough and quite simple.
Then each argument is expanded 3 times.
(cf once for temporaries without a signedness check.))

> 
> - I went with the belts-and-suspenders version of MIN()/MAX()
>   that works when comparing a negative constant against
>   an unsigned one. This requires expanding each argument
>   four or five times instead of two, so you might still
>   want the simpler version (like MIN_T/MAX_T):

I'm not sure that is worth it, there are very few negative
values (except error values) in the entire kernel.
And where ever the value is used a carefully created negative
constant is likely to be promoted to unsigned.

I'm not sure I like the casts in MIN_T() and MAX_T() either.
As with min_t() someone will use too small a type.

OTOH umin(x, y) could now be defined as:
	min_t(u64, (x) + 0 + 0u, (y) + 0 + 0u)
(which will never sign extend).
Then some of the bloating min() that are known to generate
unsigned values can be changed to umin().

It is also worth noting that umin() is likely to generate
better code than an _min() (that doesn't have the type check)
because comparing a 32bit signed type (assumed positive) with
a 64bit value won't require the (pointless) sign extension
code (particularly nasty on 32bit).
In my tests the compiler optimised for the high 32bits being
zero (from the zero extend) - so extending to 64bits doesn't matter.

The typeof((x) + 0) in the signedness test (a different email)
that matches the C promotion of u8 and u16 might have been added
before I did the change to allow unsigned comparisons against
positive integer constants.

Another anti-bloat is to do all the type tests on the temporaries.
So something based on:
#define ok_unsigned(_x, x)
	(is_unsigned_type((typeof(_x)) ? 1 : if_constexpr((x), (x) >= 0, 0))) 

#define min(x, y)
	__auto_type _x = x;
	__auto_type _y = y;
	if (!is_signed_type(typeof(_x + _y))
		&& !(ok_unsigned(_x, x) && ok_unsigned(_y, y)))
	   error();
	__x < _y ? _x : _y;
which only has 3 expansions of each term.

(Of course the 'uniq' needed for nested calls don't help.)

	David

	

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Powered by blists - more mailing lists