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

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 14 Nov 2008 13:46:42 -0800 (PST)
Trent Piepho <tpiepho@freescale.com> wrote:

> 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

stupid gcc.

I wonder if we could do something along these lines:

static inline u8 __div_round_up_u8(u8 n, u8 d)
{
	...
}

static inline u16 __div_round_up_u16(u16 n, u16 d)
{
	...
}

<etc>

#define DIV_ROUND_UP(n, d)
	(sizeof(n) == 8 ? __div_round_up_u8(n, d) :
	    (sizeof(n) == 16 ? __div_round_up_u16(n, d) :
		(sizeof(n) == 32 ? __div_round_up_u32(n, d) :
		 	(sizeof(n) == 64 ? __div_round_up_u64(n, d) :
				__panic_i_am_confused()))))

which might work but is arguably too stupid to live.  And whcih still cannot
be used for compile-time array-sizing.