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

Cannot build with Intel MKL include dirs. #202

Closed
iago-lito opened this issue Sep 11, 2023 · 9 comments
Closed

Cannot build with Intel MKL include dirs. #202

iago-lito opened this issue Sep 11, 2023 · 9 comments
Assignees
Labels
bug Something isn't working

Comments

@iago-lito
Copy link

Environment

toml++ version and/or commit hash:

Submodule : TOML++ v3.3.0 (c635f21)

Compiler:

$ c++ --version
c++ (GCC) 13.2.1 20230801

C++ standard mode:

set(CMAKE_CXX_STANDARD 17)

Target arch:

$ uname -a
Linux copak 6.4.12-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 24 Aug 2023 00:38:14 +0000 x86_64 GNU/Linux

Library configuration overrides:
none

Relevant compilation flags:
After having installed intel-oneapi-mkl:

-I/opt/intel/oneapi/compiler/latest/linux/compiler/include

Describe the bug

The library compiles fine, until I add the above flag (because the project otherwise uses MKL), then I get:

In file included from ./extern/toml++/include/toml++/toml.h:36,
                 from ./main.cpp:1:
./extern/toml++/include/toml++/impl/forward_declarations.h:38:15: error: ‘FLT_RADIX’ was not declared in this scope
   38 | static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);
      |               ^~~~~~~~~

Steps to reproduce (or a small repro code sample)

main.cpp

#include <toml++/toml.h>
int main(){};

CMakeLists.txt

cmake_minimum_required(VERSION 3.20..3.22)

project(TomlAndMkl LANGUAGES CXX)

set(CMAKE_CXX_STANDARD 17)

add_library(toml++ INTERFACE)
target_include_directories(toml++ INTERFACE "./extern/toml++/include")

add_executable(main main.cpp)

#-------------------------------------------------------------------------------
# Works if I remove this snippet.
find_package(PkgConfig REQUIRED)
pkg_search_module(MKL mkl-dynamic-lp64-iomp)
target_include_directories(main PRIVATE ${MKL_INCLUDE_DIRS})
#-------------------------------------------------------------------------------

target_link_libraries(main PRIVATE toml++) 

I'm not exactly sure what the error deeply means here. FWIU the FLT_RADIX macro is (re-)defined within intel/oneapi/compiler/latest/linux/compiler/include/float.h:75 to:

#undef  FLT_RADIX
#define FLT_RADIX   2

but I'm not confident what it means or why this would result in "undeclared FLT_RADIX". I'm happy to learn more ^ ^"

@iago-lito iago-lito added the bug Something isn't working label Sep 11, 2023
@marzer
Copy link
Owner

marzer commented Sep 11, 2023

Hmnmn, that is weird! If float.h is undefining and redefining FLT_RADIX immediately, nothing should be amiss from toml++'s perspective. I suppose the MKL package does something weird with FLT_RADIX elsewhere and there's some include-order shenanigans going on with the CMake config. I have no idea what to do about it, though. This seems like a bug in the MKL headers.

@marzer
Copy link
Owner

marzer commented Sep 11, 2023

Can you try replacing toml++'s use of FLT_RADIX with std::numeric_limits<double>::radix? i.e. change this line in toml++/impl/forward_declarations.hpp:

static_assert(FLT_RADIX == 2, TOML_ENV_MESSAGE);

to this:

static_assert(std::numeric_limits<double>::radix == 2, TOML_ENV_MESSAGE);

@marzer
Copy link
Owner

marzer commented Sep 11, 2023

If that doesn't work, you can #define TOML_DISABLE_ENVIRONMENT_CHECKS somewhere before you include toml++ and it should work OK. The float radix is only used to check that the local environment is compatible with the TOML spec, and isn't actually used anywhere else.

@iago-lito
Copy link
Author

Replacing the macro invocation by std::*::radix does fix the problem. And indeed I should maybe report this to intel-mkl. It seems that they broke something ^ ^" Thank you for swift support, and feel free to close then. I'll just need to figure out where to find intel now ^ ^"

@iago-lito
Copy link
Author

you can #define TOML_DISABLE_ENVIRONMENT_CHECKS

I think I'll go for some more targetted hack instead:

#ifndef FLT_RADIX
#define FLT_RADIX std::numeric_limits<float>::radix
#endif

.. but I just want to get rid of one confusion first: do you expect FLT_RADIX to be std::numeric_limits<float>::radix or std::numeric_limits<double>::radix ? (because it seems that DBL_RADIX also exists)

@marzer
Copy link
Owner

marzer commented Sep 11, 2023

I think I'll go for some more targetted hack instead [...]

I'd caution against that solution; the standard specifies std::numeric_limits<(float|double|long double)>::radix in terms of FLT_RADIX (ref), so you might end up causing compiler errors elsewhere. I'll add a workaround to the toml++ code to detect FLT_RADIX shenanigans to avoid this in future.

but I just want to get rid of one confusion first [...]

Either would be fine since they should be the same, but floats in TOML++ are (at least) 64-bit so it uses double everywhere internally. (double isn't necessarily going to be 64-bit on all platforms, of course, but the other static asserts check for that.)

@marzer marzer closed this as completed in 42a428f Sep 11, 2023
@iago-lito
Copy link
Author

Wop, thank you for spotting the snag! ^ ^"

@iago-lito
Copy link
Author

FWIW it seems that the error does not occur.. provided you opt into using Intel's proprietary compiler icpx :\

@marzer
Copy link
Owner

marzer commented Sep 13, 2023

Ah, right, so I guess MKL assumes you're using that compiler and does #undef FLT_RADIX because it knows that the compiler has some special behaviour in that area.

That's... unfortunate 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants