[Bug tree-optimization/61876] New: Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[Bug tree-optimization/61876] New: Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

            Bug ID: 61876
           Summary: Converting __builtin_round + cast into
                    __builtin_lround is not always equivalent in regards
                    to math errno
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ktkachov at gcc dot gnu.org

Consider code:

long int
foo (double a)
{
   return __builtin_round (a);
}

With an aarch64-none-elf toolchain this compiles to:
foo:
        fcvtas  x0, d0
        ret
whereas with an aarch64-linux toolchain this compiles to:
foo:
        b       lround

The linux (glibc) toolchain does not inline the lround implementation.

The suspicious starting point is this code in convert.c:
        CASE_FLT_FN (BUILT_IN_ROUND):
          /* Only convert in ISO C99 mode.  */
          if (!targetm.libc_has_function (function_c99_misc))
            break;
          if (outprec < TYPE_PRECISION (integer_type_node)
              || (outprec == TYPE_PRECISION (integer_type_node)
                  && !TYPE_UNSIGNED (type)))
            fn = mathfn_built_in (s_intype, BUILT_IN_IROUND);
          else if (outprec == TYPE_PRECISION (long_integer_type_node)
                   && !TYPE_UNSIGNED (type))
            fn = mathfn_built_in (s_intype, BUILT_IN_LROUND);
          else if (outprec == TYPE_PRECISION (long_long_integer_type_node)
                   && !TYPE_UNSIGNED (type))
            fn = mathfn_built_in (s_intype, BUILT_IN_LLROUND);
          break;
