Skip to contentSkip to conversation

remove abs64()

PATCH In review Alexey Dobriyan 4 participants 12 messages Apr 12, 2011 – Apr 13, 2011

Subsystems (3)
DRM drivers DRM misc Library code
lore
Share & tools
lore

Alexey Dobriyan

author
#0 lore raw

PATCH 1/1 remove abs64()

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 <[email protected]>

---

 drivers/gpu/drm/drm_irq.c |    4 ++--
 include/linux/kernel.h    |   43 +++++++++++++++++++++----------------------
 lib/div64.c               |    2 +-
 3 files changed, 24 insertions(+), 25 deletions(-)
drivers/gpu/drm/drm_irq.c +2 -2
@@ -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;
include/linux/kernel.h +21 -22

49 lines not on this page · raw message

lib/div64.c +1 -1
@@ -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;
#1 lore raw
Quoted 4 lines · 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.

Quoted 50 lines

New abs() doesn't expand type needlessly.

Signed-off-by Alexey Dobriyan <[email protected]>

---

 drivers/gpu/drm/drm_irq.c |    4 ++--
 include/linux/kernel.h    |   43 +++++++++++++++++++++----------------------
 lib/div64.c               |    2 +-
 3 files changed, 24 insertions(+), 25 deletions(-)
include/linux/kernel.h +11 -22
@@ -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(						\

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 10 lines
include/linux/kernel.h +10 -0
+		__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

maintainer
#2 lore raw
Quoted 43 lines · On Tue, 12 Apr 2011 14:07:26 -0700 Randy Dunlap <[email protected]> wrote:
Quoted 43 lines
include/linux/kernel.h +15 -22
@@ -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),		\
Quoted 8 lines

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 6 lines
include/linux/kernel.h +6 -0
+		(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().

author
#3 lore raw
Quoted 11 lines · Andrew Morton
Quoted 7 lines · On Tue, 12 Apr 2011 14:07:26 -0700 Randy Dunlap <[email protected]> wrote:

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 5 lines
include/linux/kernel.h +5 -0
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), long long),	\
+		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
+	_x)))));							\
+})

that is better?

Quoted 3 lines

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.

maintainer
#4 lore raw
Quoted 12 lines · On Wed, 13 Apr 2011 00:16:58 +0300 Alexey Dobriyan <[email protected]> wrote:
Quoted 11 lines · Andrew Morton
Quoted 7 lines · On Tue, 12 Apr 2011 14:07:26 -0700 Randy Dunlap <[email protected]> wrote:

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 5 lines
include/linux/kernel.h +5 -0
+	__builtin_choose_expr(						\
+		__builtin_types_compatible_p(typeof(_x), long long),	\
+		(unsigned long long)({ _x < 0 ? -_x : _x; }),		\
+	_x)))));							\
+})

that is better?

Quoted 6 lines
Quoted 3 lines

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.

#5 lore raw
Quoted 17 lines · On Tue, 12 Apr 2011 14:33:40 -0700 Andrew Morton wrote:
Quoted 14 lines · On Wed, 13 Apr 2011 00:16:58 +0300 Alexey Dobriyan <[email protected]> wrote:
Quoted 13 lines · Andrew Morton
Quoted 7 lines · On Tue, 12 Apr 2011 14:07:26 -0700 Randy Dunlap <[email protected]> wrote:

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 5 lines
include/linux/kernel.h +5 -0
+	__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.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

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 ***

author
#6 lore raw
In reply to Randy Dunlap
Quoted 8 lines · Randy Dunlap
Quoted 4 lines · 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?

Quoted 16 lines
Quoted 4 lines
include/linux/kernel.h +0 -1
 
-#define abs64(x) ({				\

include/linux/kernel.h extern int _cond_resched(void); · line 152

Quoted 10 lines
include/linux/kernel.h +7 -3
-		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.

#7 lore raw
In reply to Alexey Dobriyan
Quoted 12 lines · Alexey Dobriyan
include/linux/kernel.h +11 -0
+#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(						\

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 10 lines
include/linux/kernel.h +10 -0
+		__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.

maintainer
#8 lore raw
Quoted 26 lines · On Wed, 13 Apr 2011 16:27:03 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 12 lines · Alexey Dobriyan
include/linux/kernel.h +11 -0
+#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(						\

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 10 lines
include/linux/kernel.h +10 -0
+		__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.

Quoted 3 lines

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.

#9 lore raw
Quoted 30 lines · Andrew Morton
Quoted 26 lines · On Wed, 13 Apr 2011 16:27:03 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 12 lines · Alexey Dobriyan
include/linux/kernel.h +11 -0
+#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(						\

include/linux/kernel.h extern int _cond_resched(void); · line 166

Quoted 10 lines
include/linux/kernel.h +10 -0
+		__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.

Quoted 5 lines
Quoted 3 lines

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.

maintainer
#10 lore raw
Quoted 13 lines · On Wed, 13 Apr 2011 22:20:31 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 5 lines
Quoted 3 lines

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

Quoted 8 lines
	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.

author
#11 lore raw
Quoted 25 lines · Andrew Morton
Quoted 13 lines · On Wed, 13 Apr 2011 22:20:31 +0200 Oleg Nesterov <[email protected]> wrote:
Quoted 5 lines
Quoted 3 lines

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

Quoted 8 lines
	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)

Quoted 8 lines

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.