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