Basically it does the conversion of (cast to long int + round) == lround
But later on in builtins.c the lround does not get expanded into the sfix optab
unless -fno-math-errno is specified:

  /* There's no easy way to detect the case we need to set EDOM.  */
  if (!flag_errno_math)
    {
      rtx result = gen_reg_rtx (mode);

      /* Wrap the computation of the argument in a SAVE_EXPR, as we may
         need to expand the argument again.  This way, we will not perform
         side-effects more the once.  */
      CALL_EXPR_ARG (exp, 0) = arg = builtin_save_expr (arg);

      op0 = expand_expr (arg, NULL, VOIDmode, EXPAND_NORMAL);

      start_sequence ();

      if (expand_sfix_optab (result, op0, builtin_optab))
        {
          /* Output the entire sequence.  */
          insns = get_insns ();
          end_sequence ();
          emit_insn (insns);
          return result;
        }

I think if the cast+round -> lround transformation is done it should be assumed
in that case that lround will not set errno.

Another approach would be to not do the transformation unless -fno-math-errno

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization
          Component|tree-optimization           |middle-end

--- Comment #1 from ktkachov at gcc dot gnu.org ---
This particular example is given for aarch64 but I imagine it could occur for
any other target.

>From what I understand lround can potentially set errno on a domain error
whereas round is valid everywhere and the cast to long int could be undefined
behaviour if the double is not valid, but undefined behaviour is not the same
as setting errno...

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2014-07-23
     Ever confirmed|0                           |1

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Interestingly my manual page says

ERRORS
       See math_error(7) for information on how to determine whether an  error
       has occurred when calling these functions.

       The following errors can occur:

       Domain error: x is a NaN or infinite, or the rounded value is too large
              An invalid floating-point exception (FE_INVALID) is raised.

       These functions do not set errno.

But C99 specifies only that the functions may raise a 'range error' (as
opposed to a domain error).

That said - 'errno' handling is a tricky area...

Confirmed.  The fix is to properly guard the transform, calling a function
that the may set errno is wrong-code.

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |ktkachov at gcc dot gnu.org

--- Comment #3 from ktkachov at gcc dot gnu.org ---
> Confirmed.  The fix is to properly guard the transform, calling a function
> that the may set errno is wrong-code.

I can spin a patch for that.

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

--- Comment #4 from ktkachov at gcc dot gnu.org ---
Author: ktkachov
Date: Thu Jul 24 13:23:05 2014
New Revision: 212989

URL: https://gcc.gnu.org/viewcvs?rev=212989&root=gcc&view=rev
Log:
PR 61876: Do not convert cast + __builtin_round into __builtin_lround unless
-fno-math-errno is used.

    PR middle-end/61876
    * convert.c (convert_to_integer): Do not convert BUILT_IN_ROUND and cast
    when flag_errno_math is on.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/convert.c

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

ktkachov at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED

--- Comment #5 from ktkachov at gcc dot gnu.org ---
I think something similar should be done for the lrint case.
rint is defined everywhere whereas lrint can throw domain errors on NaN, Inf

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

--- Comment #6 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Tue, 22 Jul 2014, ktkachov at gcc dot gnu.org wrote:

> From what I understand lround can potentially set errno on a domain error
> whereas round is valid everywhere and the cast to long int could be undefined
> behaviour if the double is not valid, but undefined behaviour is not the same
> as setting errno...

Under Annex F the cast isn't undefined behavior but raising the "invalid" 
exception and returning an unspecified value (which must be a valid value 
of type long, i.e. the program must behave as if each execution of the 
cast in the abstract machine has some particular value of type long it 
returns).

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

--- Comment #7 from ktkachov at gcc dot gnu.org ---
(In reply to joseph@codesourcery.com from comment #6)
> On Tue, 22 Jul 2014, ktkachov at gcc dot gnu.org wrote:
> 
> > From what I understand lround can potentially set errno on a domain error
> > whereas round is valid everywhere and the cast to long int could be undefined
> > behaviour if the double is not valid, but undefined behaviour is not the same
> > as setting errno...
> 
> Under Annex F the cast isn't undefined behavior but raising the "invalid" 
> exception and returning an unspecified value (which must be a valid value 
> of type long, i.e. the program must behave as if each execution of the 
> cast in the abstract machine has some particular value of type long it 
> returns).

In my copy at 6.3.1.4 on Real floating and integer conversions it says:
If the value of the integral part cannot be represented by the integer type,
the behavior is undefined.


In any case, it seems to me the transformation of cast+round -> lround is only
valid when:
- the result is within the range of the long int
- the argument is not NaN or infinity.

In convert.c this transformation is already guarded by the type precision so we
got the first point covered, but I think we also have to guard it by
-ffinite-math-only.

Is that reasonable?

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[mpolacek at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mpolacek at gcc dot gnu.org

--- Comment #8 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
(In reply to ktkachov from comment #7)
> (In reply to joseph@codesourcery.com from comment #6)
> > Under Annex F the cast isn't undefined behavior but raising the "invalid" 
> > exception and returning an unspecified value (which must be a valid value 
> > of type long, i.e. the program must behave as if each execution of the 
> > cast in the abstract machine has some particular value of type long it 
> > returns).
> 
> In my copy at 6.3.1.4 on Real floating and integer conversions it says:
> If the value of the integral part cannot be represented by the integer type,
> the behavior is undefined.

Making Annex F explicitly take precedence over the generic text is DR#442.

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[joseph at codesourcery dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

--- Comment #9 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
On Fri, 25 Jul 2014, ktkachov at gcc dot gnu.org wrote:

> In any case, it seems to me the transformation of cast+round -> lround is only
> valid when:
> - the result is within the range of the long int
> - the argument is not NaN or infinity.
> 
> In convert.c this transformation is already guarded by the type precision so we
> got the first point covered, but I think we also have to guard it by
> -ffinite-math-only.
> 
> Is that reasonable?

The precision test in convert.c is about conversions to types narrower 
than long; it's purely about precisions of integer types and has nothing 
to do with the first point (which would be about exponent range).

The transformation should be conditional on -fno-math-errno.  If in future 
DTS 18661-3 support is implemented, exponent range checks would be 
relevant to being able to determine that a conversion from cast + roundf16 
to lroundf16 is safe with -ffinite-math-only (because of the limited 
exponent range of _Float16), but there are many other things that would be 
needed for proper implementation of DTS 18661-3 (making all the relevant 
builtins generic across a wider range of floating-point types).

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

--- Comment #10 from ktkachov at gcc dot gnu.org ---
Author: ktkachov
Date: Tue Aug  5 09:52:21 2014
New Revision: 213628

URL: https://gcc.gnu.org/viewcvs?rev=213628&root=gcc&view=rev
Log:
[convert.c] PR 61876: Guard transformation to lrint by -fno-math-errno.

    * convert.c (convert_to_integer): Guard transformation to lrint by
    -fno-math-errno.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/convert.c

[Bug middle-end/61876] Converting __builtin_round + cast into __builtin_lround is not always equivalent in regards to math errno

[ktkachov at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=61876

ktkachov at gcc dot gnu.org changed:

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

--- Comment #11 from ktkachov at gcc dot gnu.org ---
The lround and lrint cases are fixed on trunk.