| Message ID | 20220610113427.908751-1-alexandr.lobakin@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | bitops: let optimize out non-atomic bitops on compile-time constants | expand |
Hi Alexander, On Fri, Jun 10, 2022 at 1:35 PM Alexander Lobakin <alexandr.lobakin@intel.com> wrote: > While I was working on converting some structure fields from a fixed > type to a bitmap, I started observing code size increase not only in > places where the code works with the converted structure fields, but > also where the converted vars were on the stack. That said, the > following code: > > DECLARE_BITMAP(foo, BITS_PER_LONG) = { }; // -> unsigned long foo[1]; > unsigned long bar = BIT(BAR_BIT); > unsigned long baz = 0; > > __set_bit(FOO_BIT, foo); > baz |= BIT(BAZ_BIT); > > BUILD_BUG_ON(!__builtin_constant_p(test_bit(FOO_BIT, foo)); > BUILD_BUG_ON(!__builtin_constant_p(bar & BAR_BIT)); > BUILD_BUG_ON(!__builtin_constant_p(baz & BAZ_BIT)); > > triggers the first assertion on x86_64, which means that the > compiler is unable to evaluate it to a compile-time initializer > when the architecture-specific bitop is used even if it's obvious. > I found that this is due to that many architecture-specific > non-atomic bitop implementations use inline asm or other hacks which > are faster or more robust when working with "real" variables (i.e. > fields from the structures etc.), but the compilers have no clue how > to optimize them out when called on compile-time constants. > > So, in order to let the compiler optimize out such cases, expand the > test_bit() and __*_bit() definitions with a compile-time condition > check, so that they will pick the generic C non-atomic bitop > implementations when all of the arguments passed are compile-time > constants, which means that the result will be a compile-time > constant as well and the compiler will produce more efficient and > simple code in 100% cases (no changes when there's at least one > non-compile-time-constant argument). > The condition itself: > > if ( > __builtin_constant_p(nr) && /* <- bit position is constant */ > __builtin_constant_p(!!addr) && /* <- compiler knows bitmap addr is > always either NULL or not */ > addr && /* <- bitmap addr is not NULL */ > __builtin_constant_p(*addr) /* <- compiler knows the value of > the target bitmap */ > ) > /* then pick the generic C variant > else > /* old code path, arch-specific > > I also tried __is_constexpr() as suggested by Andy, but it was > always returning 0 ('not a constant') for the 2,3 and 4th > conditions. > > The savings are architecture, compiler and compiler flags dependent, > for example, on x86_64 -O2: > > GCC 12: add/remove: 78/29 grow/shrink: 332/525 up/down: 31325/-61560 (-30235) > LLVM 13: add/remove: 79/76 grow/shrink: 184/537 up/down: 55076/-141892 (-86816) > LLVM 14: add/remove: 10/3 grow/shrink: 93/138 up/down: 3705/-6992 (-3287) > > and ARM64 (courtesy of Mark[0]): > > GCC 11: add/remove: 92/29 grow/shrink: 933/2766 up/down: 39340/-82580 (-43240) > LLVM 14: add/remove: 21/11 grow/shrink: 620/651 up/down: 12060/-15824 (-3764) > > And the following: > > DECLARE_BITMAP(flags, __IP_TUNNEL_FLAG_NUM) = { }; > __be16 flags; > > __set_bit(IP_TUNNEL_CSUM_BIT, flags); > > tun_flags = cpu_to_be16(*flags & U16_MAX); > > if (test_bit(IP_TUNNEL_VTI_BIT, flags)) > tun_flags |= VTI_ISVTI; > > BUILD_BUG_ON(!__builtin_constant_p(tun_flags)); > > doesn't blow up anymore, so that we now can e.g. use fixed bitmaps > in compile-time assertions etc. > > The series has been in intel-next for a while with no reported issues. > > From v1[1]: > * change 'gen_' prefixes to '_generic' to disambiguate from > 'generated' etc. (Mark); > * define a separate 'const_' set to use in the optimization to keep > the generic test_bit() atomic-safe (Marco); > * unify arch_{test,__*}_bit() as well and include them in the type > check; > * add more relevant and up-to-date bloat-o-meter results, including > ARM64 (me, Mark); > * pick a couple '*-by' tags (Mark, Yury). Thanks for the update! On m68k, using gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04), this blows up immediately with: CC kernel/bounds.s In file included from include/linux/bits.h:22, from include/linux/ratelimit_types.h:5, from include/linux/printk.h:9, from include/asm-generic/bug.h:22, from arch/m68k/include/asm/bug.h:32, from include/linux/bug.h:5, from include/linux/page-flags.h:10, from kernel/bounds.c:10: include/linux/bitops.h:72:21: error: ‘arch___set_bit’ undeclared here (not in a function); did you mean ‘const___set_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:75:1: note: in expansion of macro ‘__check_bitop_pr’ 75 | __check_bitop_pr(__set_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:75:1: note: in expansion of macro ‘__check_bitop_pr’ 75 | __check_bitop_pr(__set_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch___clear_bit’ undeclared here (not in a function); did you mean ‘const___clear_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:76:1: note: in expansion of macro ‘__check_bitop_pr’ 76 | __check_bitop_pr(__clear_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:76:1: note: in expansion of macro ‘__check_bitop_pr’ 76 | __check_bitop_pr(__clear_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch___change_bit’ undeclared here (not in a function); did you mean ‘const___change_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:77:1: note: in expansion of macro ‘__check_bitop_pr’ 77 | __check_bitop_pr(__change_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:77:1: note: in expansion of macro ‘__check_bitop_pr’ 77 | __check_bitop_pr(__change_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch___test_and_set_bit’ undeclared here (not in a function); did you mean ‘const___test_and_set_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:78:1: note: in expansion of macro ‘__check_bitop_pr’ 78 | __check_bitop_pr(__test_and_set_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:78:1: note: in expansion of macro ‘__check_bitop_pr’ 78 | __check_bitop_pr(__test_and_set_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch___test_and_clear_bit’ undeclared here (not in a function); did you mean ‘const___test_and_clear_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:79:1: note: in expansion of macro ‘__check_bitop_pr’ 79 | __check_bitop_pr(__test_and_clear_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:79:1: note: in expansion of macro ‘__check_bitop_pr’ 79 | __check_bitop_pr(__test_and_clear_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch___test_and_change_bit’ undeclared here (not in a function); did you mean ‘const___test_and_change_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:80:1: note: in expansion of macro ‘__check_bitop_pr’ 80 | __check_bitop_pr(__test_and_change_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:80:1: note: in expansion of macro ‘__check_bitop_pr’ 80 | __check_bitop_pr(__test_and_change_bit); | ^~~~~~~~~~~~~~~~ include/linux/bitops.h:72:21: error: ‘arch_test_bit’ undeclared here (not in a function); did you mean ‘_test_bit’? 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:72:9: note: in expansion of macro ‘__same_type’ 72 | __same_type(arch_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:81:1: note: in expansion of macro ‘__check_bitop_pr’ 81 | __check_bitop_pr(test_bit); | ^~~~~~~~~~~~~~~~ include/linux/compiler_types.h:293:27: error: expression in static assertion is not an integer 293 | #define __same_type(a, b) __builtin_types_compatible_p(typeof(a), typeof(b)) | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/build_bug.h:78:56: note: in definition of macro ‘__static_assert’ 78 | #define __static_assert(expr, msg, ...) _Static_assert(expr, msg) | ^~~~ include/linux/bitops.h:71:2: note: in expansion of macro ‘static_assert’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~~~ include/linux/bitops.h:71:16: note: in expansion of macro ‘__same_type’ 71 | static_assert(__same_type(const_##name, generic_##name) && \ | ^~~~~~~~~~~ include/linux/bitops.h:81:1: note: in expansion of macro ‘__check_bitop_pr’ 81 | __check_bitop_pr(test_bit); | ^~~~~~~~~~~~~~~~ Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
From: Geert Uytterhoeven <geert@linux-m68k.org> Date: Mon, 13 Jun 2022 09:35:16 +0200 > Hi Alexander, > > On Fri, Jun 10, 2022 at 1:35 PM Alexander Lobakin > <alexandr.lobakin@intel.com> wrote: > > While I was working on converting some structure fields from a fixed > > type to a bitmap, I started observing code size increase not only in > > places where the code works with the converted structure fields, but > > also where the converted vars were on the stack. That said, the > > following code: > > > > DECLARE_BITMAP(foo, BITS_PER_LONG) = { }; // -> unsigned long foo[1]; > > unsigned long bar = BIT(BAR_BIT); > > unsigned long baz = 0; > > > > __set_bit(FOO_BIT, foo); > > baz |=3D BIT(BAZ_BIT); > > > > BUILD_BUG_ON(!__builtin_constant_p(test_bit(FOO_BIT, foo)); > > BUILD_BUG_ON(!__builtin_constant_p(bar & BAR_BIT)); > > BUILD_BUG_ON(!__builtin_constant_p(baz & BAZ_BIT)); > > > > triggers the first assertion on x86_64, which means that the > > compiler is unable to evaluate it to a compile-time initializer > > when the architecture-specific bitop is used even if it's obvious. > > I found that this is due to that many architecture-specific > > non-atomic bitop implementations use inline asm or other hacks which > > are faster or more robust when working with "real" variables (i.e. > > fields from the structures etc.), but the compilers have no clue how > > to optimize them out when called on compile-time constants. > > > > So, in order to let the compiler optimize out such cases, expand the > > test_bit() and __*_bit() definitions with a compile-time condition > > check, so that they will pick the generic C non-atomic bitop > > implementations when all of the arguments passed are compile-time > > constants, which means that the result will be a compile-time > > constant as well and the compiler will produce more efficient and > > simple code in 100% cases (no changes when there's at least one > > non-compile-time-constant argument). > > The condition itself: > > > > if ( > > __builtin_constant_p(nr) && /* <- bit position is constant */ > > __builtin_constant_p(!!addr) && /* <- compiler knows bitmap addr is > > always either NULL or not */ > > addr && /* <- bitmap addr is not NULL */ > > __builtin_constant_p(*addr) /* <- compiler knows the value of > > the target bitmap */ > > ) > > /* then pick the generic C variant > > else > > /* old code path, arch-specific > > > > I also tried __is_constexpr() as suggested by Andy, but it was > > always returning 0 ('not a constant') for the 2,3 and 4th > > conditions. > > > > The savings are architecture, compiler and compiler flags dependent, > > for example, on x86_64 -O2: > > > > GCC 12: add/remove: 78/29 grow/shrink: 332/525 up/down: 31325/-61560 (-30235) > > LLVM 13: add/remove: 79/76 grow/shrink: 184/537 up/down: 55076/-141892 (-86816) > > LLVM 14: add/remove: 10/3 grow/shrink: 93/138 up/down: 3705/-6992 (-3287) > > > > and ARM64 (courtesy of Mark[0]): > > > > GCC 11: add/remove: 92/29 grow/shrink: 933/2766 up/down: 39340/-82580 (-43240) > > LLVM 14: add/remove: 21/11 grow/shrink: 620/651 up/down: 12060/-15824 (-3764) > > > > And the following: > > > > DECLARE_BITMAP(flags, __IP_TUNNEL_FLAG_NUM) = { }; > > __be16 flags; > > > > __set_bit(IP_TUNNEL_CSUM_BIT, flags); > > > > tun_flags = cpu_to_be16(*flags & U16_MAX); > > > > if (test_bit(IP_TUNNEL_VTI_BIT, flags)) > > tun_flags |= VTI_ISVTI; > > > > BUILD_BUG_ON(!__builtin_constant_p(tun_flags)); > > > > doesn't blow up anymore, so that we now can e.g. use fixed bitmaps > > in compile-time assertions etc. > > > > The series has been in intel-next for a while with no reported issues. > > > > From v1[1]: > > * change 'gen_' prefixes to '_generic' to disambiguate from > > 'generated' etc. (Mark); > > * define a separate 'const_' set to use in the optimization to keep > > the generic test_bit() atomic-safe (Marco); > > * unify arch_{test,__*}_bit() as well and include them in the type > > check; > > * add more relevant and up-to-date bloat-o-meter results, including > > ARM64 (me, Mark); > > * pick a couple '*-by' tags (Mark, Yury). > > Thanks for the update! > > On m68k, using gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04), this > blows up immediately with: Yeah I saw the kernel bot report already, sorry for that >_< Fixed in v3 already, will send in 1-2 days. > > CC kernel/bounds.s > In file included from include/linux/bits.h:22, [...] > | ^~~~~~~~~~~~~~~~ > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Thanks, Olek
Hi Olek, On Mon, Jun 13, 2022 at 4:28 PM Alexander Lobakin <alexandr.lobakin@intel.com> wrote: > From: Geert Uytterhoeven <geert@linux-m68k.org> > > On Fri, Jun 10, 2022 at 1:35 PM Alexander Lobakin > > <alexandr.lobakin@intel.com> wrote: > > On m68k, using gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04), this > > blows up immediately with: > > Yeah I saw the kernel bot report already, sorry for that >_< Fixed > in v3 already, will send in 1-2 days. Is it simple to fix? I might be able to give the fixed v2 a try before that. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
From: Geert Uytterhoeven <geert@linux-m68k.org> Date: Mon, 13 Jun 2022 17:22:30 +0200 > Hi Olek, Hi! > > On Mon, Jun 13, 2022 at 4:28 PM Alexander Lobakin > <alexandr.lobakin@intel.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > On Fri, Jun 10, 2022 at 1:35 PM Alexander Lobakin > > > <alexandr.lobakin@intel.com> wrote: > > > On m68k, using gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04), this > > > blows up immediately with: > > > > Yeah I saw the kernel bot report already, sorry for that >_< Fixed > > in v3 already, will send in 1-2 days. > > Is it simple to fix? > I might be able to give the fixed v2 a try before that. > Thanks! Oh, sorry for the late reply, was busy with stuff. It's linear, there are (after applying the series) some static inlines in arch/m68k/include/asm/bitops.h (those which were converted from macros in patch 3/6), 7 ops in total, you just need to create definitions with 'arch_' prefix for each of them, e.g. #define arch_test_bit test_bit #define arch___set_bit __set_bit // will be ___set_bit after 5/6 etc. Hope I explained it clear-ish :) > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds Thanks, Olek
While I was working on converting some structure fields from a fixed type to a bitmap, I started observing code size increase not only in places where the code works with the converted structure fields, but also where the converted vars were on the stack. That said, the following code: DECLARE_BITMAP(foo, BITS_PER_LONG) = { }; // -> unsigned long foo[1]; unsigned long bar = BIT(BAR_BIT); unsigned long baz = 0; __set_bit(FOO_BIT, foo); baz |= BIT(BAZ_BIT); BUILD_BUG_ON(!__builtin_constant_p(test_bit(FOO_BIT, foo)); BUILD_BUG_ON(!__builtin_constant_p(bar & BAR_BIT)); BUILD_BUG_ON(!__builtin_constant_p(baz & BAZ_BIT)); triggers the first assertion on x86_64, which means that the compiler is unable to evaluate it to a compile-time initializer when the architecture-specific bitop is used even if it's obvious. I found that this is due to that many architecture-specific non-atomic bitop implementations use inline asm or other hacks which are faster or more robust when working with "real" variables (i.e. fields from the structures etc.), but the compilers have no clue how to optimize them out when called on compile-time constants. So, in order to let the compiler optimize out such cases, expand the test_bit() and __*_bit() definitions with a compile-time condition check, so that they will pick the generic C non-atomic bitop implementations when all of the arguments passed are compile-time constants, which means that the result will be a compile-time constant as well and the compiler will produce more efficient and simple code in 100% cases (no changes when there's at least one non-compile-time-constant argument). The condition itself: if ( __builtin_constant_p(nr) && /* <- bit position is constant */ __builtin_constant_p(!!addr) && /* <- compiler knows bitmap addr is always either NULL or not */ addr && /* <- bitmap addr is not NULL */ __builtin_constant_p(*addr) /* <- compiler knows the value of the target bitmap */ ) /* then pick the generic C variant else /* old code path, arch-specific I also tried __is_constexpr() as suggested by Andy, but it was always returning 0 ('not a constant') for the 2,3 and 4th conditions. The savings are architecture, compiler and compiler flags dependent, for example, on x86_64 -O2: GCC 12: add/remove: 78/29 grow/shrink: 332/525 up/down: 31325/-61560 (-30235) LLVM 13: add/remove: 79/76 grow/shrink: 184/537 up/down: 55076/-141892 (-86816) LLVM 14: add/remove: 10/3 grow/shrink: 93/138 up/down: 3705/-6992 (-3287) and ARM64 (courtesy of Mark[0]): GCC 11: add/remove: 92/29 grow/shrink: 933/2766 up/down: 39340/-82580 (-43240) LLVM 14: add/remove: 21/11 grow/shrink: 620/651 up/down: 12060/-15824 (-3764) And the following: DECLARE_BITMAP(flags, __IP_TUNNEL_FLAG_NUM) = { }; __be16 flags; __set_bit(IP_TUNNEL_CSUM_BIT, flags); tun_flags = cpu_to_be16(*flags & U16_MAX); if (test_bit(IP_TUNNEL_VTI_BIT, flags)) tun_flags |= VTI_ISVTI; BUILD_BUG_ON(!__builtin_constant_p(tun_flags)); doesn't blow up anymore, so that we now can e.g. use fixed bitmaps in compile-time assertions etc. The series has been in intel-next for a while with no reported issues. From v1[1]: * change 'gen_' prefixes to '_generic' to disambiguate from 'generated' etc. (Mark); * define a separate 'const_' set to use in the optimization to keep the generic test_bit() atomic-safe (Marco); * unify arch_{test,__*}_bit() as well and include them in the type check; * add more relevant and up-to-date bloat-o-meter results, including ARM64 (me, Mark); * pick a couple '*-by' tags (Mark, Yury). Also available on my open GH[2]. [0] https://lore.kernel.org/all/Yp4GQFQYD32Rs9Cw@FVFF77S0Q05N [1] https://lore.kernel.org/all/20220606114908.962562-1-alexandr.lobakin@intel.com [2] https://github.com/alobakin/linux/commits/bitops Alexander Lobakin (6): ia64, processor: fix -Wincompatible-pointer-types in ia64_get_irr() bitops: always define asm-generic non-atomic bitops bitops: unify non-atomic bitops prototypes across architectures bitops: define const_*() versions of the non-atomics bitops: wrap non-atomic bitops with a transparent macro bitops: let optimize out non-atomic bitops on compile-time constants arch/alpha/include/asm/bitops.h | 28 ++-- arch/hexagon/include/asm/bitops.h | 23 ++- arch/ia64/include/asm/bitops.h | 40 ++--- arch/ia64/include/asm/processor.h | 2 +- arch/m68k/include/asm/bitops.h | 47 ++++-- arch/sh/include/asm/bitops-op32.h | 32 ++-- arch/sparc/include/asm/bitops_32.h | 18 +- arch/sparc/lib/atomic32.c | 12 +- arch/x86/include/asm/bitops.h | 22 +-- .../asm-generic/bitops/generic-non-atomic.h | 155 ++++++++++++++++++ .../bitops/instrumented-non-atomic.h | 35 ++-- include/asm-generic/bitops/non-atomic.h | 123 ++------------ include/linux/bitops.h | 50 ++++++ tools/include/asm-generic/bitops/non-atomic.h | 34 ++-- tools/include/linux/bitops.h | 16 ++ 15 files changed, 407 insertions(+), 230 deletions(-) create mode 100644 include/asm-generic/bitops/generic-non-atomic.h base-commit: 874c8ca1e60b2c564a48f7e7acc40d328d5c8733