[PATCH] remove abs64()

[PATCH] remove abs64()

[Alexey Dobriyan]

We don't need no stinking abs64() given some GCC extensions
(especially __builtin_choose_expr()).

One abs() implementation is better than two abs() implementations.

New abs() doesn't expand type needlessly.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

 drivers/gpu/drm/drm_irq.c |    4 ++--
 include/linux/kernel.h    |   43 +++++++++++++++++++++----------------------
 lib/div64.c               |    2 +-
 3 files changed, 24 insertions(+), 25 deletions(-)

--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -154,7 +154,7 @@ static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 	 * available. In that case we can't account for this and just
 	 * hope for the best.
 	 */
-	if ((vblrc > 0) && (abs64(diff_ns) > 1000000)) {
+	if ((vblrc > 0) && (abs(diff_ns) > 1000000)) {
 		atomic_inc(&dev->_vblank_count[crtc]);
 		smp_mb__after_atomic_inc();
 	}
@@ -1290,7 +1290,7 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
 	 * e.g., due to spurious vblank interrupts. We need to
 	 * ignore those for accounting.
 	 */
-	if (abs64(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS) {
+	if (abs(diff_ns) > DRM_REDUNDANT_VBLIRQ_THRESH_NS) {
 		/* Store new timestamp in ringbuffer. */
 		vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
 
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -143,28 +143,27 @@ extern int _cond_resched(void);
 
 #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
 
-/*
- * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
- * input types abs() returns a signed long.
- * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
- * for those.
- */
-#define abs(x) ({						\
-		long ret;					\
-		if (sizeof(x) == sizeof(long)) {		\
-			long __x = (x);				\
-			ret = (__x < 0) ? -__x : __x;		\
-		} else {					\
-			int __x = (x);				\
-			ret = (__x < 0) ? -__x : __x;		\
-		}						\
-		ret;						\
-	})
-
-#define abs64(x) ({				\
-		s64 __x = (x);			\
-		(__x < 0) ? -__x : __x;		\
-	})
+#define abs(x)								\
+({									\
+	typeof(x) _x = (x);						\
+									\
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), signed char),	\
+		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), short),	\
+		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), int),		\
+		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), long),		\
+		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), long long),	\
+		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
+	_x)))));							\
+})
 
 #ifdef CONFIG_PROVE_LOCKING
 void might_fault(void);
--- a/lib/div64.c
+++ b/lib/div64.c
@@ -121,7 +121,7 @@ s64 div64_s64(s64 dividend, s64 divisor)
 {
 	s64 quot, t;
 
-	quot = div64_u64(abs64(dividend), abs64(divisor));
+	quot = div64_u64(abs(dividend), abs(divisor));
 	t = (dividend ^ divisor) >> 63;
 
 	return (quot ^ t) - t;

Re: [PATCH] remove abs64()

[Randy Dunlap]

On Wed, 13 Apr 2011 00:00:45 +0300 Alexey Dobriyan wrote:

> We don't need no stinking abs64() given some GCC extensions
> (especially __builtin_choose_expr()).
> 
> One abs() implementation is better than two abs() implementations.

questionable.

> New abs() doesn't expand type needlessly.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  drivers/gpu/drm/drm_irq.c |    4 ++--
>  include/linux/kernel.h    |   43 +++++++++++++++++++++----------------------
>  lib/div64.c               |    2 +-
>  3 files changed, 24 insertions(+), 25 deletions(-)


> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -143,28 +143,27 @@ extern int _cond_resched(void);
>  
>  #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
>  
> -/*
> - * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
> - * input types abs() returns a signed long.
> - * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
> - * for those.
> - */
> -#define abs(x) ({						\
> -		long ret;					\
> -		if (sizeof(x) == sizeof(long)) {		\
> -			long __x = (x);				\
> -			ret = (__x < 0) ? -__x : __x;		\
> -		} else {					\
> -			int __x = (x);				\
> -			ret = (__x < 0) ? -__x : __x;		\
> -		}						\
> -		ret;						\
> -	})
> -
> -#define abs64(x) ({				\
> -		s64 __x = (x);			\
> -		(__x < 0) ? -__x : __x;		\
> -	})
> +#define abs(x)								\
> +({									\
> +	typeof(x) _x = (x);						\
> +									\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), short),	\
> +		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), int),		\
> +		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), long),		\
> +		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), long long),	\
> +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> +	_x)))));							\
> +})

that is better?

---
~Randy

Re: [PATCH] remove abs64()

[Andrew Morton]

On Tue, 12 Apr 2011 14:07:26 -0700
Randy Dunlap <rdunlap@xenotime.net> wrote:

> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h
> > @@ -143,28 +143,27 @@ extern int _cond_resched(void);
> >  
> >  #define might_sleep_if(cond) do { if (cond) might_sleep(); } while (0)
> >  
> > -/*
> > - * abs() handles unsigned and signed longs, ints, shorts and chars.  For all
> > - * input types abs() returns a signed long.
> > - * abs() should not be used for 64-bit types (s64, u64, long long) - use abs64()
> > - * for those.
> > - */
> > -#define abs(x) ({						\
> > -		long ret;					\
> > -		if (sizeof(x) == sizeof(long)) {		\
> > -			long __x = (x);				\
> > -			ret = (__x < 0) ? -__x : __x;		\
> > -		} else {					\
> > -			int __x = (x);				\
> > -			ret = (__x < 0) ? -__x : __x;		\
> > -		}						\
> > -		ret;						\
> > -	})
> > -
> > -#define abs64(x) ({				\
> > -		s64 __x = (x);			\
> > -		(__x < 0) ? -__x : __x;		\
> > -	})
> > +#define abs(x)								\
> > +({									\
> > +	typeof(x) _x = (x);						\
> > +									\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> > +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), short),	\
> > +		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), int),		\
> > +		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), long),		\
> > +		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > +	_x)))));							\
> > +})
> 
> that is better?

I think so.

It's a bit concerning that it changes the return type of abs().

Re: [PATCH] remove abs64()

[Alexey Dobriyan]

On Tue, Apr 12, 2011 at 02:07:26PM -0700, Randy Dunlap wrote:
> On Wed, 13 Apr 2011 00:00:45 +0300 Alexey Dobriyan wrote:
> 
> > We don't need no stinking abs64() given some GCC extensions
> > (especially __builtin_choose_expr()).
> > 
> > One abs() implementation is better than two abs() implementations.
> 
> questionable.

Come on, what's so special in 64-bit types?

> > --- a/include/linux/kernel.h
> > +++ b/include/linux/kernel.h

> > -#define abs64(x) ({				\
> > -		s64 __x = (x);			\
> > -		(__x < 0) ? -__x : __x;		\
> > -	})
> > +#define abs(x)								\
> > +({									\
> > +	typeof(x) _x = (x);						\
> > +									\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> > +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\

> that is better?

Infinitely better.
sizeof(abs(x)) == sizeof(x) for one thing.

Implementation is ugly, but, hey, one can't do better in C.

Re: [PATCH] remove abs64()

[Alexey Dobriyan]

On Tue, Apr 12, 2011 at 02:10:40PM -0700, Andrew Morton wrote:
> On Tue, 12 Apr 2011 14:07:26 -0700
> Randy Dunlap <rdunlap@xenotime.net> wrote:
> 
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > > +	_x)))));							\
> > > +})
> > 
> > that is better?
> 
> I think so.
> 
> It's a bit concerning that it changes the return type of abs().

I haven't read every abs() user, but, yes, sizeof(abs()) silently
changing is the issue.

Re: [PATCH] remove abs64()

[Andrew Morton]

On Wed, 13 Apr 2011 00:16:58 +0300
Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Tue, Apr 12, 2011 at 02:10:40PM -0700, Andrew Morton wrote:
> > On Tue, 12 Apr 2011 14:07:26 -0700
> > Randy Dunlap <rdunlap@xenotime.net> wrote:
> > 
> > > > +	__builtin_choose_expr(						\
> > > > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > > > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > > > +	_x)))));							\
> > > > +})
> > > 
> > > that is better?
> > 
> > I think so.
> > 
> > It's a bit concerning that it changes the return type of abs().
> 
> I haven't read every abs() user, but, yes, sizeof(abs()) silently
> changing is the issue.

It changes signedness_of(abs(signed_expr)) as well.  That changes the
signedness of expressions which use abs() and on and on.

Re: [PATCH] remove abs64()

[Randy Dunlap]

On Tue, 12 Apr 2011 14:33:40 -0700 Andrew Morton wrote:

> On Wed, 13 Apr 2011 00:16:58 +0300
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
> 
> > On Tue, Apr 12, 2011 at 02:10:40PM -0700, Andrew Morton wrote:
> > > On Tue, 12 Apr 2011 14:07:26 -0700
> > > Randy Dunlap <rdunlap@xenotime.net> wrote:
> > > 
> > > > > +	__builtin_choose_expr(						\
> > > > > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > > > > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > > > > +	_x)))));							\
> > > > > +})
> > > > 
> > > > that is better?
> > > 
> > > I think so.
> > > 
> > > It's a bit concerning that it changes the return type of abs().
> > 
> > I haven't read every abs() user, but, yes, sizeof(abs()) silently
> > changing is the issue.
> 
> It changes signedness_of(abs(signed_expr)) as well.  That changes the
> signedness of expressions which use abs() and on and on.
> --

thanks for the further explanations.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] remove abs64()

[Oleg Nesterov]

On 04/13, Alexey Dobriyan wrote:
>
> +#define abs(x)								\
> +({									\
> +	typeof(x) _x = (x);						\
> +									\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), short),	\
> +		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), int),		\
> +		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), long),		\
> +		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
> +	__builtin_choose_expr(						\
> +		__builtin_types_compatible_p(typeof(_x), long long),	\
> +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> +	_x)))));							\
> +})

Personally I agree.

But, we have some stupid users which do something like abs(u32_value)
and expecting that abs() should treat this value as "signed".

Oleg.

