On Fri, 14 Nov 2008 13:46:42 -0800 (PST)
Trent Piepho <tpiepho at 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.
