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

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

Merged

Conversation

mborn-adi
Copy link
Contributor

The Intel Compiler Classic on MS-Windows doesn't have the intrinsics "__builtin_ctz" nor "__builtin_ctzll". Instead, it emulates MSVC. Unfortunately, "__has_builtin" works for version 2021, but results in a linker error nonetheless.
This pull request changes the detection of these intrinsics accordingly.
I also took the liberty to remove it from format.h, because it was already defined in core.h.
The Intel compiler also doesn't know "#pragma managed(push/pop", so I guarded those as well to get rid of the resulting warnings.

Best Regards,
Mathias

include/fmt/core.h Outdated Show resolved Hide resolved
@mborn-adi
Copy link
Contributor Author

__ICL is only defined on Windows, __INTEL_COMPILER on all platforms.

I verified this myself, but you can read about it here.

#if defined(__INTEL_COMPILER)
#ifdef __ICL
# define FMT_ICC_VERSION __ICL
# define FMT_ICC_ON_WINDOWS 1
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest replacing FMT_ICC_ON_WINDOWS with FMT_ICC_POSIX defined to 1 on non-Windows platforms. This will simplify the checks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine with me.

Comment on lines 195 to 192
# pragma managed(push, off)
# if !defined(__ICL)
# pragma managed(push, off)
# endif
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove #pragma managed completely as we don't support managed extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So done. Please have a look.

Comment on lines 168 to 169
#if (FMT_GCC_VERSION || FMT_HAS_BUILTIN(__builtin_ctz) || FMT_ICC_VERSION) && \
FMT_ICC_POSIX
Copy link
Contributor

@vitaut vitaut Sep 25, 2021

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.

#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?

@mborn-adi
Copy link
Contributor Author

I have renamed the macro to make the intend clear. It's a work around for a compiler bug.

@vitaut vitaut merged commit 32865ae into fmtlib:master Oct 2, 2021
@vitaut
Copy link
Contributor

vitaut commented Oct 2, 2021

Thank you. Please report the compiler bug to Intel.

Iniesta8 pushed a commit to jhnc-oss/fmt that referenced this pull request Oct 28, 2021
…fmtlib#2510)

* changed detection of Intel Compiler Classic to distinguish MS-Windows

* replaced !FMT_ICC_ON_WINDOWS by FMT_ICC_POSIX
removed #pragma manged

* replaced FMT_ICC_POSIX with FMT_ICC_INTRINSIC_BUG to be crystal clear about the macro's purpose
PoetaKodu pushed a commit to pacc-repo/fmt that referenced this pull request Nov 11, 2021
…fmtlib#2510)

* changed detection of Intel Compiler Classic to distinguish MS-Windows

* replaced !FMT_ICC_ON_WINDOWS by FMT_ICC_POSIX
removed #pragma manged

* replaced FMT_ICC_POSIX with FMT_ICC_INTRINSIC_BUG to be crystal clear about the macro's purpose
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