Re: [PATCH] remove abs64()

[Andrew Morton]

On Wed, 13 Apr 2011 16:27:03 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> On 04/13, Alexey Dobriyan wrote:
> >
> > +#define abs(x)								\
> > +({									\
> > +	typeof(x) _x = (x);						\
> > +									\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> > +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), short),	\
> > +		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), int),		\
> > +		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), long),		\
> > +		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
> > +	__builtin_choose_expr(						\
> > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > +	_x)))));							\
> > +})
> 
> Personally I agree.
> 
> But, we have some stupid users which do something like abs(u32_value)
> and expecting that abs() should treat this value as "signed".
> 

um, yes, I'd forgotten that one.  That's a show-stopper.

Re: [PATCH] remove abs64()

[Oleg Nesterov]

On 04/13, Andrew Morton wrote:
>
> On Wed, 13 Apr 2011 16:27:03 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
>
> > On 04/13, Alexey Dobriyan wrote:
> > >
> > > +#define abs(x)								\
> > > +({									\
> > > +	typeof(x) _x = (x);						\
> > > +									\
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), signed char),	\
> > > +		(unsigned char)({ _x < 0 ? -_x : _x; }),		\
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), short),	\
> > > +		(unsigned short)({ _x < 0 ? -_x : _x; }),		\
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), int),		\
> > > +		(unsigned int)({ _x < 0 ? -_x : _x; }),			\
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), long),		\
> > > +		(unsigned long)({ _x < 0 ? -_x : _x; }),		\
> > > +	__builtin_choose_expr(						\
> > > +		__builtin_types_compatible_p(typeof(_x), long long),	\
> > > +		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
> > > +	_x)))));							\
> > > +})
> >
> > Personally I agree.
> >
> > But, we have some stupid users which do something like abs(u32_value)
> > and expecting that abs() should treat this value as "signed".
> >
>
> um, yes, I'd forgotten that one.  That's a show-stopper.

May be we can demand to fix them?

I agree with Alexey, it is a bit ugly to have abs() and abs64(), and abs()
itself doesn't look very nice.

What if we simply add

	BUILD_BUG_ON( (typeof(_x)-1) > 0 );

into abs()?

After that it would be trivial to find the offenders and fix them,

	- abs(unsigned_int)
	+ abs((int) unsigned_int)

Oleg.

Re: [PATCH] remove abs64()

[Andrew Morton]

On Wed, 13 Apr 2011 22:20:31 +0200
Oleg Nesterov <oleg@redhat.com> wrote:

> > > But, we have some stupid users which do something like abs(u32_value)
> > > and expecting that abs() should treat this value as "signed".
> > >
> >
> > um, yes, I'd forgotten that one.  That's a show-stopper.
> 
> May be we can demand to fix them?
> 
> I agree with Alexey, it is a bit ugly to have abs() and abs64(), and abs()
> itself doesn't look very nice.
> 
> What if we simply add
> 
> 	BUILD_BUG_ON( (typeof(_x)-1) > 0 );
> 
> into abs()?
> 
> After that it would be trivial to find the offenders and fix them,
> 
> 	- abs(unsigned_int)
> 	+ abs((int) unsigned_int)

Something like that.  But it should be done before we change abs(), to
avoid nasty breakage in obscure places.

Or we rework the abs() implementation so that the abs(unsigned)
behavior is unchanged.

There will remain the problem that the abs() return value's signedness
has changed.

Re: [PATCH] remove abs64()

[Alexey Dobriyan]

On Wed, Apr 13, 2011 at 01:26:29PM -0700, Andrew Morton wrote:
> On Wed, 13 Apr 2011 22:20:31 +0200
> Oleg Nesterov <oleg@redhat.com> wrote:
> 
> > > > But, we have some stupid users which do something like abs(u32_value)
> > > > and expecting that abs() should treat this value as "signed".
> > > >
> > >
> > > um, yes, I'd forgotten that one.  That's a show-stopper.
> > 
> > May be we can demand to fix them?
> > 
> > I agree with Alexey, it is a bit ugly to have abs() and abs64(), and abs()
> > itself doesn't look very nice.
> > 
> > What if we simply add
> > 
> > 	BUILD_BUG_ON( (typeof(_x)-1) > 0 );
> > 
> > into abs()?
> > 
> > After that it would be trivial to find the offenders and fix them,
> > 
> > 	- abs(unsigned_int)
> > 	+ abs((int) unsigned_int)
> 
> Something like that.  But it should be done before we change abs(), to
> avoid nasty breakage in obscure places.
> 
> Or we rework the abs() implementation so that the abs(unsigned)
> behavior is unchanged.
> 
> There will remain the problem that the abs() return value's signedness
> has changed.

Maybe I can sneak in kabs() ang gradually convert?

Reading abs(3) manpage, it returns int minumum, so semantic change
notice would be nice.

Anyway patch as posted was buggy in the following sense:

	_b_t_c_p(char, signed char) == 0

		AND
	
	_b_t_c_p(char, unsigned char) == 0

so if char is signed, it doesn't work.