Note that the abs(long) and abs(long long) overloads, which you probably want around, are actually declared in <cstdlib>, which you are not including (-std=c++11 of course). Otherwise, <cstdlib>, which is available only because is included as an implementation detail, is fine per the Standard, any integer is supposed to unconditionally become double.

But indeed you are right that long term you want <cstdint> to be non empty for SH. That shouldn't be too hard to implement, there is already very solid infrastructure for that (as an header installed by GCC, I mean). This is a target issue, really.

Of course the second time I meant <cmath> not <cstdlib>.

Actually, <cstdlib> is normally included by <algorithm> as an implementation detail, thus I suspect the C++11 bits in <cstdlib> are also disabled for this target. Looks like the target maintainers need some serious pinging ;)

Sorry, forgot that <cstdint> only declares some types. Thus, all in all, the issue is really that <cstdlib> should provide abs (long) and abs (long long) and it isn't because below it llabs is missing, as you correctly mentioned. But the main point lives: in the generic bits of the library we can't do much about that, because <cmath> isn't supposed to treat differently the various integer types. Really the target maintainers should do something about llabs: note that adding some code to the SH libstdc++ config should be also fine (without touching newlib I mean)

(In reply to comment #1)
> Note that the abs(long) and abs(long long) overloads, which you probably want
> around, are actually declared in <cstdlib>, which you are not including
> (-std=c++11 of course).

In cstdlib, it would be possible to have abs(long long) protected by the simpler _GLIBCXX_USE_LONG_LONG instead of the full _GLIBCXX_USE_C99 (it only requires language support and not C library support), or even not protected at all (there are other places in the library that assume long long exists). There could also be an __int128 version.

> Otherwise, <cstdlib>, which is available only because
> is included as an implementation detail, is fine per the Standard, any integer
> is supposed to unconditionally become double.

Er, abs is the exception there. signed integers have their own overloads of abs (in cstdlib). Seems like only unsigned integers get converted to double, which is rather funny :-) Not sure that was intentional in the standard...

(In reply to comment #1)
> Note that the abs(long) and abs(long long) overloads, which you probably want
> around, are actually declared in <cstdlib>, which you are not including
> (-std=c++11 of course). Otherwise, <cstdlib>, which is available only because
> is included as an implementation detail, is fine per the Standard, any integer
> is supposed to unconditionally become double.

Brilliant! ;)

> 
> But indeed you are right that long term you want <cstdint> to be non empty for
> SH. That shouldn't be too hard to implement, there is already very solid
> infrastructure for that (as an header installed by GCC, I mean). This is a
> target issue, really.

(In reply to comment #3)
> Actually, <cstdlib> is normally included by <algorithm> as an implementation
> detail, thus I suspect the C++11 bits in <cstdlib> are also disabled for this
> target. 

Yeah, seems like that.

> Looks like the target maintainers need some serious pinging ;)

Probably this is due to newlib-bleh.  I'm not testing sh-linux, but I would
assume that glibc does the right thing there.  Kaz, just in case, could you
please check this on sh-linux?

Thanks for the message, Marc, I think we are both missing something here, because abs (long long) apparently (I didn't write the code) has already a fall back open coded implementation in namespace __gnu_cxx, so? Can you please investigate and see if in case we can have a simple patch benefiting a range of targets? I bet we can.

(In reply to comment #5)
> Er, abs is the exception there. signed integers have their own overloads of abs
> (in cstdlib). Seems like only unsigned integers get converted to double, which
> is rather funny :-) Not sure that was intentional in the standard...

Thanks god (or whoever) that the following:

unsigned int test_xx (unsigned int a)
{
  return std::abs (a);
}

ends up as a nop.  *phew*.
BTW, not so funny ;)

Actually adding Marc. Please see if we can have a neat enough patch handling std::abs separately, I don't have the time to do that over the next weeks, but should be doable. Please, however, let's try to not make the tangle of #ifdef even more complex, because I know that a lot of the configury for these features can be more fine grained but the situation can quickly go out of control...

