| Message ID | 03601661326c4efba4e618ead15fa0e2@AcuMS.aculab.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | minmax: reduce compilation time | expand |
On Wed, Jul 24, 2024, at 16:30, David Laight wrote: > 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> This patch gives me a 10x speedup on compiling arch/x86/xen/setup.c, taking it from 15 seconds to 1.5 seconds for a defconfig+CONFIG_XEN build. >+/* Allow unsigned compares against non-negative signed constants. */ >+#define __is_ok_unsigned(x) \ >+ ((is_unsigned_type(typeof(x)) ? 0 : __if_constexpr(x, (x) + 0, -1)) >= 0) I don't understand why this return '0' for unsigned types, shouldn't this be ((is_unsigned_type(typeof(x)) ? 1 : __if_constexpr(x, (x) + 0, -1)) >= 0) ? Arnd
On Wed, 24 Jul 2024 at 09:49, Arnd Bergmann <arnd@kernel.org> wrote: > > I don't understand why this return '0' for unsigned types, > shouldn't this be > > ((is_unsigned_type(typeof(x)) ? 1 : __if_constexpr(x, (x) + 0, -1)) >= 0) Yes, that looks more logical. Plus why do that "__if_constexpr(x, (x) + 0, -1)) >= 0)" when it would appear to be more logical to move the comparison inside, ie __if_constexpr(x, (x) >= 0, 0) but I also don't see why that "+ 0" existed in the original. So there's presumably something I'm missing. I do get the feeling that the problem came from us being much too clever with out min/max macros, and now this series is doubling down instead of saying "it wasn't really worth it". Linus
From: Linus Torvalds > Sent: 24 July 2024 21:03 > > On Wed, 24 Jul 2024 at 09:49, Arnd Bergmann <arnd@kernel.org> wrote: > > > > I don't understand why this return '0' for unsigned types, > > shouldn't this be > > > > ((is_unsigned_type(typeof(x)) ? 1 : __if_constexpr(x, (x) + 0, -1)) >= 0) > > Yes, that looks more logical. The condition is '>= 0' so it doesn't matter if it is '1' or '0'. > Plus why do that "__if_constexpr(x, (x) + 0, -1)) >= 0)" when it would > appear to be more logical to move the comparison inside, ie > > __if_constexpr(x, (x) >= 0, 0) That gives a 'comparison of unsigned type against 0 is always true' warning. (The compiler generates that for code in the unused branches of both __builtin_choose_expr() and _Generic().) Moving the comparison to the outer level stops all such compiler warnings. > but I also don't see why that "+ 0" existed in the original. So > there's presumably something I'm missing. IIRC it was there to convert a 'bool' to 'int'. Somewhere the is a max(bool,bool) that could just be |. If may not be needed now the expansion is '(cond ? 0 : bool) >= 0' since the 'bool' picks up an (int) cast from the result of ?:. David > I do get the feeling that the problem came from us being much too > clever with out min/max macros, and now this series is doubling down > instead of saying "it wasn't really worth it". > > Linus - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David, kernel test robot noticed the following build errors: [auto build test ERROR on linux/master] [also build test ERROR on linus/master v6.10 next-20240725] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp-definitions-together/20240724-224832 base: linux/master patch link: https://lore.kernel.org/r/03601661326c4efba4e618ead15fa0e2%40AcuMS.aculab.com patch subject: [PATCH 4/7] minmax: Simplify signedness check config: mips-loongson1b_defconfig (https://download.01.org/0day-ci/archive/20240725/202407252100.fDFchC5O-lkp@intel.com/config) compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240725/202407252100.fDFchC5O-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202407252100.fDFchC5O-lkp@intel.com/ All errors (new ones prefixed by >>): >> crypto/skcipher.c:83:9: error: static assertion expression is not an integral constant expression return max(start, end_page); ^~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:70:19: note: expanded from macro 'max' #define max(x, y) __careful_cmp(max, x, y) ^~~~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:56:3: note: expanded from macro '__careful_cmp' __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y))) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:49:17: note: expanded from macro '__cmp_once' _Static_assert(__types_ok(x, y), \ ^~~~~~~~~~~~~~~~ include/linux/minmax.h:38:2: note: expanded from macro '__types_ok' ((__is_ok_signed(x) && __is_ok_signed(y)) || \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ crypto/skcipher.c:83:9: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression include/linux/minmax.h:70:19: note: expanded from macro 'max' #define max(x, y) __careful_cmp(max, x, y) ^ include/linux/minmax.h:56:3: note: expanded from macro '__careful_cmp' __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y))) ^ include/linux/minmax.h:49:17: note: expanded from macro '__cmp_once' _Static_assert(__types_ok(x, y), \ ^ include/linux/minmax.h:38:4: note: expanded from macro '__types_ok' ((__is_ok_signed(x) && __is_ok_signed(y)) || \ ^ include/linux/minmax.h:34:27: note: expanded from macro '__is_ok_signed' #define __is_ok_signed(x) is_signed_type(typeof((x) + 0)) ^ include/linux/compiler.h:273:32: note: expanded from macro 'is_signed_type' #define is_signed_type(type) (((type)(-1)) < (__force type)1) ^ 1 error generated. -- >> lib/lzo/lzo1x_compress.c:53:33: error: static assertion expression is not an integral constant expression const unsigned char *limit = min(ip_end, ip + MAX_ZERO_RUN_LENGTH + 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:63:19: note: expanded from macro 'min' #define min(x, y) __careful_cmp(min, x, y) ^~~~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:56:3: note: expanded from macro '__careful_cmp' __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y))) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/minmax.h:49:17: note: expanded from macro '__cmp_once' _Static_assert(__types_ok(x, y), \ ^~~~~~~~~~~~~~~~ include/linux/minmax.h:38:2: note: expanded from macro '__types_ok' ((__is_ok_signed(x) && __is_ok_signed(y)) || \ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ lib/lzo/lzo1x_compress.c:53:33: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression include/linux/minmax.h:63:19: note: expanded from macro 'min' #define min(x, y) __careful_cmp(min, x, y) ^ include/linux/minmax.h:56:3: note: expanded from macro '__careful_cmp' __cmp_once(op, x, y, __UNIQUE_ID(__x), __UNIQUE_ID(__y))) ^ include/linux/minmax.h:49:17: note: expanded from macro '__cmp_once' _Static_assert(__types_ok(x, y), \ ^ include/linux/minmax.h:38:4: note: expanded from macro '__types_ok' ((__is_ok_signed(x) && __is_ok_signed(y)) || \ ^ include/linux/minmax.h:34:27: note: expanded from macro '__is_ok_signed' #define __is_ok_signed(x) is_signed_type(typeof((x) + 0)) ^ include/linux/compiler.h:273:32: note: expanded from macro 'is_signed_type' #define is_signed_type(type) (((type)(-1)) < (__force type)1) ^ 1 error generated. vim +83 crypto/skcipher.c b286d8b1a690667 Herbert Xu 2016-11-22 75 b286d8b1a690667 Herbert Xu 2016-11-22 76 /* Get a spot of the specified length that does not straddle a page. b286d8b1a690667 Herbert Xu 2016-11-22 77 * The caller needs to ensure that there is enough space for this operation. b286d8b1a690667 Herbert Xu 2016-11-22 78 */ b286d8b1a690667 Herbert Xu 2016-11-22 79 static inline u8 *skcipher_get_spot(u8 *start, unsigned int len) b286d8b1a690667 Herbert Xu 2016-11-22 80 { b286d8b1a690667 Herbert Xu 2016-11-22 81 u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK); b286d8b1a690667 Herbert Xu 2016-11-22 82 b286d8b1a690667 Herbert Xu 2016-11-22 @83 return max(start, end_page); b286d8b1a690667 Herbert Xu 2016-11-22 84 } b286d8b1a690667 Herbert Xu 2016-11-22 85
From: kernel test robot <lkp@intel.com> > Sent: 25 July 2024 14:24 > > Hi David, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on linux/master] > [also build test ERROR on linus/master v6.10 next-20240725] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/David-Laight/minmax-Put-all-the-clamp- > definitions-together/20240724-224832 > base: linux/master > patch link: https://lore.kernel.org/r/03601661326c4efba4e618ead15fa0e2%40AcuMS.aculab.com > patch subject: [PATCH 4/7] minmax: Simplify signedness check > config: mips-loongson1b_defconfig (https://download.01.org/0day- > ci/archive/20240725/202407252100.fDFchC5O-lkp@intel.com/config) > compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project > 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a) > reproduce (this is a W=1 build): (https://download.01.org/0day- > ci/archive/20240725/202407252100.fDFchC5O-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202407252100.fDFchC5O-lkp@intel.com/ > > All errors (new ones prefixed by >>): ... > b286d8b1a690667 Herbert Xu 2016-11-22 75 > b286d8b1a690667 Herbert Xu 2016-11-22 76 /* Get a spot of the specified length that does not straddle a page. > b286d8b1a690667 Herbert Xu 2016-11-22 77 * The caller needs to ensure that there is enough space for this operation. > b286d8b1a690667 Herbert Xu 2016-11-22 78 */ > b286d8b1a690667 Herbert Xu 2016-11-22 79 static inline u8 *skcipher_get_spot(u8 *start, unsigned int len) > b286d8b1a690667 Herbert Xu 2016-11-22 80 { > b286d8b1a690667 Herbert Xu 2016-11-22 81 u8 *end_page = (u8 *)(((unsigned long)(start + len - 1)) & PAGE_MASK); > b286d8b1a690667 Herbert Xu 2016-11-22 82 > b286d8b1a690667 Herbert Xu 2016-11-22 @83 return max(start, end_page); > b286d8b1a690667 Herbert Xu 2016-11-22 84 } > b286d8b1a690667 Herbert Xu 2016-11-22 85 I thought this version supported that :-( Certainly I've hit and fixed this before. Using min/max() on pointers just makes it all more horrid. The problem is that is_signed_type() isn't 'constant enough' for pointers. Supporting pointers may require yet another (bloating) _is_constexpr() call. Or add a simpler min_ptr() that includes a 'pointer' check. But this might only be reported by clang with W=1. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, 25 Jul 2024 at 02:01, David Laight <David.Laight@aculab.com> wrote: > > The condition is '>= 0' so it doesn't matter if it is '1' or '0'. Yes, but that's because the whole conditional is so inexplicably complex. But the explanation is: > That gives a 'comparison of unsigned type against 0 is always true' warning. > (The compiler generates that for code in the unused branches of both > __builtin_choose_expr() and _Generic().) > Moving the comparison to the outer level stops all such compiler warnings. Christ. This whole series is a nightmare of "add complexity to deal with stupid issues". But the kernel test robot clearly found even more issues. I think we need to just go back to the old code. It was stupid and limited and caused us to have to be more careful about types than was strictly necessary. But it was also about a million times simpler, and didn't cause build time regressions. Linus
On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote: > On Thu, 25 Jul 2024 at 02:01, David Laight <David.Laight@aculab.com> wrote: > > > > The condition is '>= 0' so it doesn't matter if it is '1' or '0'. > > Yes, but that's because the whole conditional is so inexplicably complex. > > But the explanation is: > > > That gives a 'comparison of unsigned type against 0 is always true' warning. > > (The compiler generates that for code in the unused branches of both > > __builtin_choose_expr() and _Generic().) > > Moving the comparison to the outer level stops all such compiler warnings. > > Christ. This whole series is a nightmare of "add complexity to deal > with stupid issues". > > But the kernel test robot clearly found even more issues. > > I think we need to just go back to the old code. It was stupid and > limited and caused us to have to be more careful about types than was > strictly necessary. The problem is simply reverting reveals that seemingly a _ton_ of code has come to rely on the relaxed conditions. When I went to gather the numbers for my initial report I had to manually fix up every case which was rather painful, and that was just a defconfig + a few extra options. allmodconfig is likely to be a hellscape. I've not dug deep into the ins + outs of this, so forgive me for being vague (Arnd has a far clearer understanding) - but it seems that the majority of the complexity comes from having to absolutely ensure all this works for compile-time constant values. Arnd had a look through and determined there weren't _too_ many cases where we need this (for instance array sizes). So I wonder whether we can't just vastly simplify this stuff (and reducing the macro expansion hell) for the usual case, and implement something like cmin()/cmax() or whatever for the true-constant cases? > > But it was also about a million times simpler, and didn't cause build > time regressions. > :) > Linus
From: Lorenzo Stoakes > Sent: 26 July 2024 10:44 > > On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote: > > On Thu, 25 Jul 2024 at 02:01, David Laight <David.Laight@aculab.com> wrote: > > > > > > The condition is '>= 0' so it doesn't matter if it is '1' or '0'. > > > > Yes, but that's because the whole conditional is so inexplicably complex. > > > > But the explanation is: > > > > > That gives a 'comparison of unsigned type against 0 is always true' warning. > > > (The compiler generates that for code in the unused branches of both > > > __builtin_choose_expr() and _Generic().) > > > Moving the comparison to the outer level stops all such compiler warnings. > > > > Christ. This whole series is a nightmare of "add complexity to deal > > with stupid issues". > > > > But the kernel test robot clearly found even more issues. > > > > I think we need to just go back to the old code. It was stupid and > > limited and caused us to have to be more careful about types than was > > strictly necessary. > > The problem is simply reverting reveals that seemingly a _ton_ of code has > come to rely on the relaxed conditions. > > When I went to gather the numbers for my initial report I had to manually > fix up every case which was rather painful, and that was just a defconfig + > a few extra options. allmodconfig is likely to be a hellscape. > > I've not dug deep into the ins + outs of this, so forgive me for being > vague (Arnd has a far clearer understanding) - but it seems that the > majority of the complexity comes from having to absolutely ensure all this > works for compile-time constant values. The problems arise due to some odd uses, not just the requirement for compile-time constants for on-stack array sizes. The test robot report is for a test between pointers. I thought there was one in the build I do - and it doesn't usually generate a warning. This one is related to the different between a 'compile time constant' and a 'constant integer expression'. This is due to is_unsigned_type(t) being (t)-1 > 0 but (type *)x not being 'constant enough' unless 'x' is zero. Fixable by something like: (__if_constexpr((t)-1, (t)-1, 1) > 0) But that requires two expansions of the type. Now the type could be that of the unique temporary - but that would make it all more complicated. There are fewer min/max of pointers than when constants are needed. So requiring them be min_ptr() wouldn't be a massive change. > Arnd had a look through and determined there weren't _too_ many cases where > we need this (for instance array sizes). > > So I wonder whether we can't just vastly simplify this stuff (and reducing > the macro expansion hell) for the usual case, and implement something like > cmin()/cmax() or whatever for the true-constant cases? I did do that in a patch set from much earlier in the year. But Linus said they'd need to be MIN() and MAX() and that requires changes to a few places where those are already defined. > > But it was also about a million times simpler, and didn't cause build > > time regressions. Just bugs because people did min_t(short, 65536, 128) and didn't expect zero. It has to be said that the chances of a min(negative_value, unsigned_constant) appearing are pretty slim. All these tests are there to trap that case. There is always the option of disabling the tests for 'normal' build, but leaving them there for (say) the W=1 builds. Then it won't matter as much if the tests slow down the build a little. I think I have tried a W=1 build - but there are too many warnings/errors from other places to get anywhere. A lot are for 'unsigned_var >= 0' in paths that get optimised away. The compiler doesn't help! David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Jul 26, 2024 at 12:57:43PM GMT, David Laight wrote: > From: Lorenzo Stoakes > > Sent: 26 July 2024 10:44 > > > > On Thu, Jul 25, 2024 at 10:02:45AM GMT, Linus Torvalds wrote: [snip] > > > Christ. This whole series is a nightmare of "add complexity to deal > > > with stupid issues". > > > > > > But the kernel test robot clearly found even more issues. > > > > > > I think we need to just go back to the old code. It was stupid and > > > limited and caused us to have to be more careful about types than was > > > strictly necessary. > > > > The problem is simply reverting reveals that seemingly a _ton_ of code has > > come to rely on the relaxed conditions. > > > > When I went to gather the numbers for my initial report I had to manually > > fix up every case which was rather painful, and that was just a defconfig + > > a few extra options. allmodconfig is likely to be a hellscape. > > > > I've not dug deep into the ins + outs of this, so forgive me for being > > vague (Arnd has a far clearer understanding) - but it seems that the > > majority of the complexity comes from having to absolutely ensure all this > > works for compile-time constant values. > > The problems arise due to some odd uses, not just the requirement for compile-time > constants for on-stack array sizes. Odd implies not many, same argument applies. [snip] > > > Arnd had a look through and determined there weren't _too_ many cases where > > we need this (for instance array sizes). > > > > So I wonder whether we can't just vastly simplify this stuff (and reducing > > the macro expansion hell) for the usual case, and implement something like > > cmin()/cmax() or whatever for the true-constant cases? > > I did do that in a patch set from much earlier in the year. > But Linus said they'd need to be MIN() and MAX() and that requires changes > to a few places where those are already defined. OK, so what's stopping you from doing that? In order to implement a MIN()/MAX() you'd need to change call sites only (should be a managable amount), so we can change this too? I'm concerned that a solution is being proposed here and then handwaved away... Unfortunately a revert is no longer possible (I had to fix up 33 call sites manually just for my defconfig build to compare perf before/after), so if the intent is to eliminate the complexity, then we need a practical way forward. > > > > But it was also about a million times simpler, and didn't cause build > > > time regressions. > > Just bugs because people did min_t(short, 65536, 128) and didn't expect zero. > > It has to be said that the chances of a min(negative_value, unsigned_constant) > appearing are pretty slim. > All these tests are there to trap that case. > > There is always the option of disabling the tests for 'normal' build, but > leaving them there for (say) the W=1 builds. > Then it won't matter as much if the tests slow down the build a little. Very much NAK disabling tests as a solution! Also leaving them for a build that's apparently broken... yeah not a fan. > > I think I have tried a W=1 build - but there are too many warnings/errors > from other places to get anywhere. > A lot are for 'unsigned_var >= 0' in paths that get optimised away. > The compiler doesn't help! > > 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 900eec7a28e5..d3ac65c1add7 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,17 @@ #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) +/* Allow unsigned compares against non-negative signed constants. */ +#define __is_ok_unsigned(x) \ + ((is_unsigned_type(typeof(x)) ? 0 : __if_constexpr(x, (x) + 0, -1)) >= 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) +/* Check for signed after promoting unsigned char/short to int */ +#define __is_ok_signed(x) is_signed_type(typeof((x) + 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)) +/* 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> --- include/linux/minmax.h | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)