Re: [RFC][PATCH 2/3] math128: Introduce {mult,add,cmp}_u128

[Måns Rullgård]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> writes:

> On Tue, Apr 24, 2012 at 9:10 AM, Peter Zijlstra <a.p.zijlstra@xxxxxxxxx> wrote:
>> Grow rudimentary u128 support without relying on gcc/libgcc.
>>
>> +#ifndef add_u128
>> +static inline u128 add_u128(u128 a, u128 b)
>> +{
>> +       u128 res;
>> +
>> +       res.hi = a.hi + b.hi;
>> +       res.lo = a.lo + b.lo;
>> +
>> +       if (res.lo < a.lo || res.lo < b.lo)
>> +               res.hi++;
>
> This is wrong. Or at least stupid.
>
> Just do one of the comparisons, not both. If overflow occurs, the
> result will be smaller than *either* of the added numbers, so
> comparing both is just silly and confused.
>
> So just pick one.
>
> Also, it might be worth looking at code generation, to see if it's
> better to just do
>
>    a.hi += b.hi;
>    a.low += b.low;
>    if (a.low < b.low)
>       a.hi++;
>    return a;

I have no idea if it makes a difference, but that if statement can be
written as a.hi += a.low < b.low.  Just an observation.

> because that might make it clear that there are fewer actual values
> live at any particular time. But gcc may not care. Try it.
>
> Also, for the multiply, please make sure gcc knows to do a "32x32->64"
> multiplication, rather than thinking it needs to do full 64x64
> multiplies..

On ARM it does the right thing at least since 4.3, which is the oldest
ARM compiler I have at hand.

-- 
Måns Rullgård
mans@xxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/