(In reply to comment #8)
> Thanks god (or whoever)

Would that make me a half-god? seems dangerous...

> that the following:
> 
> unsigned int test_xx (unsigned int a)
> {
>   return std::abs (a);
> }
> 
> ends up as a nop.  *phew*.

Try it with long (or long long) instead of int now...

(In reply to comment #10)
> (In reply to comment #8)
> > Thanks god (or whoever)
> 
> Would that make me a half-god? seems dangerous...
> 
> > that the following:
> > 
> > unsigned int test_xx (unsigned int a)
> > {
> >   return std::abs (a);
> > }
> > 
> > ends up as a nop.  *phew*.
> 
> Try it with long (or long long) instead of int now...

You really scared me now.
unsigned long is also a nop.
unsigned short is also a nop.
unsigned char is also a nop.

*drumroll*

bool bleh (bool x)
{
  return std::abs (x);
}

gets a fine floating treatment -- it is converted to double and then tested for != 0.0.

(In reply to comment #11)
> You really scared me now.
> unsigned long is also a nop.

That's only because unsigned long has less bits than the mantissa of double on your platform. On x86_64, it is converted to double (and converting between unsigned long and double is ugly).

> *drumroll*
> 
> bool bleh (bool x)
> {
>   return std::abs (x);
> }
> 
> gets a fine floating treatment -- it is converted to double and then tested for
> != 0.0.

Interesting.

To clarify, 2 things happen.
1) __builtin_fabs seems to have some magic to detect that it is called on an unsigned type (bool included) and do nothing in that case. std::abs<unsigned> is thus reduced to a cast to double.
2) casting from an integer to double and back to an integer, if the original integer type is small enough that double can represent it exactly, the casts are collapsed. Casts to bool are represented internally as !=0, so the optimization is missed. You can file an RFE for that one if you want.

Created attachment 28256 [details]
always define abs(long long)

This is what I had in mind (this abs(long long) is not a fallback, that's the only implementation there is currently). Completely untested.

Oleg, do you want to experiment with something like that?

(In reply to comment #13)
> Created attachment 28256 [details]
> always define abs(long long)
> 
> This is what I had in mind (this abs(long long) is not a fallback, that's the
> only implementation there is currently). Completely untested.
> 
> Oleg, do you want to experiment with something like that?

Yep, sure.  Looks straight forward.  I'll test it in my setup within the next couple of days.  Thanks!

Ok... thanks Marc for handling this. If we could handle in the same way div(long long, long long), it would be great and more consistent. Are we sure we can't?

Also, too bad that we don't use llabs (and lldiv) as implementation details when safe, it would be more consistent with the long overloads. I seemed to remember that we were already doing that.

I also note that so far we added the long long overloads only in C++11 mode. I know that elsewhere in the library we have long long overloads in C++98 mode too as extensions, but then maybe better wrapping in _GLIBCXX_USE_LONG_LONG?

(In reply to comment #15)
> Ok... thanks Marc for handling this. If we could handle in the same way
> div(long long, long long), it would be great and more consistent. Are we sure
> we can't?

It needs lldiv_t.

> Also, too bad that we don't use llabs (and lldiv) as implementation details
> when safe, it would be more consistent with the long overloads. I seemed to
> remember that we were already doing that.

I was a bit surprised to see that indeed.

For llabs: why bother, it isn't like there is anything fancy llabs could be doing. Is the point that with -Os, a call to llabs is slightly shorter than an inlined version? We could use llabs when safe, it's just more macros...

For lldiv: glibc's implementation does more than a simple / and % (the behavior of div with negative numbers is more strictly standardized than that of / and %, so on some platforms the adjustment may be needed, but I don't know if such platforms are still used). On the other hand, gcc doesn't have a builtin for it, so if we call lldiv, it can never be inlined.

(In reply to comment #16)
> I also note that so far we added the long long overloads only in C++11 mode. I
> know that elsewhere in the library we have long long overloads in C++98 mode
> too as extensions, but then maybe better wrapping in _GLIBCXX_USE_LONG_LONG?

If someone calls abs on a long long in C++98, does that mean they will silently use the double version? I saw some other places in the library that used long long without any macro protection, so I did the same.

I really don't know what's best, I don't understand those weird macros, it is your call.

Ok, let's leave llabs and lldiv alone, for now at least.

Otherwise, the lldiv_t issue is really annoying, I'm not sure (anymore) we want to handle abs and div separately...

(In reply to comment #17)
> 
> For llabs: why bother, it isn't like there is anything fancy llabs could be
> doing. Is the point that with -Os, a call to llabs is slightly shorter than an
> inlined version? 

I haven't checked, but the only kind of target where this could be true is
probably something < 32 bit.  Why not leave this decision up to the target
and/or middle-end abs expansion?  Stuff like "return __x >= 0 ? __x : -__x;" is
recognized by the middle-end and turned into an abs RTL, if the target defines
one for the mode in question.  If not it will expand into
if-then-else-something or try out some branch-free alternative.

(In reply to comment #18)
> (In reply to comment #16)
> > I also note that so far we added the long long overloads only in C++11 mode. I
> > know that elsewhere in the library we have long long overloads in C++98 mode
> > too as extensions, but then maybe better wrapping in _GLIBCXX_USE_LONG_LONG?
> 
> If someone calls abs on a long long in C++98, does that mean they will silently
> use the double version? I saw some other places in the library that used long
> long without any macro protection, so I did the same.

Where, exactly? I think we should be more consistent about long long. I seem to remember only tr1/type_traits maybe, not actual overloads. Anyway, _GLIBCXX_USE_LONG_LONG is always defined by default, I think it's Ok to protect with it and allow people to disable the overloads in C++98 mode if they want.

(In reply to comment #21)
> I saw some other places in the library that used long
> long without any macro protection, so I did the same.
> 
> Where, exactly?

numeric_limits for instance.

That's fine, not an overload.

(In reply to comment #23)
> That's fine, not an overload.

__lg?

That's only internal.

(In reply to comment #6)
FYI, same on sh-linux, though my glibc for gcc tests is a bit dated. Ugh.

Created attachment 28257 [details]
always define abs(long long) (v2)

Still untested, and getting uglier.

Indeed uglier ;) but I must say that overall I think we have to do something like this. I'm still annoyed that because of the type we can't handle in the same way div but I'm also coming to the conclusion that in terms of actual code we can't do much about it, besides making the configury more fine grained, and separating the stdlib.h bits, which maybe could help other targets but, if I understand correctly, would not help newlib anyway, because llabs is completely missing
 (Note that around the corner there is us providing fall backs for *everything* missing from the target libc, where in this case *us* has nothing to do with *me* ;)

Created attachment 28303 [details]
always define abs(long long) (v2.1)

This an updated patch against rev 191865, with a typo fix (missing '\').
With this patch the std::abs problem goes away on my xgcc setup.  So far I've tested it only with 'make all'.

Oops, I thought Marc was already in charge.

Daniel created this issue (thanks):

http://cplusplus.github.com/LWG/lwg-active.html#2192

Author: glisse
Date: Fri Oct  5 16:20:44 2012
New Revision: 192132

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192132
Log:
2012-10-05  Marc Glisse  <marc.glisse@inria.fr>

	PR libstdc++/54686
	* include/c_std/cstdlib (abs(long long)): Define with
	__builtin_llabs when we have long long.
	(abs(long)): Use __builtin_labs.
	(abs(__int128)): Define when we have __int128.
	* testsuite/26_numerics/headers/cstdlib/54686.c: New file.


Added:
    trunk/libstdc++-v3/testsuite/26_numerics/headers/cstdlib/54686.c   (with props)
Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/c_std/cstdlib

Propchange: trunk/libstdc++-v3/testsuite/26_numerics/headers/cstdlib/54686.c
            ('svn:eol-style' added)

Propchange: trunk/libstdc++-v3/testsuite/26_numerics/headers/cstdlib/54686.c
            ('svn:keywords' added)

Fixed on trunk. I am not backporting that.

Looks like you forgot the include/c_global/cstdlib bits?

(In reply to comment #34)
> Looks like you forgot the include/c_global/cstdlib bits?

Hmm right, I patched the wrong directory. Maybe c_std/cstdlib should be removed? It looks like a clone of the file c_global/cstdlib except that it hasn't been kept up to date...

What I know is that Linux by default uses c_global. There is configury selecting the directory, but I didn't write the logic, Benjamin did, frankly I don't know which specific target use c_std these times. In such case the safe thing to do in my opinion is changing in a consistent way existing bits in the headers and not adding or removing anything else.

I see the following in cstdlib:

namespace std
{
#if !_GLIBCXX_USE_C99_LONG_LONG_DYNAMIC
  // types
  using std::lldiv_t;


Is there a particular reason to import stuff into its own namespace?

Eh, eh, hard to reconstruct now what happened at the time. Looks like an svn pasto or a plain pasto. I suppose that entire block can be removed?

Probably a plain pasto of mine dating back to when was copying stuff from tr1/* in namespace std::tr1 to std:: protected by __GXX_EXPERIMENTAL_CXX0X__. Looks like I didn't notice that in this specific case the std version of the facilities exists in C++98 mode too (as a GNU C99 extension). Eh ;)

(In reply to comment #38)
> Eh, eh, hard to reconstruct now what happened at the time. Looks like an svn
> pasto or a plain pasto. I suppose that entire block can be removed?

Probably, I'll leave that to you. Testing for the patch adding abs is in progress.

Removing that block would make the c_global and c_std versions even closer...

Remove it, remove it.

Author: glisse
Date: Fri Oct  5 19:10:22 2012
New Revision: 192138

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192138
Log:
2012-10-05  Marc Glisse  <marc.glisse@inria.fr>

	PR libstdc++/54686
	* include/c_global/cstdlib (abs(long long)): Define with
	__builtin_llabs when we have long long.
	(abs(long)): Use __builtin_labs.
	(abs(__int128)): Define when we have __int128.


Modified:
    trunk/libstdc++-v3/ChangeLog
    trunk/libstdc++-v3/include/c_global/cstdlib