[Bug libfortran/48615] New: Invalid UP/DOWN rounding with E and ES descriptors

[Bug libfortran/48615] New: Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

           Summary: Invalid UP/DOWN rounding with E and ES descriptors
           Product: gcc
           Version: 4.7.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libfortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: thenlich@users.sourceforge.net


The E and ES descriptors ignore the I/O rounding mode:

print "(RU,F17.0)", 1.1
print "(RU,G17.1)", 1.1e9
print "(RU,E17.1)", 1.1e9 ! 0.1E+10 expected 0.2E+10
print "(RU,ES17.0)", 1.1e9  ! 1.E+09 expected 2.E+09
print "(RU,EN17.0)", 1.1e9

print "(RD,F17.0)", 1.9
print "(RD,G17.1)", 1.9e9
print "(RD,E17.1)", 1.9e9   ! 0.2E+10 expected 0.1E+10
print "(RD,ES17.0)", 1.9e9  ! 2.E+09 expected 1.E+09
print "(RD,EN17.0)", 1.9e9
end

Fortran 2008:
10.7.2.3.3 E and D editing
4 ...
- x1 x2 ... xd are the d most significant digits of the internal value after
rounding (10.7.2.3.7);

10.7.2.3.5 ES editing
5 ...
- y is a decimal digit representative of the most significant digit of the
internal value after rounding (10.7.2.3.7);
- signifies a decimal symbol (10.6);
- x1 x2 ... xd are the d next most significant digits of the internal value
after rounding;

10.7.2.3.7 I/O rounding mode
3 When the I/O rounding mode is UP, the value resulting from conversion shall
be the smallest representable value that is greater than or equal to the
original value. When the I/O rounding mode is DOWN, the value resulting
from conversion shall be the largest representable value that is less than or
equal to the original value.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

--- Comment #1 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-23 07:49:33 UTC ---
I think I found the problematic code in write_float:

  if (f->format == FMT_F || f->format == FMT_EN || f->format == FMT_G 
      || ((f->format == FMT_D || f->format == FMT_E)
      && dtp->u.p.scale_factor != 0))
    {
      /* Always convert at full precision to avoid double rounding.  */
      ndigits = MIN_FIELD_WIDTH - 4 - edigits;
    }
  else
    {
      /* The number of digits is known, so let printf do the rounding.  */
      if (f->format == FMT_ES)
    ndigits = f->u.real.d + 1;
      else
    ndigits = f->u.real.d;
      if (ndigits > MIN_FIELD_WIDTH - 4 - edigits)
    ndigits = MIN_FIELD_WIDTH - 4 - edigits;
    }

"let printf do the rounding" means the result will always be rounded according
to the rounding rules of printf, which is for GLIBC: "If the value being
printed cannot be expressed accurately in the specified number of digits, the
value is rounded to the nearest number that fits."

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

--- Comment #2 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-23 11:06:28 UTC ---
The second branch (let printf do rounding) should only be choosen if the
requested rounding mode is the same as printf uses (nearest/compatible?) and
either
- the format is ES or
- the format is D, E or G; and the scale factor is 0.

I am unsure about the last requirement (scale_factor==0); isn't the number of
digits known even if the scale factor is != 0?

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

Jerry DeLisle <jvdelisle at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2011.04.23 12:23:32
     Ever Confirmed|0                           |1

--- Comment #3 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-23 12:23:32 UTC ---
Just to confirm, is this the full expected output?

$ gfc pr48615.f08 
$ ./a.out 
               2.
          0.2E+10
          0.2E+10
           2.E+09
           2.E+09
               1.
          0.1E+10
          0.1E+10
           1.E+09
           1.E+09

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

--- Comment #4 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-23 13:19:46 UTC ---
Created attachment 24081
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24081
Revised test case

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

Jerry DeLisle <jvdelisle at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |jvdelisle at gcc dot
                   |gnu.org                     |gnu.org

--- Comment #5 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-23 16:32:16 UTC ---
Created attachment 24082
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24082
Preliminary patch

Here is just a experimental patch. Yes we could do some things in output float.
 I will think a bit on that.  The patch also has adjustments to default widths.
The default widths require quite a few test suite adjustments, which I have in
my tree and will post later to pr48488.

