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(-)
--- 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;
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 <[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(-)
> --- 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
On Tue, 12 Apr 2011 14:07:26 -0700
Randy Dunlap <[email protected]> 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().
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.
On Tue, Apr 12, 2011 at 02:10:40PM -0700, Andrew Morton wrote:
> On Tue, 12 Apr 2011 14:07:26 -0700
> Randy Dunlap <[email protected]> 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.
On Wed, 13 Apr 2011 00:16:58 +0300
Alexey Dobriyan <[email protected]> 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 <[email protected]> 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.
On Tue, 12 Apr 2011 14:33:40 -0700 Andrew Morton wrote:
> On Wed, 13 Apr 2011 00:16:58 +0300
> Alexey Dobriyan <[email protected]> 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 <[email protected]> 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 ***
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.
On Wed, 13 Apr 2011 16:27:03 +0200
Oleg Nesterov <[email protected]> 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.
On 04/13, Andrew Morton wrote:
>
> On Wed, 13 Apr 2011 16:27:03 +0200
> Oleg Nesterov <[email protected]> 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.
On Wed, 13 Apr 2011 22:20:31 +0200
Oleg Nesterov <[email protected]> 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.
On Wed, Apr 13, 2011 at 01:26:29PM -0700, Andrew Morton wrote:
> On Wed, 13 Apr 2011 22:20:31 +0200
> Oleg Nesterov <[email protected]> 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.