Messages in this thread | | | From | Rasmus Villemoes <> | Subject | Re: [PATCH v3 1/3] lib: find_*_bit reimplementation | Date | Wed, 11 Feb 2015 23:14:07 +0100 |
| |
[for some reason google decided to put this in my spam folder, hrmpf]
On Mon, Feb 09 2015, "George Spelvin" <linux@horizon.com> wrote:
> Sorry, I screwed up the bit-twiddling while messing with various options. > I was trying to get size == 32 to work; that should have been: > >> tmp &= (2UL << ((size-1) % BITS_PER_LONG)) - 1; /* Mask last word */ > > And you're right that LAST_WORD_MASK is a good wrapper. >
Well, it's not my invention, I just misremembered the name. linux/bitmap.h already exposes BITMAP_LAST_WORD_MASK.
> Vasrious working solutions include: > #define LAST_WORD_MASK(bits) ((2UL << (bits-1) % BITS_PER_LONG) - 1) > #define LAST_WORD_MASK(bits) ~(~0UL << bits % BITS_PER_LONG) > #define LAST_WORD_MASK(bits) (~0UL >> -bits % BITS_PER_LONG)
Incidentally, I had a patch lying around for replacing BITMAP_LAST_WORD_MASK by something like the last of these (it is currently using a ?:). But to allow bits to have signed type it is safer to spell it
#define BITMAP_LAST_WORD_MASK(bits) (~0UL >> ((-(bits)) & (BITS_PER_LONG-1)))
[also adding lots of parentheses so I don't have to worry about precedence].
> I'm not sure which generates the nicest code. It's 4 instructions > each way, with the last being 1 byte smaller:
I think one would have to look at effects on real code; when just compiling a function doing nothing but this gcc has to use specific registers for in and out.
>> Also, I think it is best to handle size==0 appropriately, meaning that >> one cannot dereference addr in any way (and certainly not addr[-1]). > > Ah, okay; l I figured that was a safe case to omit. But your solution is nicer > than mine overall. > > It may be that omitting the mask *is* safe, but it's a lot of wading through > callers to prove it.
I think generic library code like this should provide both safety checks, and only if some true performance bottleneck is found can one start looking at implementing __shortcuts which have further constraints on the caller.
Rasmus
| |