I believe the intent of this code block in write_float was to reduce the number
of digits converted in the later snprintf calls as a simple optimization.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

Thomas Henlich <thenlich at users dot sourceforge.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #24081|0                           |1
        is obsolete|                            |

--- Comment #6 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-23 17:04:14 UTC ---
Created attachment 24083
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=24083
Yet another revised test.

Found more bugs, even with COMPATIBLE rounding.

Tested with the equation.com build (MinGW). Please report if results on other
platforms differ.

Results obtained are in the comments.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

--- Comment #7 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-24 20:33:17 UTC ---
Patch submitted to gfortran list for review.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

--- Comment #8 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-24 21:41:16 UTC ---
I don't have access to a build system until Tuesday, so I couldn't test your
patch. But I'm not sure I understand what you are trying to do.

I see that you added one more digit in the output. I am not convinced that
adding one digit will solve the problem of rounding. It will for my first test
case, because it works for the values 1.1 and 1.9. But will it work for values
like 1.00001 or 1.99999? With your patch, they will be rounded to 1.0 (or 2.0)
by printf so libfortran has no way of knowing that it should round up (or
down).

And we recently set the maximum output width to the minimum value required by
IEEE 754-2008. Increasing that further (as your patch does) will only serve in
reducing the maximum rounding error from 1/1000 to 1/10000 (but does not
contribute to fixing this bug)

Can you confirm that the patch fixes all the testcases in attachment 24083?

I still think my comment #2 still applies: We can only use this "shortcut"
rounding if the requested rounding mode equals that of printf, which I found to
be NEAREST with __mingw_printf and COMPATIBLE with MSVC printf. Others, e.g.
glibc probably have one of these modes, but I couldn't yet find out if this
feature is documented at all.

Re: [Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[Jerry DeLisle]

On 04/24/2011 02:41 PM, thenlich at users dot sourceforge.net wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48615
>
> --- Comment #8 from Thomas Henlich<thenlich at users dot sourceforge.net>  2011-04-24 21:41:16 UTC ---
> I don't have access to a build system until Tuesday, so I couldn't test your
> patch. But I'm not sure I understand what you are trying to do.

Please read my post on the gfortran mailing list. There are four bugs addressed 
by this patch.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at frontier dot com]

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

--- Comment #9 from jvdelisle at frontier dot com 2011-04-24 23:24:53 UTC ---
On 04/24/2011 02:41 PM, thenlich at users dot sourceforge.net wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48615
>
> --- Comment #8 from Thomas Henlich<thenlich at users dot sourceforge.net>  2011-04-24 21:41:16 UTC ---
> I don't have access to a build system until Tuesday, so I couldn't test your
> patch. But I'm not sure I understand what you are trying to do.

Please read my post on the gfortran mailing list. There are four bugs addressed 
by this patch.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

--- Comment #10 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-24 23:43:34 UTC ---
Regarding comment #2.  The patch completely eliminates this code. I did not
feel confident that it is safe enough for all platforms.  My approach is to
first get the output correct, then we can think about optimizations later. A
final fix for PR48602 will be second phase of this patch work. After that is
complete, then we can re-look at any optimizations.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at frontier dot com]

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

--- Comment #11 from jvdelisle at frontier dot com 2011-04-24 23:54:11 UTC ---
On 04/24/2011 02:41 PM, thenlich at users dot sourceforge.net wrote:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=48615
>
> --- Comment #8 from Thomas Henlich<thenlich at users dot sourceforge.net>  2011-04-24 21:41:16 UTC ---
--- snip ---
>
> Can you confirm that the patch fixes all the testcases in attachment 24083?
>

The test case of attachment 24083 is included in the patch as round_3.f08 and 
yes it passes.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[thenlich at users dot sourceforge.net]

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

--- Comment #12 from Thomas Henlich <thenlich at users dot sourceforge.net> 2011-04-25 08:43:32 UTC ---
The patch submitted to the list is different to the one I based my comments on.
My mistake.

Good work.

With the current method of letting printf do the conversion, there seems to be
no better way than always converting the full 40 digits, and then throwing away
most of them.

