Allow build dft on MinGW #540

Andarwinux · 2024-05-18T13:29:39Z

It actually works fine.

Andarwinux · 2024-05-18T13:35:08Z

I don't know about gcc, but at least llvm-mingw works fine, I used sleefdft for rubberband's fft library and got better performance than fftw.

blapie · 2024-05-22T17:13:32Z

Hello! We are a bit concerned about the absence of testing for this one. But I suppose DFT can still be disabled manually.
Any chance you could test with gcc?

blapie · 2024-05-22T17:18:06Z

Btw thanks for testing and reporting this, very useful! I'll try to test locally.

These tests don't support MinGW

Some platforms such as MSYS2 provide a broken libcrypto.pc, this is needed to fix statically link build.

To fix cross-compilation compatibility

Andarwinux · 2024-05-23T09:44:59Z

Hello! We are a bit concerned about the absence of testing for this one. But I suppose DFT can still be disabled manually. Any chance you could test with gcc?

I'll try to add a MSYS2 CI that can test gcc and clang. but expect some things to not work. MSYS2 also support clang arm64 and I've done a preliminary evaluation of arm64 build with my cross build toolchain, but that's hard to do in the CI since GHA doesn't support Windows Arm64 runner.

Andarwinux · 2024-05-23T13:57:18Z

Unfortunately, the mingw-w64 gcc does not work at all. But clang passes the test, will add msys2 native build & test and llvm-mingw aarch64 & x86_64 cross build later.

blapie · 2024-05-23T16:00:40Z

I'll try to add a MSYS2 CI that can test gcc and clang. but expect some things to not work.

Yes, it is most likely why we were holding it for now. But we would be ok to start with only a subset of the tests.

MSYS2 also support clang arm64 and I've done a preliminary evaluation of arm64 build with my cross build toolchain, but that's hard to do in the CI since GHA doesn't support Windows Arm64 runner.

Yes, we have also done some work to enable Windows on Arm (for sleef/libm) that is not yet upstream, happy to try and merge that soon if that can help. However as you said it is not available on GH-hosted runners, so we will have to keep testing internally for now.
Another issue with Windows on Arm is that libm tests rely heavily on POSIX, which is an issue with MSYS2. That's a shame because MSYS2 is quite a convenient environment to use. I have not checked if it's also the case for sleef/dft though, but I suspect it is.

blapie · 2024-05-23T16:37:05Z

Thanks for the contribution, impressive GHA skills!

I like the idea of the build/test-msys2 jobs, we were definitely missing testing for Windows (x86).

On the other hand, we did not desperately need a precommit workflow for llvm-mingw, but it is quite handy to be able to (cross-)compile for windows on both x86 and arm64, so happy try and merge that too.

Before getting into the details, would you mind moving both jobs to separate yaml files (even if it means duplicating code for build-native)? We might want to schedule and parametrise the 3 workflows differently. {build,test}-msys2 to build-and-test-windows.yml and build-llvm-mingw to build-cross-windows.yml?

Btw if you are aware of simple/nice ways to re-use bits of workflows (say build-native) in other workflows, please let us know.

Andarwinux · 2024-05-23T23:57:39Z

On the other hand, we did not desperately need a precommit workflow for llvm-mingw, but it is quite handy to be able to (cross-)compile for windows on both x86 and arm64, so happy try and merge that too.

Most MinGW users are cross-compile on Linux because the build are an order of magnitude faster, so I'd prefer cross-compile to be a first-class citizen.

Before getting into the details, would you mind moving both jobs to separate yaml files (even if it means duplicating code for build-native)? We might want to schedule and parametrise the 3 workflows differently. {build,test}-msys2 to build-and-test-windows.yml and build-llvm-mingw to build-cross-windows.yml?

Done. Native build are usually very fast, and since there is no longer need to pass native build files via artifacts, this is actually faster.

Btw if you are aware of simple/nice ways to re-use bits of workflows (say build-native) in other workflows, please let us know.

