Build failure on powerpc64

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Build failure on powerpc64

Tom "spot" Callaway
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

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Martin Maechler
>>>>> 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
______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Tom "spot" Callaway
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 <[hidden email]>
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]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Serguei Sokol
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.

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Tom "spot" Callaway
No, that does not change the issue:

arithmetic.c:180:26: error: initializer element is not constant
  180 | static LDOUBLE q_1_eps = 1.L / LDBL_EPSILON;

Tom

On Fri, Dec 13, 2019 at 11:56 AM Serguei Sokol <[hidden email]>
wrote:

> 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.
>
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Martin Maechler
In reply to this post by Tom "spot" Callaway
>>>>> 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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Martin Maechler
>>>>> 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

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Peter Dalgaard-2
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 <[hidden email]> 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
>
> ______________________________________________
> [hidden email] 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: [hidden email]  Priv: [hidden email]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel
Reply | Threaded
Open this post in threaded view
|

Re: Build failure on powerpc64

Tom "spot" Callaway
This change seems correct, but as we don't have any non-PPC64 systems or
build targets in the Fedora buildsystem, it won't affect Fedora if it
doesn't make it.

Tom


On Wed, Mar 25, 2020, 6:16 AM peter dalgaard <[hidden email]> wrote:

> 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 <[hidden email]>
> 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
> >
> > ______________________________________________
> > [hidden email] 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: [hidden email]  Priv: [hidden email]
>
>
>
>
>
>
>
>
>
>

        [[alternative HTML version deleted]]

______________________________________________
[hidden email] mailing list
https://stat.ethz.ch/mailman/listinfo/r-devel