[v3,2/2] bitops: fix types

bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.

Unify to 'unsigned long' because it generates better code on x86_64.
Adjust asserts accordingly.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 bitops.h |   40 +++++++++++++++++++++-------------------
 1 files changed, 21 insertions(+), 19 deletions(-)

On 07/14/2012 03:34 PM, Blue Swirl wrote:
> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
> 
> Unify to 'unsigned long' because it generates better code on x86_64.
> Adjust asserts accordingly.
> 


Actually, plain unsigned generates the best code.  unsigned longs
require an extra byte for many instructions.  It also requires more
stack space if spilled.  Unsigned does not, but is compatible with
unsigned long in case it needs to be used in pointer arithmetic (unlike
signed int which needs a sign extension instruction).

On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>
> Unify to 'unsigned long' because it generates better code on x86_64.
> Adjust asserts accordingly.

Still disagree with this patch, for the record.

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>>
>> Unify to 'unsigned long' because it generates better code on x86_64.
>> Adjust asserts accordingly.
>
> Still disagree with this patch, for the record.

So do I.

Changing tried-and-true code for some unproven performance gain is a bad
idea.

In this particular case, it additionally deviates from the code's
source.

If you feel your patch is a worthwhile improvement, please take it to
LKML, so the kernel and future borrowers of this code can profit.
Copying free code without at least trying to contribute improvements
back to the source isn't proper.

On Tue, Jul 17, 2012 at 12:36 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 14 July 2012 13:34, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> bitops.h uses inconsistently 'unsigned long' and 'int' for bit numbers.
>>>
>>> Unify to 'unsigned long' because it generates better code on x86_64.
>>> Adjust asserts accordingly.
>>
>> Still disagree with this patch, for the record.
>
> So do I.
>
> Changing tried-and-true code for some unproven performance gain is a bad
> idea.

No it isn't and that is not the case.

>
> In this particular case, it additionally deviates from the code's
> source.

I'm getting a strong feeling that it's a bad idea to reuse any Linux
kernel sources since they are seen as divine and untouchable, unlike
for example BSD queue macros. Perhaps BSD have also bit field ops
which we could use instead? There's also the licensing problem with
GPLv2only in some cases.

>
> If you feel your patch is a worthwhile improvement, please take it to
> LKML, so the kernel and future borrowers of this code can profit.
> Copying free code without at least trying to contribute improvements
> back to the source isn't proper.

The original kernel functions use 'unsigned long'. The bit field
functions introduced by Peter use 'int' but those are not in the
kernel tree, so there's nothing to fix (except the two first hunks)
from the kernel point of view. Also 'volatile' makes sense when kernel
is banging HW registers.

On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
> I'm getting a strong feeling that it's a bad idea to reuse any Linux
> kernel sources since they are seen as divine and untouchable, unlike
> for example BSD queue macros.

We should also try to avoid deviations in our queue macros,
and I think we do (eg commit 6095aa8 added functionality by
moving us closer into sync with the BSD macros rather than
by reinventing the wheel which was IIRC what the initial pre-code-review
patch did).

-- PMM

Peter Maydell <peter.maydell@linaro.org> writes:

> On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
>> I'm getting a strong feeling that it's a bad idea to reuse any Linux
>> kernel sources since they are seen as divine and untouchable, unlike
>> for example BSD queue macros.

Reusing good code that solves the problem at hand can be a bad idea if
you can't resist the temptation to tinker with it, yet can't be bothered
to upstream your improvements.  Then you might as well build your own
bikeshed from scratch :)

> We should also try to avoid deviations in our queue macros,

Agree.

> and I think we do (eg commit 6095aa8 added functionality by
> moving us closer into sync with the BSD macros rather than
> by reinventing the wheel which was IIRC what the initial pre-code-review
> patch did).

On Tue, Jul 24, 2012 at 9:26 AM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>
>> On 23 July 2012 18:33, Blue Swirl <blauwirbel@gmail.com> wrote:
>>> I'm getting a strong feeling that it's a bad idea to reuse any Linux
>>> kernel sources since they are seen as divine and untouchable, unlike
>>> for example BSD queue macros.
>
> Reusing good code that solves the problem at hand can be a bad idea if
> you can't resist the temptation to tinker with it, yet can't be bothered
> to upstream your improvements.  Then you might as well build your own
> bikeshed from scratch :)

There's nothing wrong in tinkering with reused good code. As I
explained, there's little point to upstream these changes, so 'not
bothering' is false accusation.

>
>> We should also try to avoid deviations in our queue macros,
>
> Agree.

Avoiding "deviations" can be secondary to many other needs.

>
>> and I think we do (eg commit 6095aa8 added functionality by
>> moving us closer into sync with the BSD macros rather than
>> by reinventing the wheel which was IIRC what the initial pre-code-review
>> patch did).