Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to dodivision with rounding

[Trent Piepho]

On Tue, 11 Nov 2008, Andrew Morton wrote:
>>>>> +#define DIV_ROUND_CLOSEST(x, divisor)(			\
>>>>> +{							\
>>>>> +	typeof(divisor) __divisor = divisor;		\
>>>>> +	(((x) + ((__divisor) / 2)) / (__divisor));	\
>>>>> +}							\
>>>>> +)
>>>>
>>>> Maybe you can do away with the statement-expression extension?  I've seen
>>>> cases where it cases gcc to generate worse code.  It seems like it
>>>> shouldn't, but it does.  I know DIV_ROUND_CLOSEST (maybe DIV_ROUND_NEAR?)
>>>> uses divisor twice, but all the also divide macros do that too, so why does
>>>> this one need to be different?
>>>
>>> The others need fixing too.
>>
>> Is it worth generating worse code for these simple macros?
>
> Well that's an interesting question.
>
> The risks with the current code are
>
> a) It will introduce straightforward bugs, where pointers are
>   incremented twice, etc.
>
>   Hopefully these things will be apparent during testing and we'll
>   fix them up in the usual fashion.
>
> b) It will introduce subtle slowdowns due to needlessly executing
>   code more than once, as in the hugepage case which I identified.
>   These problems will hang around for long periods.
>
> So they're good reasons to fix the macros.  If these fixes cause the
> compiler to generate worse code then we should quantify and understand
> that.  Perhaps it is only certain compiler versions.  Perhaps we can
> find a test case (should be easy?) and send it over to the gcc guys to
> fix.  Perhaps we can find some C-level construct which prevents the
> compiler from going into stupid mode without reintroducing the existing
> problems.

My question was more along the lines of is it worth it to even have macros for
something as simple rounding up when dividing?

For an example of statement expression problems, I noticed something with
swab16(), addressed in commit 8e2c20023f34b652605a5fb7c68bb843d2b100a8

#define ___swab16(x) \
({ \
        __u16 __x = (x); \
        ((__u16)( \
                (((__u16)(__x) & (__u16)0x00ffU) << 8) | \
                (((__u16)(__x) & (__u16)0xff00U) >> 8) )); \
})

Produces this code:

             movzwl  %ax, %eax
             movl    %eax, %edx
             shrl    $8, %eax
             sall    $8, %edx
             orl     %eax, %edx

While this:

static __inline__ __attribute_const__ __u16 ___swab16(__u16 x)
{
        return x<<8 | x>>8;
}

Produces this code:

             rolw    $8, %ax

--
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/