Openblas: openblas-0.3.10 build failure on dynamic zarch

[susilehtola]

openblas-0.3.10 fails to build in Fedora s390x with DYNAMIC_ARCH=1 DYNAMIC_OLDER=1. I get a link error

/usr/bin/ld: ../libRblas-r0.3.10.a(dynamic_zarch.o): in function `gotoblas_corename':
/builddir/build/BUILD/openblas-0.3.10/Rblas/driver/others/dynamic_zarch.c:70: undefined reference to `gotoblas_Z13'
/usr/bin/ld: ../libRblas-r0.3.10.a(dynamic_zarch.o): in function `force_coretype':
/builddir/build/BUILD/openblas-0.3.10/Rblas/driver/others/dynamic_zarch.c:129: undefined reference to `gotoblas_Z13'
/usr/bin/ld: ../libRblas-r0.3.10.a(dynamic_zarch.o): in function `gotoblas_corename':
/builddir/build/BUILD/openblas-0.3.10/Rblas/driver/others/dynamic_zarch.c:70: undefined reference to `gotoblas_Z13'
/usr/bin/ld: ../libRblas-r0.3.10.a(dynamic_zarch.o): in function `force_coretype':
/builddir/build/BUILD/openblas-0.3.10/Rblas/driver/others/dynamic_zarch.c:129: undefined reference to `gotoblas_Z13'
collect2: error: ld returned 1 exit status

[martin-frbg]

Have not checked why this happens, but DYNAMIC_OLDER does not currently make sense on zarch (or anything non-x86) anway. Does it still fail when you leave out that option ?

[martin-frbg]

Does this fix it ? (dynamic_zarch.c from line 49)

 extern gotoblas_t gotoblas_ZARCH_GENERIC;
 #ifdef HAVE_Z13_SUPPORT
 extern gotoblas_t gotoblas_Z13;
+#else
+#define gotoblas_Z13 gotoblas_ZARCH_GENERIC
 #endif
 #ifdef HAVE_Z14_SUPPORT
 extern gotoblas_t gotoblas_Z14;
+#else
+#define gotoblas_Z14 gotoblas_ZARCH_GENERIC
 #endif

[susilehtola]

@martin-frbg I think the main question is why HAVE_Z13_SUPPORT would not be true?

[susilehtola]

I'm trying again with TARGET=ZARCH_GENERIC DYNAMIC_ARCH=1, build at https://koji.fedoraproject.org/koji/taskinfo?taskID=45783069

[martin-frbg]

