ENH: add support for fujitsu C/C++ compiler and SSL2 to numpy. by yamadafuyuka · Pull Request #22982 · numpy/numpy

yamadafuyuka · 2023-01-10T08:42:39Z

This is my first pull request, so please let me know if there is anything missing.

I added an implementation for the Fujitsu C/C++ compiler (fcc/FCC) and the SSL2 library.

To build with the Fujitsu compiler, please build as follows:

$ python setup.py build -c fujitsu install

SSL2 is a library that provides OpenBLAS compatible GEMM functions. SSL2 in Fujitsu Compiler is the library in the same position as MKL in Intel Compiler.
To use SSL2, please edit site.cfg. I changed system_info.py based on mkl implementation.
The regression test was run the following command:.

$ python runtests.py

Because I didn't know how to specify a compiler in runtests.py, I modified the implementation to use the Fujitsu compiler and ran the tests.
Tests passed other than version check.

seberg

Just a few thoughts to maybe help get this rolling soon. However, I am worried that this is using a C++ compiler and not a C one.

There is also a thing that we want to migrate to meson soon, and I am not sure how these changes would be affected by it. Not that it matters too much, since this cannot be tested by us anyway (if it breaks, it breaks).

seberg · 2023-01-10T15:28:24Z

numpy/core/src/npymath/npy_math_internal.h.src

    if (mod) {
-        if (isless(b, (@type@)0) != isless(mod, (@type@)0)) {
+        if (!isnan@c@(b) && !isnan@c@(mod) && !isinf@c@(b) && !isinf@c@(mod) &&
+            ((b < (@type@)0) != (mod < (@type@)0))) {


Just to get a start on this, how come this is necessary? Is the compiler not C99 conforming or are some tests flake?
Note that we already ignore some warnings on certain platforms, so if it is the latter, it might also just be necessary to ignore the warnings. (because in the end, few users care)

If it is not C99 compliant, I am not sure, I find that surprising (unless you are compiling with a C++ compiler rather C?)

Sorry for the late reply.

When I do a regression test on NumPy built with fcc, it fails on np.floor_divide for float128.
In the current fujitsu compiler, when isgreater(nan, 1.0l) or isless(nan, 1.0l) is performed for float128, it compares the number with nan without checking nan and the FE_INVALID flag is raised. This is the cause of the error.
When doing np.floor_divide with Nan, the message <stdin>:1: RuntimeWarning: invalid value encountered in floor_divide appears.

In the case of float128

$ python >>> import numpy as np >>> fnan = np.array(np.nan, dtype='float128') >>> fone = np.array(1.0, dtype='float128') >>> np.floor_divide(fnan, fone) <stdin>:1: RuntimeWarning: invalid value encountered in floor_divide nan

Other than float128

$ python >>> import numpy as np >>> fnan = np.array(np.nan) >>> fone = np.array(1.0) >>> np.floor_divide(fnan, fone) nan

Therefore, we have rewritten the code to check for Nan and run if Nan is not present.

If we change the code regarding this part, it will have a large impact on other environments. So, I think it is better not to change the code, but to make it a limitation when using the Fujitsu compiler. What do you think? I will change it so that regression tests that fail because of this will be skipped.

Oh, just the longdouble implementation of isless gets it wrong! Yes, I would suggest to skip the test, since it seems the most pragmatic choice. This had been giving warnings for years on most platforms before we fixed it :).

We have some tests that have for example (I might prefer xfail really, but either works):

@pytest.mark.skipif(platform.machine() == "armv5tel", reason="See gh-413.")

Thank you very much. I had no idea that this had happened in the past on other platforms.

I would like to make sure that NumPy was built with fujitsu compiler as this problem only happens when built with this.

I don't feel like I can determine this from sys.platform, etc. Do you know any way to verify used compiler when doing pytest?

Actually, not sure. @ganesh-k13 is there a simple way to get the compiler used? Sounds like it would also be nice to include in np.show_runtime().

Ah that would be useful, and I tried adding it on #20939. Let me see if we can include it in show_runtime somehow.
Long term fix is already in place in #22769, here

Thanks, but that means there is no easy way to get it right now to skip a test, I guess. Maybe a very specific platform rule could work in practice?

numpy/core/src/npymath/npy_math_private.h

seberg · 2023-01-10T15:33:05Z

numpy/distutils/fujitsuccompiler.py

+            linker_exe=cc_compiler +
+            ' -O3 -Nclang -fPIC -lfj90i -lfj90f -lfjsrcinfo -lelf -shared',
+            linker_so=cc_compiler +
+            ' -O3 -Nclang -lfj90i -lfj90f -lfjsrcinfo -lelf -shared'


Adding compiler options here seems not correct? -O3 is not necessarily best for NumPy anyway, although I suppose it may be for many use-cases (at least when not on Linux, since we are probably a bit sloppy with the NPY_GCC_OPT_3 macro, which could be extended!)

You are right, there is no need to specify -O3 in the linker options.
Sorry, I overlooked that. I will correct it.

seberg · 2023-01-10T15:35:38Z

Sorry, I somehow forgot the main comment: There is a spurious first commit (probably you started from the 1.24 branch rather than main). Could you remove that please?

yamadafuyuka · 2023-01-11T02:50:50Z

Hi @seberg

Thank you for your review comments.

Sorry, I somehow forgot the main comment: There is a spurious first commit (probably you started from the 1.24 branch rather than main). Could you remove that please?

Commit log has been fixed.

Just a few thoughts to maybe help get this rolling soon. However, I am worried that this is using a C++ compiler and not a C one.

The file selection.cpp seems to require a C++compiler. This file appears to have been added to 2022/06.

seberg · 2023-01-11T10:58:02Z

Just for reference, there was fortran compiler support added in gh-17792 (there is a comment about patching ssl there too).

seberg · 2023-01-12T18:08:00Z

@t-karatsu just curious if you are still interested in fujitsu compiler support work? In that case it would be great if you could have a look here!

eli-schwartz · 2023-01-13T01:18:09Z

There is also a thing that we want to migrate to meson soon, and I am not sure how these changes would be affected by it. Not that it matters too much, since this cannot be tested by us anyway (if it breaks, it breaks).

@yamadafuyuka,

To have this work in the Meson port, you'll want to add a compiler class here: https://github.com/mesonbuild/meson/blob/master/mesonbuild/compilers

Do not hesitate to ask me questions if you wish for guidance implementing this. :)

yamadafuyuka · 2023-01-13T02:26:56Z

Hi, @eli-schwartz
Thank you for your comment. I will check it out.

yamadafuyuka · 2023-01-17T09:55:11Z

Fixed implementation and pushed.

Test skip due to fujitsu compiler specification

When building NumPy with fujitsu compiler, the isless() function for float128 causes a warning and the test fails.
When using NumPy built with fujitsu compiler, we assume that SSL2 is also enabled in most cases. Therefore, the following test is skipped when SSL2 is enabled.
- test_umath.py: test_float_divmod_errors(), test_floor_division_errors()

Test skip due to SVE specification

Some tests fail due to SVE specification, so we changed to skip them. This occurs not only in fujitsu compiler but also in gcc.
If the number of array elements is not a multiple of the SIMD width, such as 15, "divide by 0" or "0 x inf" will set the FE_DIVBYZERO or FE_INVALID flag and generate a warning.
The following test is now skipped if the result of the lscpu command contains sve.
- test_multiarray.py: test_sort_float()
- test_scalarmath.py: test_blocked()

charris · 2023-01-17T15:58:52Z

Needs a release note. @mattip What about meson support?

mattip · 2023-01-17T17:12:43Z

What about meson support?

All the changes in numpy/distutils should be redone for the meson build. One way to check this would be for someone with the compiler to try out python dev.py build; python dev.py test.

yamadafuyuka · 2023-01-27T01:58:02Z

Creating release notes.
I don't support building by meson yet, but can you merge the current code?

For meson support, you will need to add fujitsu compiler to meson first, so it will take some time.

rgommers · 2023-01-27T12:06:02Z

I think we're fine with reviewing and merging this with numpy.distutils support only. @yamadafuyuka for your information, in the next release (1.25.0, ~June) we will very likely already default to building with Meson. And if not, then 1.25.1/2, because we need it for Python 3.12 support.

A few questions on the BLAS part of this. Is there anything nonstandard in SSL2 that needs custom handling (e.g. build variants that need to be probed for, with/without some symbols or CBLAS support)? I can't find much info on it, no docs online it seems. https://github.com/spack/spack/pull/18498/files seems to hardcode -lpthreads - does it not support OpenMP? Does Fujitsu ship SLL2 with pkg-config files?

yamadafuyuka · 2023-01-31T07:00:45Z

@rgommers
Thank you for reply.

Is there anything nonstandard in SSL2 that needs custom handling (e.g. build variants that need to be probed for, with/without some symbols or CBLAS support)?

SSL2 has binaries for single and multi-threaded. I think it is ok to link only the multi-threaded version in NumPy. This eliminates the need to probe build variables.
SSL2 has CBLAS support. The BLAS functions of SSL2 has the same interface as the generic BLAS.

does it not support OpenMP?

The fcc supports OpenMP of LLVM.

Does Fujitsu ship SLL2 with pkg-config files?

There is no pkg-config for SSL2.
Fcc users typically set the install_prefix of fcc to the $TCSDS_PATH. So we can use a relative path using $TCSDS_PATH for include_dir and library_dir of site.cfg.exaple.

kawakami-k · 2023-02-06T01:42:42Z

Let me give you a few additional explanations.

SSL2 library provides 8 variants (so files) for BLAS functions. Single-threaded/Multi-threaded, NEON(128-bit SIMD)/SVE(512-bit SIMD), LP64/ILP64. SVE of Armv8/9 architecture is defined as scalable SIMD size (128, 256, 384, 512, ..., 2048). H/W implementer can chose one of them. Because the SVE size of Fujitsu A64FX is 512-bit and SSL2 provided for A64FX, the so files of SSL2 for SVE is implemented for 512-bit SVE. We can chose which variant to link by linker option (-lfjlapack, -lfjlapacksve, -lfjlapack_ilp64, -lfjlapacksve_ilp64, -lfjlapackex, -lfjlapackexsve -lfjlapackex_ilp64, -lfjlapackexsve_ilp64).

We assume that the users who build numpy with the Fujitsu compilers are those who want to have A64FX handle large problems. In this case, we think it is better to use the multi-threaded SVE version, so we prefer to link multi-threaded/SVE/LP64/ by default, and how about mentioning in the release notes how to link the other variants?

yamadafuyuka · 2023-02-16T06:26:27Z

Is there anything else I can do to get this pull request merged?

I'm currently fixing the code to pass the CI test.
I do not know why the build fails with the travel CI test.

seberg

Looks good to give a shot, modulo that one typo.

I might prefer xfail at least for the first test since it generates spurious warnings, I think (which can be pretty annoying compared to missing warnings). But either is fine.

seberg · 2023-02-16T08:00:43Z

numpy/core/tests/test_multiarray.py

+                            shell=True).communicate()[0]).decode('utf-8')
+        return "sve" in process
+    except OSError:
+        return false


