Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Alexey Dobriyan]

On Fri, Sep 18, 2015 at 12:36 AM,  <akpm@linux-foundation.org> wrote:


> -#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 abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({      \
> +               s64 __x = (x);                                          \
> +               (__x < 0) ? -__x : __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;                                                    \
> +       }))

1. "char" should be banned, because it's signedness is not well defined.
2. there is unnecessary expansion to long.

I've sent kabs() before which didn't go in because it didn't work for
INT_MAX et al
(don't worry, this abs() doens't as well) but it is nicer that this
version in other aspects
(hopefully).

[PATCH v2] Add kabs()
http://marc.info/?l=linux-kernel&m=133518745522740&w=4

As for INT_MIN, it pretty impossible to make compiler to not generate a branch
despite generating correct result without a branch.

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Andrew Morton]

On Wed, 23 Sep 2015 16:44:29 +0300 Alexey Dobriyan <adobriyan@gmail.com> wrote:

> On Fri, Sep 18, 2015 at 12:36 AM,  <akpm@linux-foundation.org> wrote:
> 
> 
> > -#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 abs(x) __builtin_choose_expr(sizeof(x) == sizeof(s64), ({      \
> > +               s64 __x = (x);                                          \
> > +               (__x < 0) ? -__x : __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;                                                    \
> > +       }))
> 
> 1. "char" should be banned, because it's signedness is not well defined.
> 2. there is unnecessary expansion to long.
> 
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
> (don't worry, this abs() doens't as well) but it is nicer that this
> version in other aspects
> (hopefully).
> 
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Looks nice.  Care to send along a patch which updates the abs() in
linux-next?

> As for INT_MIN, it pretty impossible to make compiler to not generate a branch
> despite generating correct result without a branch.

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Michal Nazarewicz]

On Wed, Sep 23 2015, Alexey Dobriyan wrote:
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
> (don't worry, this abs() doens't as well) but it is nicer that this
> version in other aspects
> (hopefully).
>
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Perhaps:

+	(void)(_x)))))));						\

instead of:

+	_x))))));							\

at the end.  Since kabs makes no sense for unsigned types it’s best to
fail with compile-time error than to let user think that the call is
actually doing something.

Also, you don’t need ({ … }) around the ‘_x < 0 ? -_x : _x’ expression,
do you?

-- 
Best regards,                                            _     _
.o. | Liege of Serenely Enlightened Majesty of         o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>-----ooO--(_)--Ooo--

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Alexey Dobriyan]

On Thu, Sep 24, 2015 at 4:32 PM, Michal Nazarewicz <mina86@mina86.com> wrote:
> On Wed, Sep 23 2015, Alexey Dobriyan wrote:
>> I've sent kabs() before which didn't go in because it didn't work for
>> INT_MAX et al
>> (don't worry, this abs() doens't as well) but it is nicer that this
>> version in other aspects
>> (hopefully).
>>
>> [PATCH v2] Add kabs()
>> http://marc.info/?l=linux-kernel&m=133518745522740&w=4
>
> Perhaps:
>
> +       (void)(_x)))))));                                               \
>
> instead of:
>
> +       _x))))));                                                       \
>
> at the end.  Since kabs makes no sense for unsigned types it’s best to
> fail with compile-time error than to let user think that the call is
> actually doing something.

I thought so, but the amount of uses like

  unsigned int = abs(unsigned int - unsigned int)

is non trivial. With a few exceptions it is about 170 on
x86_64 allmodconfig. Most of them are correct because abs()
casts to signed first. So if you want to expose such uses,
then you are forced to write

  unsigned int = kabs((int)(unsigned int - unsigned int));

Don't know if it's a good thing.

> Also, you don’t need ({ … }) around the ‘_x < 0 ? -_x : _x’ expression,
> do you?

Seems so, thank you for noticing.

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Linus Torvalds]

On Wed, Sep 23, 2015 at 6:44 AM, Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> I've sent kabs() before which didn't go in because it didn't work for
> INT_MAX et al
>
> [PATCH v2] Add kabs()
> http://marc.info/?l=linux-kernel&m=133518745522740&w=4

