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.

Sign up for GitHub

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

changed detection of Intel Compiler Classic to distinguish MS-Windows #2510

Merged
merged 3 commits into from
Oct 2, 2021
Merged

changed detection of Intel Compiler Classic to distinguish MS-Windows #2510

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3acda1c
changed detection of Intel Compiler Classic to distinguish MS-Windows
mborn-adi Sep 24, 2021
17fb5de
replaced !FMT_ICC_ON_WINDOWS by FMT_ICC_POSIX removed #pragma manged
mborn-adi Sep 24, 2021
a29db70
replaced FMT_ICC_POSIX with FMT_ICC_INTRINSIC_BUG to be crystal clear…
mborn-adi Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion include/fmt/core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,15 @@
# define FMT_GCC_PRAGMA(arg)
#endif

#if defined(__INTEL_COMPILER)
#ifdef __ICL
# define FMT_ICC_VERSION __ICL
# define FMT_ICC_POSIX 0
#elif defined(__INTEL_COMPILER)
# define FMT_ICC_VERSION __INTEL_COMPILER
# define FMT_ICC_POSIX 1
#else
# define FMT_ICC_VERSION 0
# define FMT_ICC_POSIX 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is FMT_ICC_POSIX equal to 1 for other compilers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are making a good point. It is misleading. But it makes sure the intrinsic detection still works here for other compilers:

fmt/include/fmt/format.h

Lines 168 to 169 in 17fb5de

#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctz) || FMT_ICC_VERSION) && \
FMT_ICC_POSIX

Since this is essentially a work around for a compiler bug, perhaps we should introduce something like FMT_ICC_BUILTIN_CTZ_BUG to be crystal clear about it?

#endif

#ifdef __NVCC__
Expand Down
19 changes: 5 additions & 14 deletions include/fmt/format.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,6 @@
# define FMT_GCC_VISIBILITY_HIDDEN
#endif

#ifdef __INTEL_COMPILER
# define FMT_ICC_VERSION __INTEL_COMPILER
#elif defined(__ICL)
# define FMT_ICC_VERSION __ICL
#else
# define FMT_ICC_VERSION 0
#endif

#ifdef __NVCC__
# define FMT_CUDA_VERSION (__CUDACC_VER_MAJOR__ * 100 + __CUDACC_VER_MINOR__)
#else
Expand Down Expand Up @@ -173,10 +165,13 @@ FMT_END_NAMESPACE
!FMT_MSC_VER
# define FMT_BUILTIN_CLZLL(n) __builtin_clzll(n)
#endif
#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctz) || FMT_ICC_VERSION)
#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctz) || FMT_ICC_VERSION) && \
FMT_ICC_POSIX
Copy link
Contributor

@vitaut vitaut Sep 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can simplify this to just

#if FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctz) || FMT_ICC_POSIX

and similarly below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not.
FMT_HAS_BUILTIN(__builtin_ctz) and FMT_HAS_BUILTIN(__builtin_ctzll) evaluate true on the Intel Classic 2021, and using those intrinsics compiles just fine, but results in linker errors. I think that's a bug in the compiler.

# define FMT_BUILTIN_CTZ(n) __builtin_ctz(n)
#endif
#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctzll) || FMT_ICC_VERSION)
#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctzll) || \
FMT_ICC_VERSION) && \
FMT_ICC_POSIX
# define FMT_BUILTIN_CTZLL(n) __builtin_ctzll(n)
#endif

Expand All @@ -192,7 +187,6 @@ FMT_BEGIN_NAMESPACE
namespace detail {
// Avoid Clang with Microsoft CodeGen's -Wunknown-pragmas warning.
# if !defined(__clang__)
# pragma managed(push, off)
# pragma intrinsic(_BitScanForward)
# pragma intrinsic(_BitScanReverse)
# if defined(_WIN64)
Expand Down Expand Up @@ -254,9 +248,6 @@ inline auto ctzll(uint64_t x) -> int {
return static_cast<int>(r);
}
# define FMT_BUILTIN_CTZLL(n) detail::ctzll(n)
# if !defined(__clang__)
# pragma managed(pop)
# endif
} // namespace detail
FMT_END_NAMESPACE
#endif
Expand Down