Suggested change

return false

return False

seberg · 2023-02-16T08:01:24Z

numpy/core/tests/test_scalarmath.py

+        return false

 class TestBaseMath:
+    @pytest.mark.skipif(check_support_sve, reason="gh-22982")


I think I slightly prefer xfail for this one, but either is OK.

Suggested change

@pytest.mark.skipif(check_support_sve, reason="gh-22982")

@pytest.mark.xfail(check_support_sve, reason="gh-22982")

seberg · 2023-02-16T08:08:20Z

numpy/core/tests/test_scalarmath.py

+                            shell=True).communicate()[0]).decode('utf-8')
+        return "sve" in process
+    except OSError:
+        return false


Actually, same typo here. Maybe move it together with the IS_WASM definition to not repeat this?

Thank you very much.
I moved check_support_sve() functions to numpy/testing/_private/utils.py.

OOps, made this comment a few days ago, but had it stored as pending review. May just follow up today, though.

Thanks, now I notice that it is public, and since it feels a bit like a hack, I may want to rename it to _SUPPORTS_SVE for now (although happy to be convinced otherwise as soon as you need it in SciPy or so).
(Happy if others disagree, its just my gut feeling.)

You removed the except OSError:, maybe it is better to have it? I am wondering if it cannot fail on weirder setups.

