Create a helper macro to divide two numbers and round the result to the
nearest whole number. This is a helper macro for hwmon drivers that
want to convert incoming sysfs values per standard hwmon practice, though
the macro itself can be used by anyone.
Signed-off-by: Darrick J. Wong <[email protected]>
---
include/linux/kernel.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index fba141d..fb02266 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
#define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
#define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
#define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
+#define DIV_ROUND_CLOSEST(x, divisor)( \
+{ \
+ typeof(divisor) __divisor = divisor; \
+ (((x) + ((__divisor) / 2)) / (__divisor)); \
+} \
+)
#define _RET_IP_ (unsigned long)__builtin_return_address(0)
#define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })
Modify some hwmon drivers to use DIV_ROUND_CLOSEST instead of bloating
source with (naughty) macros.
Signed-off-by: Darrick J. Wong <[email protected]>
---
drivers/hwmon/adt7462.c | 14 ++++++--------
drivers/hwmon/adt7470.c | 8 +++-----
drivers/hwmon/adt7473.c | 10 ++++------
3 files changed, 13 insertions(+), 19 deletions(-)
diff --git a/drivers/hwmon/adt7462.c b/drivers/hwmon/adt7462.c
index 66107b4..1852f27 100644
--- a/drivers/hwmon/adt7462.c
+++ b/drivers/hwmon/adt7462.c
@@ -204,8 +204,6 @@ I2C_CLIENT_INSMOD_1(adt7462);
#define MASK_AND_SHIFT(value, prefix) \
(((value) & prefix##_MASK) >> prefix##_SHIFT)
-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7462_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -840,7 +838,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp) || !temp_enabled(data, attr->index))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -878,7 +876,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp) || !temp_enabled(data, attr->index))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -943,7 +941,7 @@ static ssize_t set_volt_max(struct device *dev,
return -EINVAL;
temp *= 1000; /* convert mV to uV */
- temp = ROUND_DIV(temp, x);
+ temp = DIV_ROUND_CLOSEST(temp, x);
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -985,7 +983,7 @@ static ssize_t set_volt_min(struct device *dev,
return -EINVAL;
temp *= 1000; /* convert mV to uV */
- temp = ROUND_DIV(temp, x);
+ temp = DIV_ROUND_CLOSEST(temp, x);
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -1250,7 +1248,7 @@ static ssize_t set_pwm_hyst(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 15);
/* package things up */
@@ -1337,7 +1335,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000) + 64;
+ temp = DIV_ROUND_CLOSEST(temp, 1000) + 64;
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c
index 1311a59..da6c930 100644
--- a/drivers/hwmon/adt7470.c
+++ b/drivers/hwmon/adt7470.c
@@ -137,8 +137,6 @@ I2C_CLIENT_INSMOD_1(adt7470);
#define FAN_PERIOD_INVALID 65535
#define FAN_DATA_VALID(x) ((x) && (x) != FAN_PERIOD_INVALID)
-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7470_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -360,7 +358,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -394,7 +392,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
@@ -671,7 +669,7 @@ static ssize_t set_pwm_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = SENSORS_LIMIT(temp, 0, 255);
mutex_lock(&data->lock);
diff --git a/drivers/hwmon/adt7473.c b/drivers/hwmon/adt7473.c
index 18aa308..0a6ce23 100644
--- a/drivers/hwmon/adt7473.c
+++ b/drivers/hwmon/adt7473.c
@@ -129,8 +129,6 @@ I2C_CLIENT_INSMOD_1(adt7473);
#define FAN_PERIOD_INVALID 65535
#define FAN_DATA_VALID(x) ((x) && (x) != FAN_PERIOD_INVALID)
-#define ROUND_DIV(x, divisor) (((x) + ((divisor) / 2)) / (divisor))
-
struct adt7473_data {
struct device *hwmon_dev;
struct attribute_group attrs;
@@ -459,7 +457,7 @@ static ssize_t set_temp_min(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);
mutex_lock(&data->lock);
@@ -495,7 +493,7 @@ static ssize_t set_temp_max(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);
mutex_lock(&data->lock);
@@ -720,7 +718,7 @@ static ssize_t set_temp_tmax(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);
mutex_lock(&data->lock);
@@ -756,7 +754,7 @@ static ssize_t set_temp_tmin(struct device *dev,
if (strict_strtol(buf, 10, &temp))
return -EINVAL;
- temp = ROUND_DIV(temp, 1000);
+ temp = DIV_ROUND_CLOSEST(temp, 1000);
temp = encode_temp(data->temp_twos_complement, temp);
mutex_lock(&data->lock);
Hi Darrick,
On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote:
>
> Create a helper macro to divide two numbers and round the result to the
> nearest whole number. This is a helper macro for hwmon drivers that
> want to convert incoming sysfs values per standard hwmon practice, though
> the macro itself can be used by anyone.
>
> Signed-off-by: Darrick J. Wong <[email protected]>
> ---
>
> include/linux/kernel.h | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index fba141d..fb02266 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> +#define DIV_ROUND_CLOSEST(x, divisor)( \
> +{ \
> + typeof(divisor) __divisor = divisor; \
> + (((x) + ((__divisor) / 2)) / (__divisor)); \
> +} \
> +)
>
> #define _RET_IP_ (unsigned long)__builtin_return_address(0)
> #define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })
>
I don't get why you implement this as a macro rather than an inline
function? A function would look much better.
--
Jean Delvare
On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <[email protected]> wrote:
> Hi Darrick,
>
> On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote:
> >
> > Create a helper macro to divide two numbers and round the result to the
> > nearest whole number. This is a helper macro for hwmon drivers that
> > want to convert incoming sysfs values per standard hwmon practice, though
> > the macro itself can be used by anyone.
> >
> > Signed-off-by: Darrick J. Wong <[email protected]>
> > ---
> >
> > include/linux/kernel.h | 6 ++++++
> > 1 files changed, 6 insertions(+), 0 deletions(-)
> >
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index fba141d..fb02266 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> > #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> > #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> > #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > +#define DIV_ROUND_CLOSEST(x, divisor)( \
> > +{ \
> > + typeof(divisor) __divisor = divisor; \
> > + (((x) + ((__divisor) / 2)) / (__divisor)); \
> > +} \
> > +)
> >
> > #define _RET_IP_ (unsigned long)__builtin_return_address(0)
> > #define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })
> >
>
> I don't get why you implement this as a macro rather than an inline
> function? A function would look much better.
The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
size (char, short, ... long long) and will do all the suitable
promotion and will return a type of the appropriate width and
signedness.
It's not particularly pretty and can hide traps and pitfalls, but the
other way is tricky as well - it'd need a family of functions and
there's a risk that programmers will choose the wrong one.
On Tue, 11 Nov 2008 09:07:02 -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <[email protected]> wrote:
>
> > Hi Darrick,
> >
> > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote:
> > >
> > > Create a helper macro to divide two numbers and round the result to the
> > > nearest whole number. This is a helper macro for hwmon drivers that
> > > want to convert incoming sysfs values per standard hwmon practice, though
> > > the macro itself can be used by anyone.
> > >
> > > Signed-off-by: Darrick J. Wong <[email protected]>
> > > ---
> > >
> > > include/linux/kernel.h | 6 ++++++
> > > 1 files changed, 6 insertions(+), 0 deletions(-)
> > >
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index fba141d..fb02266 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> > > #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> > > #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> > > #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > > +#define DIV_ROUND_CLOSEST(x, divisor)( \
> > > +{ \
> > > + typeof(divisor) __divisor = divisor; \
> > > + (((x) + ((__divisor) / 2)) / (__divisor)); \
> > > +} \
> > > +)
> > >
> > > #define _RET_IP_ (unsigned long)__builtin_return_address(0)
> > > #define _THIS_IP_ ({ __label__ __here; __here: (unsigned long)&&__here; })
> > >
> >
> > I don't get why you implement this as a macro rather than an inline
> > function? A function would look much better.
>
> The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
> size (char, short, ... long long) and will do all the suitable
> promotion and will return a type of the appropriate width and
> signedness.
>
> It's not particularly pretty and can hide traps and pitfalls, but the
> other way is tricky as well - it'd need a family of functions and
> there's a risk that programmers will choose the wrong one.
OK, I get it now, thanks for the clarification.
--
Jean Delvare
On Tue, 2008-11-11 at 09:07 -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <[email protected]> wrote:
> > On Mon, 10 Nov 2008 17:01:32 -0800, Darrick J. Wong wrote:
> > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > > index fba141d..fb02266 100644
> > > --- a/include/linux/kernel.h
> > > +++ b/include/linux/kernel.h
> > > @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> > > #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> > > #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> > > #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > > +#define DIV_ROUND_CLOSEST(x, divisor)( \
> > > +{ \
> > > + typeof(divisor) __divisor = divisor; \
> > > + (((x) + ((__divisor) / 2)) / (__divisor)); \
> > > +} \
> > > +)
> > I don't get why you implement this as a macro rather than an inline
> > function? A function would look much better.
> The idea is that DIV_ROUND_CLOSEST() can be used with arguments of any
> size (char, short, ... long long) and will do all the suitable
> promotion and will return a type of the appropriate width and
> signedness.
Perhaps the macro should be placed directly after
DIV_ROUND_UP and should use the same argument naming.
Perhaps HALF_UP is more descriptive and fairly common.
http://java.sun.com/j2se/1.5.0/docs/api/java/math/RoundingMode.html
On Mon, 10 Nov 2008, Darrick J. Wong wrote:
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index fba141d..fb02266 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> +#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?
Note that if divisor is a signed variable, divisor/2 generates worse code
than divisor>>1.
On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
Trent Piepho <[email protected]> wrote:
> On Mon, 10 Nov 2008, Darrick J. Wong wrote:
> > diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> > index fba141d..fb02266 100644
> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -48,6 +48,12 @@ extern const char linux_proc_banner[];
> > #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> > #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> > #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> > +#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.
> Note that if divisor is a signed variable, divisor/2 generates worse code
> than divisor>>1.
yup. I wonder why the compiler doesn't do that for itself - is there a
case where it will generate a different result?
On Tue, 11 Nov 2008, Andrew Morton wrote:
> On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
> Trent Piepho <[email protected]> wrote:
>> On Mon, 10 Nov 2008, Darrick J. Wong wrote:
>>> #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
>>> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
>>> #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
>>> +#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?
>> Note that if divisor is a signed variable, divisor/2 generates worse code
>> than divisor>>1.
>
> yup. I wonder why the compiler doesn't do that for itself - is there a
> case where it will generate a different result?
main()
{
int x = -5;
printf("%d %d\n", x>>1, x/2);
}
$ a.out
-3 -2
Hi,
2008/11/11 Andrew Morton <[email protected]>:
> yup. I wonder why the compiler doesn't do that for itself - is there a
> case where it will generate a different result?
The test program
#include <stdio.h>
int
main()
{
signed int x = -1;
printf("%d %d\n", x/2, x>>1);
return 0;
}
says
0 -1
so it seems to make a difference.
All the best,
Jochen
--
http://seehuhn.de/
On Tue, 11 Nov 2008 15:42:09 -0800 (PST)
Trent Piepho <[email protected]> wrote:
> On Tue, 11 Nov 2008, Andrew Morton wrote:
> > On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
> > Trent Piepho <[email protected]> wrote:
> >> On Mon, 10 Nov 2008, Darrick J. Wong wrote:
> >>> #define FIELD_SIZEOF(t, f) (sizeof(((t*)0)->f))
> >>> #define DIV_ROUND_UP(n,d) (((n) + (d) - 1) / (d))
> >>> #define roundup(x, y) ((((x) + ((y) - 1)) / (y)) * (y))
> >>> +#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.
> >> Note that if divisor is a signed variable, divisor/2 generates worse code
> >> than divisor>>1.
> >
> > yup. I wonder why the compiler doesn't do that for itself - is there a
> > case where it will generate a different result?
>
> main()
> {
> int x = -5;
> printf("%d %d\n", x>>1, x/2);
> }
> $ a.out
> -3 -2
ah, thanks.
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
On Fri, 14 Nov 2008 13:46:42 -0800 (PST)
Trent Piepho <[email protected]> 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.