One possible way to do this is to save native builds to action cache, which is quite a bit faster than artifacts and can across workflows. But I suspect this would still be slower than build native and cross within same instance.

blapie · 2024-05-29T08:55:22Z

One possible way to do this is to save native builds to action cache, which is quite a bit faster than artifacts and can across workflows. But I suspect this would still be slower than build native and cross within same instance.

Thanks for the insight, action cache might be useful for other jobs. I like the present solution though, hopefully in the future a native build isn't even needed to cross-compile.

blapie · 2024-05-29T09:03:46Z

Regarding the libm and quad tests on msys2, would you be able to link us to the build or test failures?
The main reason I expect the build to fail is that wait.h is not available on msys2, since it is POSIX. For this reason I think the suggested changes to CMakeLists are acceptable, until we find a workaround.

Do you have suggestions on ways to port a code that relies on wait.h (wait, fork, ...) to mingw/msys2?

@shibatch Have you ever considered ways to make the libm tester compatible with mingw?

shibatch · 2024-05-29T09:27:11Z

Since tester1 does not need to be compiled with mingw, the test should be possible by combining tester1 for Cygwin or linux, although it may be a bit troublesome to write the script for the test.

Another idea could be to add a new tester using tlfloat, which I am currently developing. With tlfloat, testing is possible even if libmpfr is not available.

blapie · 2024-05-29T10:11:08Z

Since tester1 does not need to be compiled with mingw, the test should be possible by combining tester1 for Cygwin or linux, although it may be a bit troublesome to write the script for the test.

Fair, it's true that they can be compiled independently, but I'm afraid this will confuse things more than they already are on Windows.

Another idea could be to add a new tester using tlfloat, which I am currently developing. With tlfloat, testing is possible even if libmpfr is not available.

That could be interesting! Looks like you already have support for several OS-es there too. Mpfr is not the worst dependency to have, but always good to remove dependencies I suppose.

Would tlfloat resolve the dependency on wait.h and POSIX in general? Is it able to handle multithreading too?
My suggestion would have been to just brutally remove multithreading/calls to wait.h in tester1 on windows, but I'm not sure how to do that.

Alternatively, we could look at straight windows port for wait.h https://github.com/win32ports/sys_wait_h ?

shibatch · 2024-05-29T10:19:25Z

The new tester can be made pipe-free and multi-threading capable.

blapie · 2024-05-29T10:32:44Z

The new tester can be made pipe-free and multi-threading capable.

I suggest we create an issue specifically for this?

shibatch · 2024-05-29T10:45:11Z

Writing an Issue is fine, but there are other things to consider regarding this.
First is how to link tlfloat from sleef.

If we implement the fp16 support mentioned here, it could be related to that as well.
#320

blapie · 2024-05-29T14:43:14Z

.github/workflows/build-and-test-windows.yml

+            _install-*
+        if: always()
+
+  test-msys2:


Could we display information on the cpu in this job at least? Equivalent to what we have in the linux workflow:
https://github.com/shibatch/sleef/actions/runs/8845094575/workflow#L102
Ideally in all jobs but that might be a bit much.

blapie · 2024-05-29T14:45:06Z

.github/workflows/build-cross-windows.yml

+            -DCMAKE_INSTALL_PREFIX="$(pwd)/_install-${{ matrix.arch }}" \
+            -DCMAKE_SYSROOT="$(pwd)/llvm-mingw/${{ matrix.arch }}-w64-mingw32" \
+            -DNATIVE_BUILD_DIR="$(pwd)/_build-native" \
+            ${COMMON_CMAKE_FLAGS} \
+            -DCMAKE_C_COMPILER=${{ matrix.arch }}-w64-mingw32-clang \
+            -DCMAKE_ASM_COMPILER=${{ matrix.arch }}-w64-mingw32-clang \
+            -DCMAKE_SYSTEM_NAME=Windows \
+            -DCMAKE_SYSTEM_PROCESSOR=${{ matrix.arch }} \


Do you think we can rely on a toolchain file for most of these options? Our toolchain files are located in https://github.com/shibatch/sleef/tree/master/toolchains.

