[PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding

[PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division with rounding

[Darrick J. Wong]

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 <djwong@us.ibm.com>
---

 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; })

[PATCH 2/2] adt74{62, 70, 73}: Use DIV_ROUND_CLOSEST for rounded division

[Darrick J. Wong]

Modify some hwmon drivers to use DIV_ROUND_CLOSEST instead of bloating
source with (naughty) macros.

Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
---

 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);

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

[Jean Delvare]

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 <djwong@us.ibm.com>
> ---
> 
>  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

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

[Andrew Morton]

On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> 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 <djwong@us.ibm.com>
> > ---
> > 
> >  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.

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

[Jean Delvare]

On Tue, 11 Nov 2008 09:07:02 -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> 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 <djwong@us.ibm.com>
> > > ---
> > > 
> > >  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

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

[Joe Perches]

On Tue, 2008-11-11 at 09:07 -0800, Andrew Morton wrote:
> On Tue, 11 Nov 2008 11:04:54 +0100 Jean Delvare <khali@linux-fr.org> 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

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

[Trent Piepho]

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.

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

[Andrew Morton]

On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
Trent Piepho <tpiepho@freescale.com> 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?

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

[Trent Piepho]

On Tue, 11 Nov 2008, Andrew Morton wrote:
> On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
> Trent Piepho <tpiepho@freescale.com> 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

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

[Andrew Morton]

On Tue, 11 Nov 2008 15:42:09 -0800 (PST)
Trent Piepho <tpiepho@freescale.com> wrote:

> On Tue, 11 Nov 2008, Andrew Morton wrote:
> > On Tue, 11 Nov 2008 15:05:02 -0800 (PST)
> > Trent Piepho <tpiepho@freescale.com> 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.

Re: [lm-sensors] [PATCH 1/2] Create a DIV_ROUND_CLOSEST macro to do division 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

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

[Andrew Morton]

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.

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

[Jochen Voß]

Hi,

2008/11/11 Andrew Morton <akpm@linux-foundation.org>:
> 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/

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

[Peter Zijlstra]

On Mon, 2008-11-10 at 17:01 -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.

Its too late to rename this now isn't it :-/

DIV_ROUND_{UP,CEIL}
DIV_ROUND_{DOWN,FLOOR}

I get.

The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
is just wonky.

Also, shouldn't it be something like ?

  DIV_ROUND(x, y) (((x) + (((y)+1)/2)) / (y))

Also, do we want the default rounding to be symmetric or asymmetric?

I think round to half even or something would be nice, but would add an
extra conditional, not sure its worth it (round away from zero only
works for signed numbers and it would also cost one conditional).

> Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> ---
> 
>  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; })

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

[Jean Delvare]

Hi Peter,

On Mon, 03 Aug 2009 13:57:52 +0200, Peter Zijlstra wrote:
> On Mon, 2008-11-10 at 17:01 -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.
> 
> Its too late to rename this now isn't it :-/

We can always rename things, but it is costly, so we only do that if we
have a good reason (e.g. the original name caused confusion.)

> 
> DIV_ROUND_{UP,CEIL}
> DIV_ROUND_{DOWN,FLOOR}

The latter is the simple integer division, so there is no point in
defining a macro for it. The former we already have.

> 
> I get.
> 
> The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
> is just wonky.

I can't disagree. I even think I argued about it back then, but then
finally gave up. You should have participated in the debate when it was
hot rather than 8 months later :(

> 
> Also, shouldn't it be something like ?
> 
>   DIV_ROUND(x, y) (((x) + (((y)+1)/2)) / (y))

No, that wouldn't work. Simple example, 10/3 is 3.33. (10+3/2)/3 is 3
which is the correct rounded value. (10+(3+1)/2)/3 is 4 which is not
the correct rounded value.

> Also, do we want the default rounding to be symmetric or asymmetric?

I don't think we care at all. This macro will rarely be called on
negative numbers, and even then...

> 
> I think round to half even or something would be nice, but would add an
> extra conditional, not sure its worth it (round away from zero only
> works for signed numbers and it would also cost one conditional).

Definitely not worth the extra cost. We do the rounding because it is
not too expensive and adds value. Fine-tuning it will cost more than is
worth, plus would lead to endless debates about what is the best
rounding strategy.

If anyone really needs additional control on rounding, they better do
not rely on a general helper, but instead implement their own rounding
function.

-- 
Jean Delvare

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

[Peter Zijlstra]

On Mon, 2009-08-03 at 14:21 +0200, Jean Delvare wrote:
> > The proposed thing is simply DIV_ROUND, but this DIV_ROUND_CLOSEST name
> > is just wonky.
> 
> I can't disagree. I even think I argued about it back then, but then
> finally gave up. You should have participated in the debate when it was
> hot rather than 8 months later :(

Yeah, sometimes its hard to keep up with all on lkml, I only noticed it
because I ran into some code that used it.

Agreed on the other points.