| Message ID | 20170207184635.GA64694@troutmask.apl.washington.edu |
|---|---|
| State | New |
| Headers | show |
Le 07/02/2017 à 19:46, Steve Kargl a écrit : > All, > > The attach patch maps REAL128 from iso_fortran_env to > the REAL kind type with widest precision. Prior to > this patch, REAL128 is mapped to the first REAL kind > type with a matching storage size of 128 bits. On > x86_64-*-freebsd both REAL(10) and REAL(16) occupy > 16 bytes, but REAL(10) is mapped to REAL128. This > may surprise users that assume REAL128 is mapped to > an IEEE754-2008 128-bit floating point entity with > 113 bits of precision. OK to commit? > Yes, thanks. Mikael
Hi Steve, I see Mikael has okayed the patch, but I did not have any time to comment prior. I wanted to note that: - the choice was deliberate, as the standard allows us to choose which real kind REAL128 corresponds to when there are several matching choices. The idea behind the current choice was to avoid forcing the use of slower soft-float arithmetic when a hardware type existed. I don’t have a strong opinion myself on the issue. - This is a breakage of the ABI, so we want to write it in the release notes, and in the doc about ISO_FORTRAN_ENV. - Is this appropriate for stage 4? FX
On Wed, Feb 08, 2017 at 10:25:22AM +0100, FX wrote: > Hi Steve, > > I see Mikael has okayed the patch, but I did not have any > time to comment prior. I wanted to note that: > > - the choice was deliberate, as the standard allows us to > choose which real kind REAL128 corresponds to when there are > several matching choices. The idea behind the current choice > was to avoid forcing the use of slower soft-float arithmetic > when a hardware type existed. I don’t have a strong opinion > myself on the issue. Yes, I considered that REAL128 would now be a software implementation. However, as an individual who works on FreeBSD, I found it rather unpleasing for i686-*-freebsd to map REAL128 to REAL(16) and x86_64-*-freebsd to map it to REAL(10). I also considered that some (many?, most?) users who don't look under the hood may expect REAL128 to map to a IEEE754 128-bit floating point entity where it has 113 bits of precision. > - This is a breakage of the ABI, so we want to write it in the > release notes, and in the doc about ISO_FORTRAN_ENV. The 6.3 manual has REAL32, REAL64, REAL128: Kind type parameters to specify a REAL type with a storage size of 32, 64, and 128 bits. It is negative if a target platform does not support the particular kind. (Fortran 2008 or later.) My patch does not change the above. I'll also note that with my patch REAL128 does not change with -m96bit-long-double, -mlong-double-64, or -mlong-double-80. So, the ABI is now stable. On my x86_64-*-freebsd system, gfortran6 does not include my unpatched and gfc7 is patches. % cat a.f90 program foo use iso_fortran_env REAL(REAL128) x print '(3(I0,1X))', kind(x), digits(x), precision(x) end program foo % gfortran6 -static -o z a.f90 && ./z 10 64 18 % gfortran6 -static -o z a.f90 -m96bit-long-double && ./z 16 113 33 % gfc7 -static -o z a.f90 && ./z 16 113 33 % gfc7 -static -o z a.f90 -m96bit-long-double && ./z 16 113 33 > - Is this appropriate for stage 4? I think it is appropriate as we already broke libgfortran for 7.0. I'll revert the patch as I can always maintain a local patches.
Le 08/02/2017 à 10:25, FX a écrit : > Hi Steve, > > I see Mikael has okayed the patch, but I did not have any time to comment prior. I wanted to note that: > > - the choice was deliberate, as the standard allows us to choose which real kind REAL128 corresponds to when there are several matching choices. The idea behind the current choice was to avoid forcing the use of slower soft-float arithmetic when a hardware type existed. I don’t have a strong opinion myself on the issue. Hello, I didn’t have this in mind when I OKed the patch. Still, I think the rule above goes against the principle of least surprise. Actually I don’t really see the use case for these constants. When would a user say; give me a real that big, including padding bits? > > - Is this appropriate for stage 4? > Now that you remind of it, if we take the rules to the letter, no. My opinion is that it should be accepted, in stage 4 or later. But I don’t have a strong opinion about it either. Mikael
On Wed, Feb 8, 2017 at 9:23 PM, Mikael Morin <morin-mikael@orange.fr> wrote: > Le 08/02/2017 à 10:25, FX a écrit : >> >> Hi Steve, >> >> I see Mikael has okayed the patch, but I did not have any time to comment >> prior. I wanted to note that: >> >> - the choice was deliberate, as the standard allows us to choose which >> real kind REAL128 corresponds to when there are several matching choices. >> The idea behind the current choice was to avoid forcing the use of slower >> soft-float arithmetic when a hardware type existed. I don’t have a strong >> opinion myself on the issue. > > Hello, I didn’t have this in mind when I OKed the patch. > Still, I think the rule above goes against the principle of least surprise. > Actually I don’t really see the use case for these constants. > When would a user say; give me a real that big, including padding bits? IMO, the current behavior is confusing and unportable, users shouldn't need to know target ABI details to understand why the compiler chooses the kind it does. FWIW wikipedia gets it wrong too, see https://en.wikipedia.org/wiki/Quadruple-precision_floating-point_format . So I think Steve's patch is a clear improvement. That being said, I think even with Steve's patch, it's not guaranteed to give you IEEE 754-2008 binary128. E.g. on IBM POWER targets, depending on the ABI you may get an IBM extended double (double-double or __ibm128) format. Although if IEEE binary128 is also available, with Steve's patch you should get that one has it has more precision that __ibm128. > >> >> - Is this appropriate for stage 4? >> > Now that you remind of it, if we take the rules to the letter, no. > My opinion is that it should be accepted, in stage 4 or later. > But I don’t have a strong opinion about it either. I think it's Ok, the patch is quite small and unlikely to cause regressions. In particular since the ABI has already been bumped, now is a good time to piggyback other ABI changing stuff. Steve, please don't revert, but add a note to gcc-7/changes.html and the GFortran wiki.
Index: gcc/fortran/trans-types.c =================================================================== --- gcc/fortran/trans-types.c (revision 245068) +++ gcc/fortran/trans-types.c (working copy) @@ -234,27 +234,42 @@ gfc_get_int_kind_from_width_isofortranen return -1; } -/* Get the kind number corresponding to a real of given storage size, - following the required return values for ISO_FORTRAN_ENV REAL* constants: - -2 is returned if we support a kind of larger size, -1 otherwise. */ + +/* Get the kind number corresponding to a real of a given storage size. + If two real's have the same storage size, then choose the real with + the largest precision. If a kind type is unavailable and a real + exists with wider storage, then return -2; otherwise, return -1. */ + int gfc_get_real_kind_from_width_isofortranenv (int size) { - int i; + int digits, i, kind; size /= 8; + kind = -1; + digits = 0; + /* Look for a kind with matching storage size. */ for (i = 0; gfc_real_kinds[i].kind != 0; i++) if (int_size_in_bytes (gfc_get_real_type (gfc_real_kinds[i].kind)) == size) - return gfc_real_kinds[i].kind; + { + if (gfc_real_kinds[i].digits > digits) + { + digits = gfc_real_kinds[i].digits; + kind = gfc_real_kinds[i].kind; + } + } + + if (kind != -1) + return kind; /* Look for a kind with larger storage size. */ for (i = 0; gfc_real_kinds[i].kind != 0; i++) if (int_size_in_bytes (gfc_get_real_type (gfc_real_kinds[i].kind)) > size) - return -2; + kind = -2; - return -1; + return kind; }