Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Windows support and make FlexiBLAS relocatable #72

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

julianh-siemens
Copy link

Description of the three commits in this PR:

  • native Windows support ( issue Windows support #9 )
    • use https://github.com/dlfcn-win32/dlfcn-win32 for dlopen() and the like on Windows to unify the code for all operating systems. Simply tell cmake where dlfcn-win32 resides at configure time, e.g. via -DCMAKE_PREFIX_PATH="C:\path\to\dlfcn-win32
    • To avoid extra implementation of POSIX functionality that is missing on Windows but used in FlexiBLAS, I am using the headers from https://github.com/win32ports/, specifically: sys/time.h, dirent.h, getopt.h, strings.h, unistd.h. These headers can be put anywhere and added at cmake configuration time via -DFLEXIBLAS_WINDOWS_POSIX_HEADERS="C:\path\to\win32ports\include
    • The built is tested with VS2019 and Intel 2024.2.1 compilers
  • make FlexiBLAS relocatable ( issue Make FlexiBLAS relocatable #59 )
    • The location of libflexiblas.so / flexiblas.dll is detected at runtime and the paths to fallback libraries or config files is constructed relative to that path at runtime.
  • fix Windows compilation issue due to fatal error LNK1170: line in command file contains 131071 or more characters
    • This commit needs some discussion and adaption for the other LAPACK versions, if adopted, and might need to be merged with the first commit.
    • This is just an example for LAPACK 3.12.0. For BLAS the same thing is already done in FlexiBLAS, where all functions are put into a single file. For this commit I simply copied everything from src/lapack_interface/wrapper/*.c into one file (actually by separated by data type but that is irrelevant) and removed the duplicate copyright statements and includes. No other changes to the source code.

@@ -106,6 +106,11 @@ INCLUDE(ccache)


#SET(BUILD_SHARED_LIBS ON)
IF (WIN32)
SET(LIBS libm)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Afaict, there is no libm on Windows. Instead, the math functions are part of the Windows CRT (which is linked by default anyway).
If you have a libm on Windows, it is most likely empty.

If you made this modification to avoid an error about a missing library, consider making this change instead:

IF (NOT WIN32)
    SET(LIBS m)
ENDIF()

Fwiw, that situation is not specific to Windows only. Afaict, MUSL also provides the math functions as part of the CRT (and mostly ships an empty libm).

I'm also not sure whether it is intended to override the value of LIBS (which might be set to something different by a user before this change).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, I started at some point to check if libm is required. So in generate in the root file, the must be a check, if libm is required and which is the correct name.

ELSE ()
SET ( CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -D_FILE_OFFSET_BITS=64")
IF ( CMAKE_Fortran_COMPILER_ID STREQUAL "Intel" OR CMAKE_Fortran_COMPILER_ID STREQUAL "GNU")
IF (WIN32)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like parts of the flags in this block are MSVC-specific. Afaict, they won't work with a MinGW compiler.
E.g., instead of adding -Qstd=c99, consider setting CMAKE_CXX_STANDARD.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean CMAKE_C_STANDARD?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Yes, CMAKE_C_STANDARD would be correct here.

@@ -434,6 +472,28 @@ INCLUDE(lapack)

INCLUDE_DIRECTORIES(${PROJECT_SOURCE_DIR}/src)

IF(WIN32)
# place all (link) libraries in the same folder as the dynamic libraries
set(CMAKE_INSTALL_FULL_LIBDIR "${CMAKE_INSTALL_FULL_BINDIR}")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is probably not intended for MinGW toolchains (like the ones from MSYS2).

config.h.in Outdated Show resolved Hide resolved
#endif

#ifndef FALLBACK_NAME
#define FALLBACK_NAME "libflexiblas_netlib"
#if defined(__WIN32__)
#define FALLBACK_NAME "flexiblas_netlib"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be MSVC specific. MinGW toolchains prefix libraries with lib also on Windows.

The same holds a couple of lines below.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So checking for __MINGW32__ and/or __MINGW64__ should be done here as well.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd guess specifically identifying MSVC (or compilers that are behaving similar to MSVC - like Clang in MSVC mode) would be easier to implement (instead of trying to find out that the compiler is not MSVC). E.g., something along the lines of the following:

 #if defined(_MSC_VER)
  #define FALLBACK_NAME "flexiblas_netlib"
 #else
  #define FALLBACK_NAME "libflexiblas_netlib"
 #endif

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively (and maybe more general): there might be some CMake generator expression magic that could determine the library name from the target as part of the configuration process.

@@ -75,7 +79,11 @@ INCLUDE(CXXCompilerSettings)
INCLUDE(FortranCompilerSettings)

# Compile Options
SET(LIBS ${LIBRARIES} "m")
IF (WIN32)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See the comment on the change for the root CMakeLists.txt.

Co-authored-by: Markus Mützel <markus.muetzel@gmx.de>
#ifndef WINDOWS_FIXES_H
#define WINDOWS_FIXES_H 1

#if defined(_WIN32) || defined(_WIN64)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _WIN64 is defined by the toolchain _WIN32 is also always defined. So, the second part of this condition could be dropped for simplicity.
But it doesn't hurt to keep it either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants