* [PATCH] Introduce a boolean "single_bit_set" function. @ 2009-04-23 17:43 Robert P. J. Day 2009-04-23 19:57 ` David Daney 2009-05-28 12:21 ` Petr Tesarik 0 siblings, 2 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-04-23 17:43 UTC (permalink / raw) To: Linux Kernel Mailing List; +Cc: Andrew Morton A boolean single_bit_set() routine would simplify the numerous constructs of the form (((n & (n - 1)) == 0)) when testing for single-bitness. Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> --- This is similar to the current is_power_of_2() routine defined in include/linux/log2.h, which is mathematically identical but, semantically, should be defined independently just so the code is more readable. I'm open to an alternative function name. diff --git a/include/linux/bitops.h b/include/linux/bitops.h index 6182913..1c0c840 100644 --- a/include/linux/bitops.h +++ b/include/linux/bitops.h @@ -45,6 +45,13 @@ static inline unsigned long hweight_long(unsigned long w) return sizeof(w) == 4 ? hweight32(w) : hweight64(w); } +static inline __attribute__((const)) +bool single_bit_set(unsigned long n) +{ + return (n != 0 && ((n & (n - 1)) == 0)); +} + + /** * rol32 - rotate a 32-bit value left * @word: value to rotate ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 17:43 [PATCH] Introduce a boolean "single_bit_set" function Robert P. J. Day @ 2009-04-23 19:57 ` David Daney 2009-04-23 20:11 ` Robert P. J. Day 2009-04-23 23:57 ` Andrew Morton 2009-05-28 12:21 ` Petr Tesarik 1 sibling, 2 replies; 17+ messages in thread From: David Daney @ 2009-04-23 19:57 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Linux Kernel Mailing List, Andrew Morton Robert P. J. Day wrote: > A boolean single_bit_set() routine would simplify the numerous > constructs of the form (((n & (n - 1)) == 0)) when testing for > single-bitness. > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > --- > > This is similar to the current is_power_of_2() routine defined in > include/linux/log2.h, which is mathematically identical but, > semantically, should be defined independently just so the code is more > readable. > > I'm open to an alternative function name. > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > index 6182913..1c0c840 100644 > --- a/include/linux/bitops.h > +++ b/include/linux/bitops.h > @@ -45,6 +45,13 @@ static inline unsigned long hweight_long(unsigned long w) > return sizeof(w) == 4 ? hweight32(w) : hweight64(w); > } > > +static inline __attribute__((const)) > +bool single_bit_set(unsigned long n) > +{ > + return (n != 0 && ((n & (n - 1)) == 0)); > +} > + > + It would be nice to be able to override this per architecture. For example a more efficient implementation on CPUs that have a population count instruction (__builtin_popcountl()) might be: static inline __attribute__((const)) bool singe_bit_set(unsigned long n) { return __builtin_popcountl(n) == 1; } Also, are we still putting 'inline' everywhere? David Daney ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 19:57 ` David Daney @ 2009-04-23 20:11 ` Robert P. J. Day 2009-04-23 23:57 ` Andrew Morton 1 sibling, 0 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-04-23 20:11 UTC (permalink / raw) To: David Daney; +Cc: Linux Kernel Mailing List, Andrew Morton On Thu, 23 Apr 2009, David Daney wrote: > Robert P. J. Day wrote: > > A boolean single_bit_set() routine would simplify the numerous > > constructs of the form (((n & (n - 1)) == 0)) when testing for > > single-bitness. > > > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > > > --- > > > > This is similar to the current is_power_of_2() routine defined in > > include/linux/log2.h, which is mathematically identical but, > > semantically, should be defined independently just so the code is more > > readable. > > > > I'm open to an alternative function name. > > > > diff --git a/include/linux/bitops.h b/include/linux/bitops.h > > index 6182913..1c0c840 100644 > > --- a/include/linux/bitops.h > > +++ b/include/linux/bitops.h > > @@ -45,6 +45,13 @@ static inline unsigned long hweight_long(unsigned long w) > > return sizeof(w) == 4 ? hweight32(w) : hweight64(w); > > } > > > > +static inline __attribute__((const)) > > +bool single_bit_set(unsigned long n) > > +{ > > + return (n != 0 && ((n & (n - 1)) == 0)); > > +} > > + > > + > > > It would be nice to be able to override this per architecture. sure, that makes sense. but in the meantime, there's nothing to keep from starting the process and, arch by arch, overriding it down the road as it becomes convenient. > Also, are we still putting 'inline' everywhere? beats me. are we? and, just to be definitively pedantic about this, for maximum readability, i think it would be nice to define *both* the function and its converse: if (exactly_one_bit_set()) if (more_than_one_bit_set()) or something to that effect. i'll leave the final naming decisions up to others higher up the food chain. rday p.s. you can see the potential simplification by running, at the top of the kernel tree: $ grep -Ern "([^\(\)]+) ?\& ?\(\1 ?- ?1\)" . some of those represent power of 2 semantics, while others are the single bit thingy. and others are just weird. ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 19:57 ` David Daney 2009-04-23 20:11 ` Robert P. J. Day @ 2009-04-23 23:57 ` Andrew Morton 2009-04-24 10:40 ` Robert P. J. Day 2009-04-24 13:51 ` Robert P. J. Day 1 sibling, 2 replies; 17+ messages in thread From: Andrew Morton @ 2009-04-23 23:57 UTC (permalink / raw) To: David Daney; +Cc: rpjday, linux-kernel On Thu, 23 Apr 2009 12:57:11 -0700 David Daney <ddaney@caviumnetworks.com> wrote: > > +static inline __attribute__((const)) > > +bool single_bit_set(unsigned long n) > > +{ > > + return (n != 0 && ((n & (n - 1)) == 0)); > > +} > > + > > + > > > It would be nice to be able to override this per architecture. > > For example a more efficient implementation on CPUs that have a > population count instruction (__builtin_popcountl()) might be: > > static inline __attribute__((const)) > bool singe_bit_set(unsigned long n) > { > return __builtin_popcountl(n) == 1; > } Already done, via hweight_long(). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 23:57 ` Andrew Morton @ 2009-04-24 10:40 ` Robert P. J. Day 2009-04-24 17:46 ` Andrew Morton 2009-04-24 13:51 ` Robert P. J. Day 1 sibling, 1 reply; 17+ messages in thread From: Robert P. J. Day @ 2009-04-24 10:40 UTC (permalink / raw) To: Andrew Morton; +Cc: David Daney, Linux Kernel Mailing List On Thu, 23 Apr 2009, Andrew Morton wrote: > On Thu, 23 Apr 2009 12:57:11 -0700 > David Daney <ddaney@caviumnetworks.com> wrote: > > > > +static inline __attribute__((const)) > > > +bool single_bit_set(unsigned long n) > > > +{ > > > + return (n != 0 && ((n & (n - 1)) == 0)); > > > +} > > > + > > > + > > > > > > It would be nice to be able to override this per architecture. > > > > For example a more efficient implementation on CPUs that have a > > population count instruction (__builtin_popcountl()) might be: > > > > static inline __attribute__((const)) > > bool singe_bit_set(unsigned long n) > > { > > return __builtin_popcountl(n) == 1; > > } > > Already done, via hweight_long(). so it would be a simple matter to define the bit set boolean in terms of hweight_long(), yes? so what about, in bitops.h: static inline bool exactly_one_bit_set(unsigned long w) { return hweight_long(w) == 1; } static inline bool more_than_one_bit_set(unsigned long w) { return hweight_long(w) > 1; } or something to that effect, *if* people think it's worth it. obviously, none of the above is strictly necessary, but it would make a lot of code semantically cleaner. rday p.s. i notice that, even in a single header file like bitops.h, there is a mixture of both "inline" and "__inline__". what's the recommended choice these days? -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-24 10:40 ` Robert P. J. Day @ 2009-04-24 17:46 ` Andrew Morton 2009-04-25 22:09 ` Robert P. J. Day 2009-06-29 18:15 ` Petr Tesarik 0 siblings, 2 replies; 17+ messages in thread From: Andrew Morton @ 2009-04-24 17:46 UTC (permalink / raw) To: Robert P. J. Day; +Cc: David Daney, Linux Kernel Mailing List On Fri, 24 Apr 2009 06:40:39 -0400 (EDT) "Robert P. J. Day" <rpjday@crashcourse.ca> wrote: > so it would be a simple matter to define the bit set boolean in > terms of hweight_long(), yes? so what about, in bitops.h: > > static inline bool > exactly_one_bit_set(unsigned long w) > { > return hweight_long(w) == 1; > } > > static inline bool > more_than_one_bit_set(unsigned long w) > { > return hweight_long(w) > 1; > } > > or something to that effect, *if* people think it's worth it. > obviously, none of the above is strictly necessary, but it would make > a lot of code semantically cleaner. > Doing plain old if (hweight32(foo) == 1) (say) at the call sites quite clearly expresses what the code is trying to do. > rday > > p.s. i notice that, even in a single header file like bitops.h, there > is a mixture of both "inline" and "__inline__". what's the > recommended choice these days? `inline'. Or uninline the function ;) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-24 17:46 ` Andrew Morton @ 2009-04-25 22:09 ` Robert P. J. Day 2009-06-29 18:15 ` Petr Tesarik 1 sibling, 0 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-04-25 22:09 UTC (permalink / raw) To: Andrew Morton; +Cc: Linux Kernel Mailing List On Fri, 24 Apr 2009, Andrew Morton wrote: > On Fri, 24 Apr 2009 06:40:39 -0400 (EDT) "Robert P. J. Day" <rpjday@crashcourse.ca> wrote: > > > so it would be a simple matter to define the bit set boolean in > > terms of hweight_long(), yes? so what about, in bitops.h: > > > > static inline bool > > exactly_one_bit_set(unsigned long w) > > { > > return hweight_long(w) == 1; > > } > > > > static inline bool > > more_than_one_bit_set(unsigned long w) > > { > > return hweight_long(w) > 1; > > } > > > > or something to that effect, *if* people think it's worth it. > > obviously, none of the above is strictly necessary, but it would make > > a lot of code semantically cleaner. > > > > Doing plain old > > if (hweight32(foo) == 1) > > (say) at the call sites quite clearly expresses what the code is trying > to do. yes, that seems reasonable. but would you really prefer "hweight32" over "hweight_long"? rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-24 17:46 ` Andrew Morton 2009-04-25 22:09 ` Robert P. J. Day @ 2009-06-29 18:15 ` Petr Tesarik 2009-06-29 18:50 ` Robert P. J. Day 1 sibling, 1 reply; 17+ messages in thread From: Petr Tesarik @ 2009-06-29 18:15 UTC (permalink / raw) To: Andrew Morton; +Cc: Robert P. J. Day, David Daney, Linux Kernel Mailing List Andrew Morton píše v Pá 24. 04. 2009 v 10:46 -0700: > On Fri, 24 Apr 2009 06:40:39 -0400 (EDT) "Robert P. J. Day" <rpjday@crashcourse.ca> wrote: > > > so it would be a simple matter to define the bit set boolean in > > terms of hweight_long(), yes? so what about, in bitops.h: > > > > static inline bool > > exactly_one_bit_set(unsigned long w) > > { > > return hweight_long(w) == 1; > > } > > > > static inline bool > > more_than_one_bit_set(unsigned long w) > > { > > return hweight_long(w) > 1; > > } > > Andrew, you must be kidding! Are you seriously suggesting to replace a simple and instruction with a call to an extern library function with 17 instructions (not including the call and ret)? I'd better check the use of hweight in the kernel to eradicate as many calls to it as possible... Petr Tesarik ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-06-29 18:15 ` Petr Tesarik @ 2009-06-29 18:50 ` Robert P. J. Day 2009-06-30 6:12 ` Petr Tesarik 0 siblings, 1 reply; 17+ messages in thread From: Robert P. J. Day @ 2009-06-29 18:50 UTC (permalink / raw) To: Petr Tesarik; +Cc: Andrew Morton, David Daney, Linux Kernel Mailing List [-- Attachment #1: Type: TEXT/PLAIN, Size: 1847 bytes --] On Mon, 29 Jun 2009, Petr Tesarik wrote: > Andrew Morton píše v Pá 24. 04. 2009 v 10:46 -0700: > > On Fri, 24 Apr 2009 06:40:39 -0400 (EDT) "Robert P. J. Day" <rpjday@crashcourse.ca> wrote: > > > > > so it would be a simple matter to define the bit set boolean in > > > terms of hweight_long(), yes? so what about, in bitops.h: > > > > > > static inline bool > > > exactly_one_bit_set(unsigned long w) > > > { > > > return hweight_long(w) == 1; > > > } > > > > > > static inline bool > > > more_than_one_bit_set(unsigned long w) > > > { > > > return hweight_long(w) > 1; > > > } > > > > > Andrew, you must be kidding! Are you seriously suggesting to replace > a simple and instruction with a call to an extern library function > with 17 instructions (not including the call and ret)? > > I'd better check the use of hweight in the kernel to eradicate as > many calls to it as possible... since i originally muttered about this, the rationale behind it was not for performance (obviously), but for semantic clarification, so that when you saw the expression "n & (n-1)", it was more obvious which test you were doing semantically: 1) is n a power of 2? 2) does n represent a single set bit? nothing ever came of that, but that was the thinking behind it. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-06-29 18:50 ` Robert P. J. Day @ 2009-06-30 6:12 ` Petr Tesarik 2009-06-30 10:18 ` Robert P. J. Day 0 siblings, 1 reply; 17+ messages in thread From: Petr Tesarik @ 2009-06-30 6:12 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Andrew Morton, David Daney, Linux Kernel Mailing List Robert P. J. Day píše v Po 29. 06. 2009 v 14:50 -0400: > On Mon, 29 Jun 2009, Petr Tesarik wrote: > > > Andrew Morton píše v Pá 24. 04. 2009 v 10:46 -0700: > > > On Fri, 24 Apr 2009 06:40:39 -0400 (EDT) "Robert P. J. Day" <rpjday@crashcourse.ca> wrote: > > > > > > > so it would be a simple matter to define the bit set boolean in > > > > terms of hweight_long(), yes? so what about, in bitops.h: > > > > > > > > static inline bool > > > > exactly_one_bit_set(unsigned long w) > > > > { > > > > return hweight_long(w) == 1; > > > > } > > > > > > > > static inline bool > > > > more_than_one_bit_set(unsigned long w) > > > > { > > > > return hweight_long(w) > 1; > > > > } > > > > > > > > Andrew, you must be kidding! Are you seriously suggesting to replace > > a simple and instruction with a call to an extern library function > > with 17 instructions (not including the call and ret)? > > > > I'd better check the use of hweight in the kernel to eradicate as > > many calls to it as possible... > > since i originally muttered about this, the rationale behind it was > not for performance (obviously), but for semantic clarification, so > that when you saw the expression "n & (n-1)", it was more obvious > which test you were doing semantically: > > 1) is n a power of 2? > 2) does n represent a single set bit? > > nothing ever came of that, but that was the thinking behind it. Yes, I can remember and I would still appreciate it. It's always better to show _what_ the code does rather than _how_ it does it. IIRC Andrew rejected your patch on the grounds that it is possible to replace the expression "n & (n-1)" with "hweight(n) == 1" if one wants to show that it really tests for a single bit set. But I don't like his proposal quite as much as yours, because of the big overhead. In short, if you re-post your patch, I'll gladly join you in the battle of getting it in. ;-) Petr Tesarik ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-06-30 6:12 ` Petr Tesarik @ 2009-06-30 10:18 ` Robert P. J. Day 0 siblings, 0 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-06-30 10:18 UTC (permalink / raw) To: Petr Tesarik; +Cc: Andrew Morton, David Daney, Linux Kernel Mailing List [-- Attachment #1: Type: TEXT/PLAIN, Size: 2235 bytes --] On Tue, 30 Jun 2009, Petr Tesarik wrote: > Robert P. J. Day píše v Po 29. 06. 2009 v 14:50 -0400: > > since i originally muttered about this, the rationale behind it > > was not for performance (obviously), but for semantic > > clarification, so that when you saw the expression "n & (n-1)", it > > was more obvious which test you were doing semantically: > > > > 1) is n a power of 2? > > 2) does n represent a single set bit? > > > > nothing ever came of that, but that was the thinking behind it. > > Yes, I can remember and I would still appreciate it. It's always > better to show _what_ the code does rather than _how_ it does it. > > IIRC Andrew rejected your patch on the grounds that it is possible > to replace the expression "n & (n-1)" with "hweight(n) == 1" if one > wants to show that it really tests for a single bit set. But I don't > like his proposal quite as much as yours, because of the big > overhead. > > In short, if you re-post your patch, I'll gladly join you in the > battle of getting it in. ;-) i never meant for this to turn into a pitched battle of philosophies -- it just seemed like a simple way to make the code clearer, particularly since some of the patches i've submitted allowed for the removal of comments like "test that blocksize is a power of 2" given that what you're testing is now painfully obvious. :-) if someone has a quick, simple and performance non-crippling suggestion for this, i'm all ears. but there's no point thinking about it if it's actually going to cause performance issues. rday p.s. a simple grep to find potential cleanup of the form n&(n-1): grep -Ern "([^\(\)]+) ?\& ?\(\1 ?- ?1\)" * | less -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 23:57 ` Andrew Morton 2009-04-24 10:40 ` Robert P. J. Day @ 2009-04-24 13:51 ` Robert P. J. Day 1 sibling, 0 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-04-24 13:51 UTC (permalink / raw) To: Andrew Morton; +Cc: David Daney, Linux Kernel Mailing List On Thu, 23 Apr 2009, Andrew Morton wrote: > On Thu, 23 Apr 2009 12:57:11 -0700 > David Daney <ddaney@caviumnetworks.com> wrote: > > > > +static inline __attribute__((const)) > > > +bool single_bit_set(unsigned long n) > > > +{ > > > + return (n != 0 && ((n & (n - 1)) == 0)); > > > +} > > > + > > > + > > > > > > It would be nice to be able to override this per architecture. > > > > For example a more efficient implementation on CPUs that have a > > population count instruction (__builtin_popcountl()) might be: > > > > static inline __attribute__((const)) > > bool singe_bit_set(unsigned long n) > > { > > return __builtin_popcountl(n) == 1; > > } > > Already done, via hweight_long(). i'm guessing that taking advantage of helper functions like that might simplify code like, say, this from fs/nfs/internal.h: /* * Determine the actual block size (and log2 thereof) */ static inline unsigned long nfs_block_bits(unsigned long bsize, unsigned char *nrbitsp) { /* make sure blocksize is a power of two */ if ((bsize & (bsize - 1)) || nrbitsp) { unsigned char nrbits; for (nrbits = 31; nrbits && !(bsize & (1 << nrbits)); nrbits--) ; bsize = 1 << nrbits; if (nrbitsp) *nrbitsp = nrbits; } return bsize; } surely, there's a simpler way to write that. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-04-23 17:43 [PATCH] Introduce a boolean "single_bit_set" function Robert P. J. Day 2009-04-23 19:57 ` David Daney @ 2009-05-28 12:21 ` Petr Tesarik 2009-05-28 12:27 ` Robert P. J. Day 2009-05-28 12:32 ` Robert P. J. Day 1 sibling, 2 replies; 17+ messages in thread From: Petr Tesarik @ 2009-05-28 12:21 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Linux Kernel Mailing List, Andrew Morton Robert P. J. Day píše v Čt 23. 04. 2009 v 13:43 -0400: > A boolean single_bit_set() routine would simplify the numerous > constructs of the form (((n & (n - 1)) == 0)) when testing for > single-bitness. > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > --- > > This is similar to the current is_power_of_2() routine defined in > include/linux/log2.h, which is mathematically identical but, > semantically, should be defined independently just so the code is more > readable. > > I'm open to an alternative function name. ispow2() ? Because what it really does is to check that a value is a power of two, doesn'it. Regards, Petr Tesarik P.S. aka I want my bikeshed mathematical! ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-05-28 12:21 ` Petr Tesarik @ 2009-05-28 12:27 ` Robert P. J. Day 2009-05-28 12:32 ` Robert P. J. Day 1 sibling, 0 replies; 17+ messages in thread From: Robert P. J. Day @ 2009-05-28 12:27 UTC (permalink / raw) To: Petr Tesarik; +Cc: Linux Kernel Mailing List, Andrew Morton [-- Attachment #1: Type: TEXT/PLAIN, Size: 2253 bytes --] On Thu, 28 May 2009, Petr Tesarik wrote: > Robert P. J. Day píše v Čt 23. 04. 2009 v 13:43 -0400: > > A boolean single_bit_set() routine would simplify the numerous > > constructs of the form (((n & (n - 1)) == 0)) when testing for > > single-bitness. > > > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > > > --- > > > > This is similar to the current is_power_of_2() routine defined in > > include/linux/log2.h, which is mathematically identical but, > > semantically, should be defined independently just so the code is > > more readable. > > > > I'm open to an alternative function name. > > ispow2() ? > > Because what it really does is to check that a value is a power of > two, doesn'it. yes, mathematically it's identical, but *semantically*, i think it's worth distinguishing between those two tests. there's still a number of places in the code that can be rewritten with "is power of 2" tests, but that rewriting makes sense only if that's really what you're asking, as in checking an alleged block size value you've been passed, or something similar. OTOH, there are lots of places in the code that have to verify that only a single (flag?) bit has been set, and that code would read a lot better using a better worded test. someone already pointed out that perhaps one of the hweight*() routines from include/linux/bitops.h would be appropriate. and, yes, i realize that there would be no functional change, but rewriting tests like this not only makes the code more readable, it allows for the removal of accompanying comments that have to explain to the reader what's going on. but i'll leave the decision to those higher up the food chain. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-05-28 12:21 ` Petr Tesarik 2009-05-28 12:27 ` Robert P. J. Day @ 2009-05-28 12:32 ` Robert P. J. Day 2009-05-28 13:12 ` Petr Tesarik 1 sibling, 1 reply; 17+ messages in thread From: Robert P. J. Day @ 2009-05-28 12:32 UTC (permalink / raw) To: Petr Tesarik; +Cc: Linux Kernel Mailing List, Andrew Morton [-- Attachment #1: Type: TEXT/PLAIN, Size: 1636 bytes --] On Thu, 28 May 2009, Petr Tesarik wrote: > Robert P. J. Day píše v Čt 23. 04. 2009 v 13:43 -0400: > > A boolean single_bit_set() routine would simplify the numerous > > constructs of the form (((n & (n - 1)) == 0)) when testing for > > single-bitness. > > > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > > > --- > > > > This is similar to the current is_power_of_2() routine defined in > > include/linux/log2.h, which is mathematically identical but, > > semantically, should be defined independently just so the code is more > > readable. > > > > I'm open to an alternative function name. > > ispow2() ? > > Because what it really does is to check that a value is a power of two, > doesn'it. by the way, a search for places in the code that are candidates for this kind of rewriting can be seen at one of my wiki kernel cleanup pages: http://www.crashcourse.ca/wiki/index.php/The_style_script scroll down, you'll see a section entitled "Testing for power of 2 or a single bit set". lots of potential for clarification if people think it's worth it. rday -- ======================================================================== Robert P. J. Day Waterloo, Ontario, CANADA Linux Consulting, Training and Annoying Kernel Pedantry. Web page: http://crashcourse.ca Linked In: http://www.linkedin.com/in/rpjday Twitter: http://twitter.com/rpjday ======================================================================== ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-05-28 12:32 ` Robert P. J. Day @ 2009-05-28 13:12 ` Petr Tesarik 2009-06-29 18:50 ` H. Peter Anvin 0 siblings, 1 reply; 17+ messages in thread From: Petr Tesarik @ 2009-05-28 13:12 UTC (permalink / raw) To: Robert P. J. Day; +Cc: Linux Kernel Mailing List, Andrew Morton Robert P. J. Day píše v Čt 28. 05. 2009 v 08:32 -0400: > On Thu, 28 May 2009, Petr Tesarik wrote: > > > Robert P. J. Day píše v Čt 23. 04. 2009 v 13:43 -0400: > > > A boolean single_bit_set() routine would simplify the numerous > > > constructs of the form (((n & (n - 1)) == 0)) when testing for > > > single-bitness. > > > > > > Signed-off-by: Robert P. J. Day <rpjday@crashcourse.ca> > > > > > > --- > > > > > > This is similar to the current is_power_of_2() routine defined in > > > include/linux/log2.h, which is mathematically identical but, > > > semantically, should be defined independently just so the code is more > > > readable. > > > > > > I'm open to an alternative function name. > > > > ispow2() ? > > > > Because what it really does is to check that a value is a power of two, > > doesn'it. > > by the way, a search for places in the code that are candidates for > this kind of rewriting can be seen at one of my wiki kernel cleanup > pages: > > http://www.crashcourse.ca/wiki/index.php/The_style_script Ah, yes, sorry, I missed the top of your email. Ok, then my only concern is that the hweight* functions return the exact weight, which might be much less efficient if all we need is to know whether it's 1. Theoretically, gcc should be able to optimize things out, but I'm not all that optimistic about how well it does it. Petr Tesarik ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Introduce a boolean "single_bit_set" function. 2009-05-28 13:12 ` Petr Tesarik @ 2009-06-29 18:50 ` H. Peter Anvin 0 siblings, 0 replies; 17+ messages in thread From: H. Peter Anvin @ 2009-06-29 18:50 UTC (permalink / raw) To: Petr Tesarik; +Cc: Robert P. J. Day, Linux Kernel Mailing List, Andrew Morton Petr Tesarik wrote: > > Ok, then my only concern is that the hweight* functions return the exact > weight, which might be much less efficient if all we need is to know > whether it's 1. > > Theoretically, gcc should be able to optimize things out, but I'm not > all that optimistic about how well it does it. > I can almost guarantee it won't in this case. -hpa ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2009-06-30 10:20 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-04-23 17:43 [PATCH] Introduce a boolean "single_bit_set" function Robert P. J. Day 2009-04-23 19:57 ` David Daney 2009-04-23 20:11 ` Robert P. J. Day 2009-04-23 23:57 ` Andrew Morton 2009-04-24 10:40 ` Robert P. J. Day 2009-04-24 17:46 ` Andrew Morton 2009-04-25 22:09 ` Robert P. J. Day 2009-06-29 18:15 ` Petr Tesarik 2009-06-29 18:50 ` Robert P. J. Day 2009-06-30 6:12 ` Petr Tesarik 2009-06-30 10:18 ` Robert P. J. Day 2009-04-24 13:51 ` Robert P. J. Day 2009-05-28 12:21 ` Petr Tesarik 2009-05-28 12:27 ` Robert P. J. Day 2009-05-28 12:32 ` Robert P. J. Day 2009-05-28 13:12 ` Petr Tesarik 2009-06-29 18:50 ` H. Peter Anvin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox