>>>>> Tom Callaway >>>>> on Thu, 12 Dec 2019 14:21:10 -0500 writes:> Hi R folks, > Went to build R 3.6.2 for Fedora/EPEL and got failures across the board. > Disabling the test suite for all non-intel architectures resolves most of > the failures, but powerpc64 dies in the compiler, specifically here: > gcc -m64 -I../../src/extra/xdr -I. -I../../src/include -I../../src/include > -I/usr/local/include -I../../src/nmath -DHAVE_CONFIG_H -fopenmp -fPIC > -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 > -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong > -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8 > -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection -c > arithmetic.c -o arithmetic.o > arithmetic.c:180:26: error: initializer element is not constant > 180 | static LDOUBLE q_1_eps = (1 / LDBL_EPSILON); > | ^ > make[3]: *** [../../Makeconf:124: arithmetic.o] Error 1 > Took me a bit to figure out why, but this is happening because on > powerpc64, gcc is compiled with -mlong-double-128, and the long double > format used on PPC is IBM's 128bit long double which is two doubles added > together. As a result, gcc can't safely do const assignments to long > doubles on ppc64, so it dies there. > The fix is easy enough, do not try to assign a value to a static long > double on ppc64. > --- ./src/main/arithmetic.c.orig 2019-12-12 18:30:12.416334062 +0000 > +++ ./src/main/arithmetic.c 2019-12-12 18:30:44.966334062 +0000 > @@ -179,7 +179,10 @@ void attribute_hidden InitArithmetic() > #endif > } > -#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) > +/* PowerPC 64 (when gcc has -mlong-double-128) breaks here because > + * of issues constant folding 128bit IBM long doubles. > + */ > +#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) && !__PPC64__ > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; > #else > static double q_1_eps = 1 / DBL_EPSILON; > Hope that helps someone else. > Tom Thank you, Tom. That is "interesting" ... The piece in question had been added by me, ------------------------------------------------------------------------ r77193 | maechler | 2019-09-18 13:21:49 +0200 (Wed, 18 Sep 2019) | 1 line x %% +/- Inf now typically return non-NaN; fix the "FIXME" in C ------------------------------------------------------------------------ in order to use double precision in order to deal with finite cases close to Inf, etc. IIUC, your proposed patch is really a workaround a bug on PPC64 ? But note the check on LONG_DOUBLE is not the only one in R's sources: Via 'grep' in src/main/*.? I also see connections.c4285:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) connections.c4329:#if HAVE_LONG_DOUBLE connections.c4379:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) connections.c4514:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) connections.c4592:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) format.c250:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) format.c285:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) format.c339:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) Do they also need protection against this PPC64 bug ? Best, Martin Maechler ETH Zurich and R Core Team
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 On Fri, Dec 13, 2019 at 5:44 AM Martin Maechler <maechler at stat.math.ethz.ch> wrote:> >>>>> Tom Callaway > >>>>> on Thu, 12 Dec 2019 14:21:10 -0500 writes: > > > Hi R folks, > > > Went to build R 3.6.2 for Fedora/EPEL and got failures across the > board. > > > Disabling the test suite for all non-intel architectures resolves > most of > > the failures, but powerpc64 dies in the compiler, specifically here: > > > gcc -m64 -I../../src/extra/xdr -I. -I../../src/include > -I../../src/include > > -I/usr/local/include -I../../src/nmath -DHAVE_CONFIG_H -fopenmp > -fPIC > > -O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 > > -Wp,-D_GLIBCXX_ASSERTIONS -fexceptions -fstack-protector-strong > > -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 > > -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mcpu=power8 > > -mtune=power8 -fasynchronous-unwind-tables -fstack-clash-protection > -c > > arithmetic.c -o arithmetic.o > > arithmetic.c:180:26: error: initializer element is not constant > > 180 | static LDOUBLE q_1_eps = (1 / LDBL_EPSILON); > > | ^ > > make[3]: *** [../../Makeconf:124: arithmetic.o] Error 1 > > > Took me a bit to figure out why, but this is happening because on > > powerpc64, gcc is compiled with -mlong-double-128, and the long > double > > format used on PPC is IBM's 128bit long double which is two doubles > added > > together. As a result, gcc can't safely do const assignments to long > > doubles on ppc64, so it dies there. > > > The fix is easy enough, do not try to assign a value to a static long > > double on ppc64. > > --- ./src/main/arithmetic.c.orig 2019-12-12 > 18:30:12.416334062 +0000 > > +++ ./src/main/arithmetic.c 2019-12-12 18:30:44.966334062 +0000 > > @@ -179,7 +179,10 @@ void attribute_hidden InitArithmetic() > > #endif > > } > > > -#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) > > +/* PowerPC 64 (when gcc has -mlong-double-128) breaks here because > > + * of issues constant folding 128bit IBM long doubles. > > + */ > > +#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > SIZEOF_DOUBLE) && > !__PPC64__ > > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON; > > #else > > static double q_1_eps = 1 / DBL_EPSILON; > > > > Hope that helps someone else. > > Tom > > Thank you, Tom. That is "interesting" ... > > The piece in question had been added by me, > ------------------------------------------------------------------------ > r77193 | maechler | 2019-09-18 13:21:49 +0200 (Wed, 18 Sep 2019) | 1 line > > x %% +/- Inf now typically return non-NaN; fix the "FIXME" in C > ------------------------------------------------------------------------ > in order to use double precision in order to deal with finite cases > close to Inf, etc. > > IIUC, your proposed patch is really a workaround a bug on PPC64 ? > > But note the check on LONG_DOUBLE is not the only one in R's > sources: Via 'grep' in src/main/*.? I also see > > connections.c 4285:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > connections.c 4329:#if HAVE_LONG_DOUBLE > connections.c 4379:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > connections.c 4514:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > connections.c 4592:#if HAVE_LONG_DOUBLE && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > format.c 250:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > format.c 285:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > format.c 339:#if defined(HAVE_LONG_DOUBLE) && (SIZEOF_LONG_DOUBLE > > SIZEOF_DOUBLE) > > Do they also need protection against this PPC64 bug ? > > Best, > > Martin Maechler > ETH Zurich and R Core Team >[[alternative HTML version deleted]]
Le 13/12/2019 ? 17:06, Tom Callaway a ?crit?:> arithmetic.c: > static LDOUBLE q_1_eps = 1 / LDBL_EPSILON;Just a thought: can it be that it's "1" which is at the origin of compiler complaint? In this case, would the syntax "1.L" be sufficient to keep it calm? Serguei.
>>>>> 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