Anyway, may just follow up on that myself quickly, thanks.

@seberg

Sorry for the late reply.
Changed to _SUPPORTS_SVE.
Changed the check method to use OSError.

I also reviewed the fujitsu compiler options.

If it looks good, put the commit together.

Thank you.

Thanks, tests are failing now, do you know why? I don't recall them failing before and am not aware what would have changed.

@seberg
Sorry, I fixed it so that the test passes.
Could you please check again?

charris · 2023-02-23T23:09:53Z

@seberg Ping.

seberg · 2023-03-06T07:59:57Z

Thanks @yamadafuyuka lets give it a shot :).

github-actions bot added the 01 - Enhancement label Jan 10, 2023

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch 3 times, most recently from c8aa06d to 6a1d406 Compare January 10, 2023 09:46

seberg reviewed Jan 10, 2023

View reviewed changes

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch 2 times, most recently from 15b78fd to b2499de Compare January 11, 2023 02:29

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch from b2499de to be59f10 Compare January 17, 2023 09:44

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch from be59f10 to 559354b Compare January 17, 2023 09:56

charris added the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Jan 17, 2023

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch 3 times, most recently from 2594de2 to 5fcd83f Compare January 27, 2023 11:40

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch 2 times, most recently from 3d7a22b to 9a3da62 Compare February 14, 2023 01:52

