The next program is aborted in execution (avr-gcc 4.3.0, -Os): int isinf (double); void abort (void); volatile double x = -1.0/0.0; int main () { if (isinf (x) != isinf (-__builtin_inf ())) abort (); return 0; } The second comparison argument is evaluated at compile time: -1 value is substituted. This is traditional result for isinf(-Inf). The first call of isinf() is replaced to inline code, which evaluates the -Inf to +1 value. The early avr-gcc versions are fine: the extern isinf() function was called.
We run into the machine-independent inline expansion that replaces isinf(x) with isgreater(fabs(x),DBL_MAX). This is indeed inconsistend with the constant folding we do.
Note that one might argue that the testcase is invalid as as you say, C99 only says isinf returns non-zero. So strictly speaking comparing the return value of two invocations is not a way to check if both values are an infinity. Of course this is at least an QOI issue. The question is what behavior to retain? The glibc manpage documents returning -1 for -Inf and +1 for +Inf, which we could preserve on glibc targets then not expanding via isgreater(fabs(x),DBL_MAX). (Only i386 and s390 have target dependent expansions for isinf - what is their behavior here?) Kaveh, you introduced the generic fallback, Andreas, what does s390 return here, Uros, how is the situation on i386?
(In reply to comment #2) > Note that one might argue that the testcase is invalid as as you say, C99 only > says isinf returns non-zero. So strictly speaking comparing the return value > of two invocations is not a way to check if both values are an infinity. > Of course this is at least an QOI issue. The question is what behavior to > retain? The glibc manpage documents returning -1 for -Inf and +1 for +Inf, > which we could preserve on glibc targets then not expanding via > isgreater(fabs(x),DBL_MAX). When I wrote this, I was relying on the minimal C99 requirements, and Rth's blessing here: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30652#c2 One of my goals in writing these builtins was that GCC could always rely on the inline expansion, perhaps allowing libc implementors to just define isinf into the builtin. I'd like to keep that as much as possible. So on glibc systems, instead of bypassing the expansion, perhaps we could instead enhance it to return the sign dependent value using builtin signbit to determine the sign and paste it (bitop it) into the result. Another option would be to inline a variant of the ieee glibc isinf function code which seems to be very small. I don't know how generic it is.
(In reply to comment #2) > (Only i386 and s390 have target dependent expansions for isinf - what is > their behavior here?) > > Kaveh, you introduced the generic fallback, Andreas, what does s390 return > here, Uros, how is the situation on i386? When compiled on 32bit target with -std=c99, testcase aborts. For !c99, library call is emitted and the test passes OK.
I don't think this is even a QOI bug. The values are defined by the C99 standard. If the person used -std=c89, then he would get what the target's libc defines them. The inconstaint value is ok at different optimization level too because only non-zero and zero are defined as return values.
So did we decide to fix this or not? FWIW, I could fix the generic isinf transformation using: isinf(x) -> isless(x,DBL_MAX) ? -1 : (isgreater(x,DBL_MAX)) and just always take the runtime penalty calculating for the -inf case when the user says "if (isinf(x))" and doesn't care about the sign. Maybe __builtin_expect preferring zero would help here. (?) I have an preliminary patch to do the ?: transformation, however the obtabs isinf versions will still fail to honor sign. This makes trouble in the testcases if some platforms get -1 and others get 1. So switching it would need to be coordinated. Thoughts?
Possible the (isgreater(fabs(x), DBL_MAX) ? (signbit(x) ? -1 : 1) : 0) will be fast with common finite numbers? Question: in case of AVR target, is it possible to call extern isinf() function regardless of language dialect? Inline code is very undesirable with small memory. Thanks.
I propose that we do the following: - add a warning to the C/C++ frontends that isinf (x) CMP isinf (y) is only well-defined for !isinf (x) && !isinf (y). this doesn't result in a runtime penalty and informs people about the possible problem in their code (it is non-portable). A different workaround would be to canonicalize the return value of isinf to bool.
(In reply to comment #7) > Possible the > (isgreater(fabs(x), DBL_MAX) ? (signbit(x) ? -1 : 1) : 0) > will be fast with common finite numbers? Yes, it seems to be slightly faster (~5%), but you get larger code generated. I think the best thing would be to find a way to fold "if (isinf(x))" to the boolean version and other uses could remain as the sign dependent one. > Question: in case of AVR target, is it possible to call extern > isinf() function regardless of language dialect? Inline code is > very undesirable with small memory. > Thanks. Yes, you can override the middle-end builtins using your own or delete them altogether. See config/pa/pa.c:pa_init_builtins(). If you want an extern isinf, you'll need some magic in the header file to define a macro based on the type size. The nice thing about gcc's type-generic builtins is that you can define isinf(x) to __builtin_isinf(x) and it'll work for all three types: float, double or long double.
(In reply to comment #8) > I propose that we do the following: > - add a warning to the C/C++ frontends that isinf (x) CMP isinf (y) is > only well-defined for !isinf (x) && !isinf (y). > this doesn't result in a runtime penalty and informs people about the possible > problem in their code (it is non-portable). IMHO the above warning would have limited coverage because there are other ways users can test the results of isinf that would run into the sign problem. > A different workaround would be to canonicalize the return value of isinf to > bool. Doesn't this change the interface defined by c99?
Subject: Re: [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution On Tue, 15 Apr 2008, ghazi at gcc dot gnu dot org wrote: > > > ------- Comment #10 from ghazi at gcc dot gnu dot org 2008-04-15 01:36 ------- > (In reply to comment #8) > > I propose that we do the following: > > - add a warning to the C/C++ frontends that isinf (x) CMP isinf (y) is > > only well-defined for !isinf (x) && !isinf (y). > > this doesn't result in a runtime penalty and informs people about the possible > > problem in their code (it is non-portable). > > IMHO the above warning would have limited coverage because there are other ways > users can test the results of isinf that would run into the sign problem. Well, of course. > > A different workaround would be to canonicalize the return value of isinf to > > bool. > > Doesn't this change the interface defined by c99? Yes, we would need to hide that fact and always add promotion code to int via isinf() != 0. Richard.
I don't see that this is a bug, given that the compiler's optimization is within the bounds set by the C standard. I agree with Kaveh's comment that it is desirable that C libraries be able to implement external functions in terms of the builtin, if they choose. If you disagree with that conclusion, please raise the issue on gcc@gcc.gnu.org. If there is consensus that QoI dictates that we should do something different, then we can reopen this issue.
Patch to implement a separate builtin isinf_sign posted here: http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01067.html
Subject: Bug 35509 Author: ghazi Date: Sun May 18 23:19:38 2008 New Revision: 135517 URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=135517 Log: PR middle-end/35509 * builtins.c (mathfn_built_in_1): Renamed from mathfn_built_in. Add `implicit' parameter. Handle BUILT_IN_SIGNBIT. (mathfn_built_in): Rewrite in terms of mathfn_built_in_1. (fold_builtin_classify): Handle BUILT_IN_ISINF_SIGN. (fold_builtin_1): Likewise. * builtins.def (BUILT_IN_ISINF_SIGN): New. c-common.c (check_builtin_function_arguments): Handle BUILT_IN_ISINF_SIGN. * doc/extend.texi: Document __builtin_isinf_sign. * fold-const.c (operand_equal_p): Handle COND_EXPR. testsuite: * gcc.dg/builtins-error.c: Test __builtin_isinf_sign. * gcc.dg/tg-tests.h: Likewise. Mark variables volatile. * gcc.dg/torture/builtin-isinf_sign-1.c: New test. Modified: trunk/gcc/ChangeLog trunk/gcc/builtins.c trunk/gcc/builtins.def trunk/gcc/c-common.c trunk/gcc/doc/extend.texi trunk/gcc/fold-const.c trunk/gcc/testsuite/ChangeLog trunk/gcc/testsuite/gcc.dg/builtins-error.c trunk/gcc/testsuite/gcc.dg/tg-tests.h
Addressed this by adding __builtin_isinf_sign()
Fixed in 4.4.0 by adding a new builtin for systems that care about the sign of isinf's return value. E.g. do this: #define isinf(x) __builtin_isinf_sign(x)