[Bug libstdc++/58625] std::signbit always converts to double
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #16 from joseph at codesourcery dot com --- That C __builtin_signbit should be type-generic is bug 36757.
[Bug libstdc++/58625] std::signbit always converts to double
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625
Tijl Coosemans changed:
What|Removed |Added
CC||tijl at coosemans dot org
--- Comment #15 from Tijl Coosemans ---
(In reply to Jakub Jelinek from comment #8)
> That said, I'd say that every conversion to double that would change the
> sign looks wrong to me, no matter of what the rounding mode is, except
> perhaps for NaN canonicalization and that sNaN could be signalled. So IMHO
> this is mostly an optimization thing, not correctness.
I do think this is a correctness problem for long double. Consider the
following C code:
int test( long double x ) {
return( __builtin_signbit( x ));
}
With "gcc49 -O2 -S test.c" on x86_64 this compiles to:
fldt 8(%rsp)
fstpl -16(%rsp)
movsd -16(%rsp), %xmm0
movmskpd %xmm0, %eax
andl $1, %eax
ret
The fstpl instruction converts long double to double and may generate all kinds
of floating point exceptions if the value can't be converted exactly and I
don't think signbit(x) is allowed to generate FP exceptions. So it would be
best to fix the c_std version too.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 Paolo Carlini changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED Assignee|paolo.carlini at oracle dot com|unassigned at gcc dot gnu.org --- Comment #14 from Paolo Carlini --- Done.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #13 from paolo at gcc dot gnu.org --- Author: paolo Date: Sun Oct 6 13:44:47 2013 New Revision: 203228 URL: http://gcc.gnu.org/viewcvs?rev=203228&root=gcc&view=rev Log: 2013-10-06 Oleg Endo Paolo Carlini PR libstdc++/58625 * include/c_global/cmath (signbit): Use __builtin_signbitf and __builtin_signbitl. Modified: trunk/libstdc++-v3/ChangeLog trunk/libstdc++-v3/include/c_global/cmath
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 Paolo Carlini changed: What|Removed |Added Status|NEW |ASSIGNED Assignee|unassigned at gcc dot gnu.org |paolo.carlini at oracle dot com Target Milestone|--- |4.9.0 --- Comment #12 from Paolo Carlini --- Mine.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #11 from Paolo Carlini --- At some point, as we discussed already a bit, we should even try to remove completely the c_std version. For now as I said, let's just leave it alone.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625
--- Comment #10 from Daniel Krügler ---
(In reply to Oleg Endo from comment #4)
> There's another place in file libstdc++-v3/include/c_std/cmath:
>
> template
> inline typename __gnu_cxx::__enable_if<__is_arithmetic<_Tp>::__value,
> int>::__type
> signbit(_Tp __f)
> {
> typedef typename __gnu_cxx::__promote<_Tp>::__type __type;
> return __builtin_signbit(__type(__f));
> }
I think this template should no longer exist after resolution of
http://cplusplus.github.io/LWG/lwg-defects.html#1327
(The P/R doesn't become very clear in the quoted reference. In essence it
replaced the previous template by the three overloads mentioned above, see
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2011/n3289.pdf
)
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #9 from Paolo Carlini --- Ok, thanks a lot for the comments. Thus, Oleg, given in particular the last comment from Jakub, let's just apply to mainline your initial proposed change, let's leave for now include/c_std alone. Just test the change, send it to the mailing lists as preapproved. Please also add a short comment before the code, explaining that a type generic signbit builtin isn't available. Thanks!
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #8 from Jakub Jelinek --- That said, I'd say that every conversion to double that would change the sign looks wrong to me, no matter of what the rounding mode is, except perhaps for NaN canonicalization and that sNaN could be signalled. So IMHO this is mostly an optimization thing, not correctness.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #7 from Jakub Jelinek --- I don't think it is a good idea to extend the number of type generic builtins unless strictly necessary, the overloading in C is quite ugly hack.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625
--- Comment #6 from Oleg Endo ---
(In reply to Jakub Jelinek from comment #5)
> Some builtins are type generic, meaning that you can use just the non-{l,f}
> suffixed variant always, but __builtin_signbit isn't among them.
Yep. So libstdc++-v3/include/c_std/cmath (signbit) would require a fix, too in
this case, right? Or maybe rather make the signbit builtin type generic?
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625
--- Comment #5 from Jakub Jelinek ---
Some builtins are type generic, meaning that you can use just the non-{l,f}
suffixed variant always, but __builtin_signbit isn't among them.
Grep shows that these builtins have "type generic" attribute:
DEF_GCC_BUILTIN(BUILT_IN_FPCLASSIFY, "fpclassify",
BT_FN_INT_INT_INT_INT_INT_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISFINITE, "isfinite", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISINF_SIGN, "isinf_sign", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_C99_C90RES_BUILTIN (BUILT_IN_ISINF, "isinf", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC)
DEF_C99_C90RES_BUILTIN (BUILT_IN_ISNAN, "isnan", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISNORMAL, "isnormal", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISGREATER, "isgreater", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISGREATEREQUAL, "isgreaterequal",
BT_FN_INT_VAR, ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISLESS, "isless", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISLESSEQUAL, "islessequal", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISLESSGREATER, "islessgreater", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
DEF_GCC_BUILTIN(BUILT_IN_ISUNORDERED, "isunordered", BT_FN_INT_VAR,
ATTR_CONST_NOTHROW_TYPEGENERIC_LEAF)
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625
--- Comment #4 from Oleg Endo ---
(In reply to Paolo Carlini from comment #1)
> Weird that nobody noticed for so much time.
Probably not everybody is practicing the neurotic habit of checking the asm
output for every bit and piece ;)
(In reply to Paolo Carlini from comment #2)
> Well, now I see this can be certainly a correctness issue, for eg very small
> negative long doubles. Thus I think we should certainly do the change anyway
> for 4.9. For c_std I'm afraid we have to add overloads (I think, essentially
> copy over the corresponding bits of c_global without constexpr). Can you
> send a patch to the libstdc++-v3 mailing list? And, please, double check
> isnan and isinf. Thanks!
Yes, I can do that. But how to check this in a target independent way? Tree
dump and check whether the correct builtin names appear?
There's another place in file libstdc++-v3/include/c_std/cmath:
template
inline typename __gnu_cxx::__enable_if<__is_arithmetic<_Tp>::__value,
int>::__type
signbit(_Tp __f)
{
typedef typename __gnu_cxx::__promote<_Tp>::__type __type;
return __builtin_signbit(__type(__f));
}
I guess that this is supposed to be used when C++ code pulls in and
uses the C macro signbit instead of std::signbit. It doesn't happen on my SH /
newlib cross config -- I get something like ((sizeof(x) == sizeof(float)) ?
__signbitf(x) : __signbitd(x)). But if on some config it does happen, then the
problem would be the same, thus requiring signbit (and friends) overloads as
it's done for sqrt etc in the same file, right?
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 --- Comment #3 from Andrew Pinski --- (In reply to Paolo Carlini from comment #1) > Weird that nobody noticed for so much time. Are there actual testcases for > this? Or in practice it's just an optimization issue, not a correctness > issue? In some cases there could be a correctness issue with some rounding modes though not that many.
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 Paolo Carlini changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2013-10-04 Ever confirmed|0 |1 --- Comment #2 from Paolo Carlini --- Well, now I see this can be certainly a correctness issue, for eg very small negative long doubles. Thus I think we should certainly do the change anyway for 4.9. For c_std I'm afraid we have to add overloads (I think, essentially copy over the corresponding bits of c_global without constexpr). Can you send a patch to the libstdc++-v3 mailing list? And, please, double check isnan and isinf. Thanks!
[Bug libstdc++/58625] std::signbit always converts to double
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58625 Paolo Carlini changed: What|Removed |Added CC||jakub at gcc dot gnu.org --- Comment #1 from Paolo Carlini --- Weird that nobody noticed for so much time. Are there actual testcases for this? Or in practice it's just an optimization issue, not a correctness issue? Now I suspect isinf and isnan too, for which we would not have available *f and *l counterparts at all. Could you please check? I suppose we, libstdc++-v3 people, simply assumed that the C++ front-end provided a full set of intrinsics corresponding to C99 classification macros, besides those for the comparison macros. Adding Jakub in CC: the change per se is so easy, but maybe I'm missing some background for this.
