User account creation filtered due to spam.

Bug 35509 - [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution
Summary: [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: 4.4.0
Assignee: Kaveh Ghazi
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2008-03-08 10:02 UTC by Dmitry K.
Modified: 2008-05-19 06:19 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work: 4.4.0
Known to fail:
Last reconfirmed: 2008-05-19 06:16:49


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry K. 2008-03-08 10:02:57 UTC
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.
Comment 1 Richard Biener 2008-03-08 10:40:32 UTC
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.
Comment 2 Richard Biener 2008-03-08 10:52:15 UTC
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?
Comment 3 Kaveh Ghazi 2008-03-08 15:45:00 UTC
(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.
Comment 4 Uroš Bizjak 2008-03-08 15:51:14 UTC
(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.


Comment 5 Andrew Pinski 2008-03-08 15:55:27 UTC
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.
Comment 6 Kaveh Ghazi 2008-04-13 16:27:57 UTC
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?


Comment 7 Dmitry K. 2008-04-14 10:55:20 UTC
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.
Comment 8 Richard Biener 2008-04-14 11:06:28 UTC
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.
Comment 9 Kaveh Ghazi 2008-04-15 00:38:00 UTC
(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.

Comment 10 Kaveh Ghazi 2008-04-15 01:36:49 UTC
(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?

Comment 11 rguenther@suse.de 2008-04-15 08:58:20 UTC
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.
Comment 12 Mark Mitchell 2008-04-28 04:37:21 UTC
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.
Comment 13 Kaveh Ghazi 2008-05-18 15:51:15 UTC
Patch to implement a separate builtin isinf_sign posted here:
http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01067.html
Comment 14 Kaveh Ghazi 2008-05-18 23:20:24 UTC
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

Comment 15 Kaveh Ghazi 2008-05-19 06:16:20 UTC
Addressed this by adding __builtin_isinf_sign()
Comment 16 Kaveh Ghazi 2008-05-19 06:19:41 UTC
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)