Yeah, no, that's bad.

Testing against char/short is pointless. They should get upgraded to "int".

And guys, stop the idiotic "shouldn't work on unsigned values" crap.
It damn well should work on unsigned values, and the semantics we want
is that we treat it as a signed value.

Why? That's how "abs()" works. Really. Stop fighting it. If you were
to use a real "abs()", it takes an "int" argument, and if you pass it
an unsigned value, it gets converted to "int".

End of story. Stop with the broken "but but but unsigned" crap already.

So the end result is that

 (a) we should look at the *size* of the argument type, not the
signedness, because the signedness is immaterial

 (b) we should *not* cast the thing to unsigned, because we
traditionally haven't.

 (c) we should definitely not use smaller than "int" as a minimum
size, no crazy games with char/short. And considering our legacy, I
think we should probably skip "int" and stay with "long" as the
minimum size.

So I *much* prefer Michal's "abs()" definition that doesn't radically
change the meaning (it keeps the old behavior *except* if you pass in
a bigger size than "long", in which case it will auto-widen to "s64").
That's the maximally compatible model given the "we handle bigger
types automatically" extension.

Because "maximally compatible" is a strong argument. The signedness
stuff and the "let's try to use the exact same type" is just bogus.

One thing that *is* interesting is "what if 'long' and 's64' are the
same size?" In particular, it means that right now Michal's patch
*always* returns "long" on a 64-bit architecture, but will return
"long" or "s64" on a 32-bit one. The reason that is somewhat
interesting is that while the sizes and values are the same, and the
resulting C type expansions are "equivalent" types, i people *print*
things, you have to use different modifiers for the two cases. So you
might get warnings on 32-bit architectures and not get them on 64-bit,
or vice versa.

However, I don't see a good solution for that. And assuming we don't
use "abs()" in an expression to printk(), I guess it doesn't much
matter either.

              Linus

Re: + kernelh-make-abs-work-with-64-bit-types.patch added to -mm tree

[Michal Nazarewicz]

On Thu, Sep 24 2015, Linus Torvalds wrote:
> One thing that *is* interesting is "what if 'long' and 's64' are the
> same size?" In particular, it means that right now Michal's patch
> *always* returns "long" on a 64-bit architecture, but will return
> "long" or "s64" on a 32-bit one.

That’s not accurate.  s64 is defined as long long so:
- on 64-bit architectures, the macro will return s64 (i.e. long long)
  for long arguments (because sizeof(long) == 8 == sizeof(s64) and the
  first path is taken), but
- on 32-bit architectures, it will return long for long arguments (since
  sizeof(long) == 4 != 8 == sizeof(long long) and the second path is
  taken).
But yes, the point remains, depending on architecture, the macro returns
different type for long arguments.

> The reason that is somewhat interesting is that while the sizes and
> values are the same, and the resulting C type expansions are
> "equivalent" types, i people *print* things, you have to use different
> modifiers for the two cases. So you might get warnings on 32-bit
> architectures and not get them on 64-bit, or vice versa.
>
> However, I don't see a good solution for that. And assuming we don't
> use "abs()" in an expression to printk(), I guess it doesn't much
> matter either.

This should do the trick:

#define abs(x) __builtin_choose_expr(				\
	__builtin_types_compatible_p(typeof(x), s64) ||		\
	__builtin_types_compatible_p(typeof(x), u64),		\
	({ s64 __x = (x); __x < 0 ? -__x : __x; }),		\
	__builtin_choose_expr(sizeof(x) <= sizeof(long), ({	\
		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;						\
	}), (void)(x)))

It’ll return s64 for s64 and u64 (i.e. long long and unsigned long long)
types, long for anything whose sizeof <= sizeof(long) and will bail out
with compile time error if used for any other type (if return value is
used).

I dunno whether added complexity is worth solving the problem though.

-- 
Best regards,                                            _     _
.o. | Liege of Serenely Enlightened Majesty of         o' \,=./ `o
..o | Computer Science,  ミハウ “mina86” ナザレヴイツ  (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>-----ooO--(_)--Ooo--