[Bug target/35509] New: [avr] 4.3.0: builtin isinf() mismatch to compile-time substitution

[Bug target/35509] New: [avr] 4.3.0: builtin isinf() mismatch to compile-time substitution

[dmixm at marine dot febras dot ru]

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.


-- 
           Summary: [avr] 4.3.0: builtin isinf() mismatch to compile-time
                    substitution
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: dmixm at marine dot febras dot ru


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[rguenth at gcc dot gnu dot org]

------- Comment #1 from rguenth at gcc dot gnu dot org  2008-03-08 10:40 -------
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.


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
          Component|target                      |middle-end
     Ever Confirmed|0                           |1
           Keywords|                            |wrong-code
   Last reconfirmed|0000-00-00 00:00:00         |2008-03-08 10:40:32
               date|                            |
            Summary|[avr] 4.3.0: builtin isinf()|[4.3/4.4 Regression] builtin
                   |mismatch to compile-time    |isinf() mismatch to compile-
                   |substitution                |time substitution
   Target Milestone|---                         |4.3.1


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[rguenth at gcc dot gnu dot org]

------- Comment #2 from rguenth at gcc dot gnu dot org  2008-03-08 10:52 -------
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?


-- 

rguenth at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ghazi at gcc dot gnu dot
                   |                            |org, uros at gcc dot gnu dot
                   |                            |org, rguenth at gcc dot gnu
                   |                            |dot org, krebbel at gcc dot
                   |                            |gnu dot org


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #3 from ghazi at gcc dot gnu dot org  2008-03-08 15:45 -------
(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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ubizjak at gmail dot com]

------- Comment #4 from ubizjak at gmail dot com  2008-03-08 15:51 -------
(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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[pinskia at gcc dot gnu dot org]

------- Comment #5 from pinskia at gcc dot gnu dot org  2008-03-08 15:55 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #6 from ghazi at gcc dot gnu dot org  2008-04-13 16:27 -------
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?


-- 

ghazi at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|2008-03-08 10:40:32         |2008-04-13 16:27:57
               date|                            |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[dmixm at marine dot febras dot ru]

------- Comment #7 from dmixm at marine dot febras dot ru  2008-04-14 10:55 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[rguenth at gcc dot gnu dot org]

------- Comment #8 from rguenth at gcc dot gnu dot org  2008-04-14 11:06 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #9 from ghazi at gcc dot gnu dot org  2008-04-15 00:38 -------
(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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- 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.

> A different workaround would be to canonicalize the return value of isinf to
> bool.

Doesn't this change the interface defined by c99?


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[rguenther at suse dot de]

------- Comment #11 from rguenther at suse dot de  2008-04-15 08:58 -------
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.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[mmitchel at gcc dot gnu dot org]

------- Comment #12 from mmitchel at gcc dot gnu dot org  2008-04-28 04:37 -------
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.


-- 

mmitchel at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #13 from ghazi at gcc dot gnu dot org  2008-05-18 15:51 -------
Patch to implement a separate builtin isinf_sign posted here:
http://gcc.gnu.org/ml/gcc-patches/2008-05/msg01067.html


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #14 from ghazi at gcc dot gnu dot org  2008-05-18 23:20 -------
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


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #15 from ghazi at gcc dot gnu dot org  2008-05-19 06:16 -------
Addressed this by adding __builtin_isinf_sign()


-- 

ghazi at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

-- 

ghazi at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot gnu   |ghazi at gcc dot gnu dot org
                   |dot org                     |
             Status|REOPENED                    |ASSIGNED
   Last reconfirmed|2008-04-13 16:27:57         |2008-05-19 06:16:49
               date|                            |
   Target Milestone|4.3.1                       |4.4.0


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509

[Bug middle-end/35509] [4.3/4.4 Regression] builtin isinf() mismatch to compile-time substitution

[ghazi at gcc dot gnu dot org]

------- Comment #16 from ghazi at gcc dot gnu dot org  2008-05-19 06:19 -------
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)


-- 

ghazi at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
      Known to work|                            |4.4.0
         Resolution|                            |FIXED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=35509