In Bug 48511 David Gay's dtoa implementation was mentioned. I looked into that
and I think it can be modified to be fully Fortran standard compliant. It
combines digit generation and rounding in one step and is probably
significantly more efficient than our current solution. Will investigate
further.

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

--- Comment #13 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-29 14:56:08 UTC ---
Author: jvdelisle
Date: Fri Apr 29 14:56:02 2011
New Revision: 173166

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=173166
Log:
2011-04-29  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
        Janne Blomqvist  <jb@gcc.gnu.org>

    PR libgfortran/48488
    PR libgfortran/48602
    PR libgfortran/48615
    PR libgfortran/48684
    PR libgfortran/48787
    * io/write.c (write_d, write_e, write_f, write_en,
    write_es): Add precision compemsation parameter to call.
    (set_fnode_default): Adjust default widths to assure
    round trip on write and read. (write_real): Adjust call to write_float.
    (write_real_g0): Calculate compensation for extra precision and adjust
    call to write_float. 
    * io/write_float.def (output_float_FMT_G_): Use volatile rather than
    asm volatile to avoid optimization issue. Correctly calculate the
    number of blanks (nb) to be appended and simplify calculation logic.
    (write_float): Increase MIN_FIELD_WIDTH by one to accomodate the new
    default widths. Eliminate the code that attempted to reduce the
    the precision used in later sprintf functions.  Add call parameter to
    compensate for extra precision.

Modified:
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/io/write.c
    trunk/libgfortran/io/write_float.def

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

--- Comment #14 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-04-29 15:09:03 UTC ---
Author: jvdelisle
Date: Fri Apr 29 15:08:57 2011
New Revision: 173168

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=173168
Log:
2011-04-29  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

    PR libgfortran/48488
    PR libgfortran/48602
    PR libgfortran/48615
    PR libgfortran/48684
    PR libgfortran/48787
    * gfortran.dg/fmt_g.f: Adjust test.
    * gfortran.dg/fmt_g0_1.f08: Adjust test.
    * gfortran.dg/round_3.f08: New test.
    * gfortran.dg/namelist_print_1.f: Adjust test.
    * gfortran.dg/char4_iunit_1.f03: Adjust test.
    * gfortran.dg/f2003_io_5.f03: Adjust test.
    * gfortran.dg/coarray_15.f90: Adjust test.
    * gfortran.dg/namelist_65.f90: Adjust test.
    * gfortran.dg/fmt_cache_1.f: Adjust test.
    * gfortran.dg/char4_iunit_2.f03: Adjust test.
    * gfortran.dg/real_const_3.f90: Adjust test.

Added:
    trunk/gcc/testsuite/gfortran.dg/round_3.f08
Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/char4_iunit_1.f03
    trunk/gcc/testsuite/gfortran.dg/char4_iunit_2.f03
    trunk/gcc/testsuite/gfortran.dg/coarray_15.f90
    trunk/gcc/testsuite/gfortran.dg/f2003_io_5.f03
    trunk/gcc/testsuite/gfortran.dg/fmt_cache_1.f
    trunk/gcc/testsuite/gfortran.dg/fmt_g.f
    trunk/gcc/testsuite/gfortran.dg/fmt_g0_1.f08
    trunk/gcc/testsuite/gfortran.dg/namelist_65.f90
    trunk/gcc/testsuite/gfortran.dg/namelist_print_1.f
    trunk/gcc/testsuite/gfortran.dg/real_const_3.f90

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[jvdelisle at gcc dot gnu.org]

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

--- Comment #15 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2011-05-06 20:06:42 UTC ---
Fixed on trunk. Holding for backport

[Bug libfortran/48615] Invalid UP/DOWN rounding with E and ES descriptors

[burnus at gcc dot gnu.org]

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

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
                 CC|                            |burnus at gcc dot gnu.org
         Resolution|                            |FIXED

--- Comment #16 from Tobias Burnus <burnus at gcc dot gnu.org> 2013-01-06 14:13:15 UTC ---
Close as FIXED (in the 4.7 trunk; hence, in the 4.7.0 release.)

The fix was committed on 2011-04-29 for the trunk (i.e. 4.7). As there was no
activity for almost two years and as it is no regression, I don't think we will
ever port it back to 4.6.