seberg reviewed Feb 16, 2023

View reviewed changes

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch from 0337aff to c317bb3 Compare February 21, 2023 06:33

charris removed the 56 - Needs Release Note. Needs an entry in doc/release/upcoming_changes label Feb 23, 2023

ENH: add support for fujitsu C/C++ compiler and SSL2 to numpy.

fccb005

yamadafuyuka force-pushed the add_fujitsuccompiler_and_SSL2 branch from 2a6b449 to fccb005 Compare March 2, 2023 08:33

seberg merged commit b4f6b0b into numpy:main Mar 6, 2023

kawakami-k mentioned this pull request Mar 9, 2023

BLD: add SSL2 default path for Fujitsu C/C++ compiler #23361

Merged

lschoe mentioned this pull request Jun 15, 2023

BUG: message "'lscpu' is not recognized as an internal or external command, ..." printed #23954

Closed

	@pytest.mark.skipif(check_support_sve, reason="gh-22982")
	@pytest.mark.xfail(check_support_sve, reason="gh-22982")

Uh oh!

Conversation

yamadafuyuka commented Jan 10, 2023

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamadafuyuka Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seberg commented Jan 10, 2023

Uh oh!

yamadafuyuka commented Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seberg commented Jan 11, 2023

Uh oh!

seberg commented Jan 12, 2023

Uh oh!

eli-schwartz commented Jan 13, 2023

Uh oh!

yamadafuyuka commented Jan 13, 2023

Uh oh!

yamadafuyuka commented Jan 17, 2023

Test skip due to fujitsu compiler specification

Test skip due to SVE specification

Uh oh!

charris commented Jan 17, 2023

Uh oh!

mattip commented Jan 17, 2023

Uh oh!

yamadafuyuka commented Jan 27, 2023

Uh oh!

rgommers commented Jan 27, 2023

Uh oh!

yamadafuyuka commented Jan 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kawakami-k commented Feb 6, 2023

Uh oh!

yamadafuyuka commented Feb 16, 2023

Uh oh!

seberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yamadafuyuka Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

yamadafuyuka Jan 13, 2023 •

edited

Loading

yamadafuyuka commented Jan 11, 2023 •

edited

Loading

yamadafuyuka commented Jan 31, 2023 •

edited

Loading

yamadafuyuka Mar 3, 2023 •

edited

Loading