I don't think this makes sense, ideally llvm-mingw is multi-target, in order to support multiple targets, toolchain.cmake only include platform specific not architecture specific, which actually means that toolchain.cmake only include CMAKE_SYSTEM_NAME=Windows.

blapie · 2024-05-29T14:46:33Z

.github/workflows/build-and-test-windows.yml

+          #- mingw64
+          #- ucrt64


Could we have a quick comment saying why these are commented out?

mingw64 and ucrt64 are gcc envs, and gcc is completely broken.

Do you have any more details on that, maybe a link? Is the compiler broken or is it SLEEF when compiled in a gcc envs?

see https://github.com/Andarwinux/sleef/actions/runs/9207374820/job/25327401215
msys2/MSYS2-packages#1209

blapie · 2024-05-29T14:48:48Z

.github/workflows/build-cross-windows.yml

+            -DNATIVE_BUILD_DIR="$(pwd)/_build-native" \
+            ${COMMON_CMAKE_FLAGS} \
+            -DCMAKE_C_COMPILER=${{ matrix.arch }}-w64-mingw32-clang \
+            -DCMAKE_ASM_COMPILER=${{ matrix.arch }}-w64-mingw32-clang \


CMAKE_ASM_COMPILER one does not seem to be used, could just rm?

Andarwinux · 2024-05-29T18:22:11Z

Made some fixes and cleanups, and enabled LTO for clang.

blapie · 2024-05-30T16:01:31Z

README.md

 </tr>
 <tr align="center"><th>x86_64</th><th>SSE2, SSE4,<br>AVX, AVX2, AVX512F</th>
  <td>:green_circle:</td><td>:green_circle:</td><td>:white_circle:</td>
  <td>:white_circle:</td><td>:white_circle:</td>
-  <td>:white_circle:</td><td>:white_circle:</td><td>:white_circle:</td>
+  <td>:white_circle:</td><td>:green_circle:</td><td>:white_circle:</td><td>:white_circle:</td>


Thanks for taking the time to document that.
In this case only DFT is tested though. What we do at the moment is at least add a comment in the following subsections, here Component support section should indicate that on Windows only DFT is tested but everything is built.

Let's not bother in this patch but we should probably use a different colour scheme when only part of the library is tested, or when component is compiled only and not tested. Ultimately, this could probably be mapped to the workflow/job status.

msys2 clang and llvm-mingw cross

Subdivided Windows LLVM into llvm-gnu and llvm-msvc to distinguish between *-w64-mingw32 (aka *-pc-windows-gnu) and *-pc-windows-msvc targets. Updated Windows AArch64 from N/A to Not tested in CI.

blapie · 2024-05-31T09:49:48Z

.github/workflows/build-and-test-windows.yml

+        run: |
+          EXTRA_CMAKE_FLAGS=""
+          if [[ ${{ matrix.sys }} = "clang64" ]]; then
+            EXTRA_CMAKE_FLAGS="$EXTRA_CMAKE_FLAGS -DSLEEF_ENABLE_LTO=ON"


Can we please test without LTO for now? I would like to test SLEEF's basic features primarily, and test LTO separately if that's ok.

Added lto matrix.

Ok, works for me. But please refrain from adding too many variants/parameters as we have limited resources to address failures. Also special casing for combinations of parameters is a bit gross in GHA workflow files.

blapie · 2024-05-31T09:50:14Z

.github/workflows/build-and-test-windows.yml

+            EXTRA_CMAKE_FLAGS="$EXTRA_CMAKE_FLAGS -DSLEEF_ENABLE_LTO=ON"
+            EXTRA_CMAKE_FLAGS="$EXTRA_CMAKE_FLAGS -DSLEEF_ENFORCE_SSE2=ON -DSLEEF_ENFORCE_SSE4=ON -DSLEEF_ENFORCE_AVX=ON -DSLEEF_ENFORCE_AVX2=ON -DSLEEF_ENFORCE_AVX512F=ON -DSLEEF_ENFORCE_FMA4=ON"
+          elif [[ ${{ matrix.sys }} = "clangarm64" ]]; then
+            EXTRA_CMAKE_FLAGS="$EXTRA_CMAKE_FLAGS -DSLEEF_ENABLE_LTO=ON"


