>>>>> Tom Callaway >>>>> on Fri, 13 Dec 2019 11:06:25 -0500 writes:> An excellent question. It is important to remember two key > facts: > 1. With gcc on ppc64, long doubles exist, they can > be used, just not safely as constants (and maybe they > still can be used safely under specific conditions?). > 2. I am not an expert in either PowerPC64 or gcc. :) > Looking at connections.c, we have (in order): > * handling long double as a valid case in a switch statement checking size > * adding long double as a field in the u union define > * handling long double as a valid case in a switch statement checking size > * handling long double as a valid case in a switch statement checking size > * memcpy from the address of a long double > In format.c, we have (in order): > * conditionally creating private_nearbyintl for R_nearbyintl > * defining a static const long double tbl[] > * use exact scaling factor in long double precision > For most of these, it seems safe to leave them as is for ppc64. I would > have thought that the gcc compiler might have had issue with: > connections.c: > static long double ld1; > for (i = 0, j = 0; i < len; i++, j += size) { > ld1 = (long double) REAL(object)[i]; > format.c: > static const long double tbl[] > ... but it doesn't. Perhaps the original code at issue: > arithmetic.c: > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; > only makes gcc unhappy because of the very large value trying to be stored > in the static long double, which would make it span the "folded double" on > that architecture. > ***** > It seems that the options are: > A) Patch the one place where the compiler determines it is not safe to use > a static long double on ppc64. > B) Treat PPC64 as a platform where it is never safe to use a static long > double > FWIW, I did run the test suite after applying my patch and all of the tests > pass on ppc64. > Tom Thank you, Tom. You were right... and only A) is needed. In the mean time I've also been CC'ed in a corresponding debian bug report on the exact same architecture. In the end, after explanation and recommendation by Tomas Kalibera, I've committed a slightly better change to R's sources, both in the R-devel (trunk) and the "R-3.6.x patched" branch: Via a macro, it continues to use long double also for the PPC 64 in this case: $ svn diff -c77587 Index: src/main/arithmetic.c ==================================================================--- src/main/arithmetic.c (Revision 77586) +++ src/main/arithmetic.c (Revision 77587) @@ -176,8 +176,14 @@ #endif } + #if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) +# ifdef __PPC64__ + // PowerPC 64 (when gcc has -mlong-double-128) fails constant folding with LDOUBLE +# define q_1_eps (1 / LDBL_EPSILON) +# else static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; +# endif #else static double q_1_eps = 1 / DBL_EPSILON; #endif ------------- ------------- ------------- Thank you (and everybody else involved) once more, Martin
>>>>> Martin Maechler >>>>> on Tue, 17 Dec 2019 11:25:31 +0100 writes:>>>>> Tom Callaway >>>>> on Fri, 13 Dec 2019 11:06:25 -0500 writes:>> An excellent question. It is important to remember two key >> facts: >> 1. With gcc on ppc64, long doubles exist, they can >> be used, just not safely as constants (and maybe they >> still can be used safely under specific conditions?). >> 2. I am not an expert in either PowerPC64 or gcc. :) >> Looking at connections.c, we have (in order): >> * handling long double as a valid case in a switch statement checking size >> * adding long double as a field in the u union define >> * handling long double as a valid case in a switch statement checking size >> * handling long double as a valid case in a switch statement checking size >> * memcpy from the address of a long double >> In format.c, we have (in order): >> * conditionally creating private_nearbyintl for R_nearbyintl >> * defining a static const long double tbl[] >> * use exact scaling factor in long double precision >> For most of these, it seems safe to leave them as is for ppc64. I would >> have thought that the gcc compiler might have had issue with: >> connections.c: >> static long double ld1; >> for (i = 0, j = 0; i < len; i++, j += size) { >> ld1 = (long double) REAL(object)[i]; >> format.c: >> static const long double tbl[] >> ... but it doesn't. Perhaps the original code at issue: >> arithmetic.c: >> static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; >> only makes gcc unhappy because of the very large value trying to be stored >> in the static long double, which would make it span the "folded double" on >> that architecture. >> ***** >> It seems that the options are: >> A) Patch the one place where the compiler determines it is not safe to use >> a static long double on ppc64. >> B) Treat PPC64 as a platform where it is never safe to use a static long >> double >> FWIW, I did run the test suite after applying my patch and all of the tests >> pass on ppc64. >> Tom > Thank you, Tom. > You were right... and only A) is needed. > In the mean time I've also been CC'ed in a corresponding debian > bug report on the exact same architecture. > In the end, after explanation and recommendation by Tomas > Kalibera, I've committed a slightly better change to R's > sources, both in the R-devel (trunk) and the "R-3.6.x patched" > branch: Via a macro, it continues to use long double also for > the PPC 64 in this case: > $ svn diff -c77587 > Index: src/main/arithmetic.c > ================================================================== > --- src/main/arithmetic.c (Revision 77586) > +++ src/main/arithmetic.c (Revision 77587) > @@ -176,8 +176,14 @@ > #endif > } > + > #if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) > +# ifdef __PPC64__ > + // PowerPC 64 (when gcc has -mlong-double-128) fails constant folding with LDOUBLE > +# define q_1_eps (1 / LDBL_EPSILON) > +# else > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; > +# endif > #else > static double q_1_eps = 1 / DBL_EPSILON; > #endif > ------------- ------------- ------------- Now, Debian Bug#946836 has been reopened, because __PPC64__ does not cover all powerpc architectures and in their build farm they found non-PPC64 cases which also needed to switch from a static variable to a macro, the suggestion being to replace __PPC64__ by __powerpc__ which is what I'm going to commit now (for R-3.6.x patched and for R-devel !). I hope that these macros are +- universally working and not too much depending on the exact compiler flavor. Martin Maechler ETH Zurich and R Core team
Do note that 3.6-patched will only be live for a day or two as we branch for 4.0.0 on Friday. Anything committed there is unlikely to make it into an official release (in principle, the 3.6 branch can be revived but it would take a very strong incentive to do so.) If you want an R-3.6.3-for-ppc, I think a vendor patch is the way. AFAIR (it's been more than a decade since I looked at this stuff) the RPM spec files make it fairly easy to apply changes to the sources before building. -pd> On 25 Mar 2020, at 10:00 , Martin Maechler <maechler at stat.math.ethz.ch> wrote: > >>>>>> Martin Maechler >>>>>> on Tue, 17 Dec 2019 11:25:31 +0100 writes: > >>>>>> Tom Callaway >>>>>> on Fri, 13 Dec 2019 11:06:25 -0500 writes: > >>> An excellent question. It is important to remember two key >>> facts: > >>> 1. With gcc on ppc64, long doubles exist, they can >>> be used, just not safely as constants (and maybe they >>> still can be used safely under specific conditions?). > >>> 2. I am not an expert in either PowerPC64 or gcc. :) > >>> Looking at connections.c, we have (in order): >>> * handling long double as a valid case in a switch statement checking size >>> * adding long double as a field in the u union define >>> * handling long double as a valid case in a switch statement checking size >>> * handling long double as a valid case in a switch statement checking size >>> * memcpy from the address of a long double > >>> In format.c, we have (in order): >>> * conditionally creating private_nearbyintl for R_nearbyintl >>> * defining a static const long double tbl[] >>> * use exact scaling factor in long double precision > >>> For most of these, it seems safe to leave them as is for ppc64. I would >>> have thought that the gcc compiler might have had issue with: > >>> connections.c: >>> static long double ld1; >>> for (i = 0, j = 0; i < len; i++, j += size) { >>> ld1 = (long double) REAL(object)[i]; > >>> format.c: >>> static const long double tbl[] > >>> ... but it doesn't. Perhaps the original code at issue: > >>> arithmetic.c: >>> static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; > >>> only makes gcc unhappy because of the very large value trying to be stored >>> in the static long double, which would make it span the "folded double" on >>> that architecture. > >>> ***** > >>> It seems that the options are: > >>> A) Patch the one place where the compiler determines it is not safe to use >>> a static long double on ppc64. >>> B) Treat PPC64 as a platform where it is never safe to use a static long >>> double > >>> FWIW, I did run the test suite after applying my patch and all of the tests >>> pass on ppc64. > >>> Tom > >> Thank you, Tom. >> You were right... and only A) is needed. > >> In the mean time I've also been CC'ed in a corresponding debian >> bug report on the exact same architecture. > >> In the end, after explanation and recommendation by Tomas >> Kalibera, I've committed a slightly better change to R's >> sources, both in the R-devel (trunk) and the "R-3.6.x patched" >> branch: Via a macro, it continues to use long double also for >> the PPC 64 in this case: > >> $ svn diff -c77587 >> Index: src/main/arithmetic.c >> ==================================================================>> --- src/main/arithmetic.c (Revision 77586) >> +++ src/main/arithmetic.c (Revision 77587) >> @@ -176,8 +176,14 @@ >> #endif >> } > >> + >> #if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) >> +# ifdef __PPC64__ >> + // PowerPC 64 (when gcc has -mlong-double-128) fails constant folding with LDOUBLE >> +# define q_1_eps (1 / LDBL_EPSILON) >> +# else >> static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; >> +# endif >> #else >> static double q_1_eps = 1 / DBL_EPSILON; >> #endif > >> ------------- ------------- ------------- > > Now, Debian Bug#946836 has been reopened, > because __PPC64__ does not cover all powerpc architectures > and in their build farm they found non-PPC64 cases which also > needed to switch from a static variable to a macro, > > the suggestion being to replace __PPC64__ by __powerpc__ > > which is what I'm going to commit now (for R-3.6.x patched and > for R-devel !). > I hope that these macros are +- universally working and not too > much depending on the exact compiler flavor. > > Martin Maechler > ETH Zurich and R Core team > > ______________________________________________ > R-devel at r-project.org mailing list > https://stat.ethz.ch/mailman/listinfo/r-devel-- Peter Dalgaard, Professor, Center for Statistics, Copenhagen Business School Solbjerg Plads 3, 2000 Frederiksberg, Denmark Phone: (+45)38153501 Office: A 4.23 Email: pd.mes at cbs.dk Priv: PDalgd at gmail.com