8c33861 (part of @mhillenibm 's PR #2614) made its availability depend on os and compiler versions, guess this may not work if you set TARGET=Z13 at the same time (which was not clear from the above) and/or your RH/gcc setup is not among the "accepted" ones.
My proposed fix should make it fall back properly in either case (at least that is how it is done on x86_64) but I cannot test it here.

[mhillenibm]

So, my code in Makefile.system that identifies whether the present gcc version has support for z13 has a bug. @susilehtola Your build uses gcc 10 and thereby triggers the bug. My bad.

From your build log the message

OpenBLAS: Not building Z13 kernels because gcc is older than 5.2 or 6.x

... points to the Makefile assuming that gcc would be too old to support z13. As a result, it omits building the kernels with z13 optimizations. (https://kojipkgs.fedoraproject.org//work/tasks/3107/45783107/build.log)

In contrast, the detection mechanism in driver/others/dynamic_zarch.c correctly identifies that your gcc supports z13, and thus refers the respective symbols. Thus, the linker errors.

A quick workaround would override the detection in Makefile.system:

diff --git a/Makefile.system b/Makefile.system
index 8d78b420..c62d8f75 100644
--- a/Makefile.system
+++ b/Makefile.system
@@ -570,7 +570,7 @@ ifeq ($(ARCH), zarch)
 DYNAMIC_CORE = ZARCH_GENERIC

 # Z13 is supported since gcc-5.2, gcc-6, and in RHEL 7.3 and newer
-GCC_GE_52 := $(subst 0,,$(shell expr `$(CC) -dumpversion` \>= "5.2"))
+GCC_GE_52 := 1

The bug is the lexicographical comparison between the compiler version and "5.2". The corresponding check for Z14 a few lines down works alright, since it's doing an arithmetic comparison. (A less hacky workaround would replace the "5.2" with "6").

Working on a proper fix now...

[martin-frbg]

This should probably be unified with the section already containing various other gcc version tests in Makefile.system (there is already a "major version >4", and at least one test for a particular minor version IIRC)

[RajalakshmiSR]

@mhillenibm Can this help gcc --version | cut -f2 -d. | head -1 ..

[mhillenibm]

@RajalakshmiSR indeed, I reuse the variables in https://github.com/xianyi/OpenBLAS/blob/1bd3cd66c270134d138f7b61cd158407a07086cf/Makefile.system#L283 and below, as Martin suggested.

[RajalakshmiSR]

@mhillenibm I noticed that dumpversion was not showing minor version on older compilers..
For example,
$gcc -dumpversion
7
$ gcc --version
gcc (Ubuntu 7.4.0-1ubuntu1~18.04.1) 7.4.0

[mhillenibm]

@RajalakshmiSR the behavior of gcc -dumpversion changed with gcc-7.1. Now, we have -dumpfullversion which will always print the full version info, while the behavior of -dumpversion is configurable at compile time.

Since the minor version only matters for gcc versions 4 and 5, in the checks currently present, we are OK, for now. As a followup, we should use -dumpfullversion when detecting gcc-7 or newer.

[mhillenibm]

Posted #2669 to address this issue.

[susilehtola]

OK, #2670 fixes the build issue.

Unfortunately, the build still crashes, now in the tests:

*----------------------------*
| DGEMV thread safety tester |
*----------------------------*
Size of random matrices and vectors(N=M): 1024
Number of concurrent calls into OpenBLAS : 0
Number of testing rounds : 16
This test will need 0 MiB of RAM
Initializing random number generator...done
Preparing to test CBLAS DGEMV thread safety
Allocating matrices...done
Allocating vectors...done
Filling matrices with random numbers...make[1]: *** [Makefile:8: dgemv_tester] Aborted (core dumped)
make[1]: *** Deleting file 'dgemv_tester'
make[1]: Leaving directory '/builddir/build/BUILD/openblas-0.3.10/openmp64/cpp_thread_test'
make: Leaving directory '/builddir/build/BUILD/openblas-0.3.10/openmp64'
make: *** [Makefile:143: tests] Error 2
RPM build errors:

See log at https://kojipkgs.fedoraproject.org//work/tasks/8069/45798069/build.log

[susilehtola]

How is the number of concurrent calls into OpenBLAS 0?

[martin-frbg]

Seems I bungled the commit that made this test run on smaller machines at all, must have been some local testing that made me set the number of threads to "max available threads -4" instead
of max available. I bet your setup reports four OpenMP threads available, so far the test was meant to check thread safety on computers with many more cores.

[mhillenibm]

That's indeed what we see here. The build runs on a system with 4 CPUs (according to https://kojipkgs.fedoraproject.org//work/tasks/8069/45798069/hw_info.log). Forcing the number of concurrent threads to 0 (e.g., ./dgemv_tester 1024 0 16) will reproduce the test failure (tested on both s390x and x86-64).

@susilehtola your build for Fedora31 runs with 2 CPUs and thus fails a bit earlier (because of the underflow 2 - 4) yet due to the same underlying reason.

Proposed fix:

--- a/cpp_thread_test/dgemv_thread_safety.cpp
+++ b/cpp_thread_test/dgemv_thread_safety.cpp
@@ -18,7 +18,7 @@ int main(int argc, char* argv[]){
        uint32_t maxHwThreads = omp_get_max_threads();

        if (maxHwThreads < 52)
-               numConcurrentThreads = maxHwThreads -4;
+               numConcurrentThreads = std::max((int)maxHwThreads - 4, 1);

        if (argc > 4){
                std::cout<<"ERROR: too many arguments for thread safety tester"<<std::endl;
@@ -34,6 +34,11 @@ int main(int argc, char* argv[]){
                numConcurrentThreads = std::stoul(cliArgs.at(1));
                numTestRounds = std::stoul(cliArgs.at(2));
        }
+
+       if(numConcurrentThreads==0) {
+               std::cout<<"ERROR: Invalid parameter. Cannot run test with zero threads."<<std::endl;
+               return -1;
+       }

        std::uniform_real_distribution<double> rngdist{-1.0, 1.0};
        std::vector<std::vector<double>> matBlock(numConcurrentThreads);

[martin-frbg]

As I said I can't recall why I ever wanted to run this with N-4 rather than N (as the equivalent DGEMM test still does) so the fix probably does not need to be that elaborate.

[susilehtola]

How about

$ gendiff OpenBLAS-0.3.10/ .threadsafe
diff -up OpenBLAS-0.3.10/cpp_thread_test/dgemv_thread_safety.cpp.threadsafe OpenBLAS-0.3.10/cpp_thread_test/dgemv_thread_safety.cpp
--- OpenBLAS-0.3.10/cpp_thread_test/dgemv_thread_safety.cpp.threadsafe  2020-06-14 22:03:04.000000000 +0200
+++ OpenBLAS-0.3.10/cpp_thread_test/dgemv_thread_safety.cpp 2020-06-17 14:04:01.061831613 +0200
@@ -13,12 +13,9 @@ void launch_cblas_dgemv(double* A, doubl

 int main(int argc, char* argv[]){
    blasint randomMatSize = 1024; //dimension of the random square matrices and vectors being used
-   uint32_t numConcurrentThreads = 52; //number of concurrent calls of the functions being tested
    uint32_t numTestRounds = 16; //number of testing rounds before success exit
    uint32_t maxHwThreads = omp_get_max_threads();
-   
-   if (maxHwThreads < 52)
-       numConcurrentThreads = maxHwThreads -4;
+   uint32_t numConcurrentThreads = std::max(64, maxHwThreads); //number of concurrent calls of the functions being tested

    if (argc > 4){
        std::cout<<"ERROR: too many arguments for thread safety tester"<<std::endl;

[martin-frbg]

52 was probably a tradeoff between memory requirement and test efficiency (and the number is documented in Makefile.rule) but I already "broke" that when I changed the DGEMM value to use the maximum available on the drone.io CI. Calling omp_get_max_threads then got added as it turned out drone.io jobs cycled between a 96-core and a 44-core hardware...

[susilehtola]

Sure, I mean you can probably reduce to 32 threads.