Same here, can we remove LTO for now.

blapie · 2024-05-31T09:50:42Z

.github/workflows/build-cross-windows.yml

+    -DSLEEF_BUILD_SCALAR_LIB=ON
+    -DSLEEF_BUILD_STATIC_TEST_BINS=ON
+    -DSLEEF_BUILD_TESTS=OFF
+    -DSLEEF_ENABLE_LTO=ON


Andarwinux · 2024-05-31T10:27:19Z

Rename yml to msys2 and llvm-mingw to avoid conflicts with future msvc/llvm-msvc{msvc-style, gnu-style}/llvm-msvc-cross{msvc-style, gnu-style}.

blapie · 2024-05-31T14:44:46Z

Are you happy for me to merge?

Andarwinux · 2024-05-31T15:25:59Z

Yes, LGTM.

Allow build dft on MinGW

aa7fdea

It actually works fine.

Andarwinux added 3 commits May 23, 2024 00:00

Don't build test for libm or quad on MinGW

2ef9f03

These tests don't support MinGW

Allow add extra libraries for OpenSSL

fcc5c23

Some platforms such as MSYS2 provide a broken libcrypto.pc, this is needed to fix statically link build.

arraymap.c: use windows.h instead of Windows.h

9326ba7

To fix cross-compilation compatibility

blapie mentioned this pull request May 23, 2024

Test more OS-es in Github Actions #493

Closed

blapie mentioned this pull request May 29, 2024

Improve portability of libm and quad testers #544

Open

blapie reviewed May 29, 2024

View reviewed changes

Andarwinux force-pushed the mingw branch 2 times, most recently from 904a57d to d663a23 Compare May 29, 2024 18:10

blapie reviewed May 30, 2024

View reviewed changes

Andarwinux force-pushed the mingw branch from d663a23 to bdc9a50 Compare May 30, 2024 16:26

Andarwinux added 2 commits May 31, 2024 00:00

Add CI on Github Actions for Windows

626c38d

msys2 clang and llvm-mingw cross

Update test matrix

b27b91c

Subdivided Windows LLVM into llvm-gnu and llvm-msvc to distinguish between *-w64-mingw32 (aka *-pc-windows-gnu) and *-pc-windows-msvc targets. Updated Windows AArch64 from N/A to Not tested in CI.

blapie reviewed May 31, 2024

View reviewed changes

Andarwinux force-pushed the mingw branch from bdc9a50 to c9cd251 Compare May 31, 2024 10:22

Andarwinux force-pushed the mingw branch from c9cd251 to b27b91c Compare May 31, 2024 10:29

blapie merged commit 0f25148 into shibatch:master May 31, 2024
39 checks passed

Andarwinux deleted the mingw branch June 1, 2024 09:33

blapie mentioned this pull request Jun 3, 2024

Restart/Fix CI tests #481

Open

BrewTestBot mentioned this pull request Jun 10, 2024

sleef 3.6.1 Homebrew/homebrew-core#174140

Merged

Allow build dft on MinGW #540

Allow build dft on MinGW #540

Conversation

Andarwinux commented May 18, 2024

Andarwinux commented May 18, 2024

blapie commented May 22, 2024

blapie commented May 22, 2024

Andarwinux commented May 23, 2024

Andarwinux commented May 23, 2024

blapie commented May 23, 2024

blapie commented May 23, 2024

Andarwinux commented May 23, 2024

blapie commented May 29, 2024

blapie commented May 29, 2024

shibatch commented May 29, 2024

blapie commented May 29, 2024

shibatch commented May 29, 2024

blapie commented May 29, 2024

shibatch commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarwinux May 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarwinux commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andarwinux commented May 31, 2024

blapie commented May 31, 2024

Andarwinux commented May 31, 2024

Andarwinux May 29, 2024 •

edited

Loading