[clang][X86] Add __cpuidex function to cpuid.h

[aidengrossman]

Summary

MSVC has a __cpuidex function implemented to call the underlying cpuid
instruction which accepts a leaf, subleaf, and data array that the output
data is written into. This patch adds this functionality into clang
under the cpuid.h header. This also makes clang match GCC's behavior.
GCC has had __cpuidex in its cpuid.h since 2020.

[craig.topper]

For MSVC compatibility this was already implemented as a builtin for intrin.h in https://reviews.llvm.org/D121653

[craig.topper]

LGTM

I find it a little odd that we have if you include cpuid.h you get the gcc interface for __cpuid and the MSVC interface for __cpuidex. But gcc did it first so I guess we should match.

[aidengrossman]

Thanks for the review! Definitely a little bit odd and it probably would've been better if originally replicated in some other way, but definitely agree on the matching behavior.

[glandium]

This causes errors like:

/builds/worker/fetches/clang/lib/clang/17/include/cpuid.h(331,22): error: definition of builtin function '__cpuidex'
 static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                      ^
 1 error generated.

when building Firefox with clang-cl. (presumably, just including cpuid.h with clang-cl triggers it)

[glandium]

Repro:

$ echo '#include "cpuid.h"' | ./clang/bin/clang-cl  -Tp -
clang-cl: warning: unable to find a Visual Studio installation; try running Clang from a developer command prompt [-Wmsvc-not-found]
In file included from <stdin>:1:
/tmp/ct/clang/lib/clang/17/include/cpuid.h(331,22): error: definition of builtin function '__cpuidex'
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
1 error generated.

(interestingly doesn't happen in C mode (with -Tc instead of -Tp))

[aidengrossman]

Thanks for notifying me of this. I've reverted it as I don't have too much time currently to thoroughly go through things and I don't want to keep you blocked. Essentially, the built-in for __cpuidex on Windows platforms landed in https://reviews.llvm.org/D121653 which is conflicting with this new implementation (from what I can gather). If that's the issue though, it's interesting that it doesn't seem like there are any conflicts between the __cpuid builtin and the one defined in cpuid.h. Hoping to get a fix up (relatively) soon but not sure currently exactly what it will look like. @glandium Do you have a link to a source file that ends up triggering this error in Firefox? I'd be interested in seeing the context.

[craig.topper]

In D150646#4351981, @aidengrossman wrote:

Thanks for notifying me of this. I've reverted it as I don't have too much time currently to thoroughly go through things and I don't want to keep you blocked. Essentially, the built-in for __cpuidex on Windows platforms landed in https://reviews.llvm.org/D121653 which is conflicting with this new implementation (from what I can gather). If that's the issue though, it's interesting that it doesn't seem like there are any conflicts between the __cpuid builtin and the one defined in cpuid.h. Hoping to get a fix up (relatively) soon but not sure currently exactly what it will look like. @glandium Do you have a link to a source file that ends up triggering this error in Firefox? I'd be interested in seeing the context.

I think __cpuid doesn't conflict because it's a macro in cpuid.h rather than a function definition.

[glandium]

This is at least one of the spots where it happens: https://searchfox.org/mozilla-central/rev/2ca95198a2a0806de358a0484d96d4354e3cbaab/third_party/highway/hwy/targets.cc#42 (HWY_COMPILER_MSVC is explicitly 0 with clang-cl per https://searchfox.org/mozilla-central/rev/2ca95198a2a0806de358a0484d96d4354e3cbaab/third_party/highway/hwy/detect_compiler_arch.h#36-40)

[aidengrossman]

Fix conflict with MS compatibility __cpuidex builtin. This update adds a
preprocessor guard around the __cpuidex definition in cpuid.h that checks
if _MSC_EXTENSIONS is defined. If _MSC_EXTENSIONS is defined, we can know
that the MS builtins are also included (see ./clang/lib/Basic/Targets/OSTargets.cpp:229
and ./clang/lib/Basic/Builtins.cpp:91), so we shouldn't define the
function. Also adds in a test to make sure that everything works as expected
when MS extensions/compatibility is enabled.

[ronlieb]

seeing this issue in our downstream builders:
[2023-05-24T11:46:07.333Z] /opt/rocm-5.6.0-12074/llvm/lib/clang/17.0.0/include/cpuid.h:333:22: error: static declaration of '__cpuidex' follows non-static declaration

[2023-05-24T11:46:07.333Z] static inline void cpuidex (int cpu_info[4], int leaf, int __subleaf)

[2023-05-24T11:46:07.333Z] ^

[aidengrossman]

Do you have any more information? Do you have your own implementation of __cpuidex somewhere else before including cpuid.h?

[glandium]

There seem to still be two problems with this change, with mingw:

• with -fms-extensions:

echo '#include "cpuid.h"' | ./clang/bin/clang++ --target=x86_64-w64-windows-gnu -xc++ - -fms-extensions
In file included from <stdin>:1:
/tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: definition of builtin function '__cpuidex'
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
1 error generated.

• conflict with mingw headers without the extensions:

$ echo '#include "intrin.h"' | ./clang/bin/x86_64-w64-mingw32-clang++ -xc++ -
In file included from <stdin>:1:
In file included from /tmp/clang/lib/clang/17/include/intrin.h:12:
In file included from /tmp/clang/bin/../x86_64-w64-mingw32/include/intrin.h:70:
/tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: static declaration of '__cpuidex' follows non-static declaration
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
/tmp/clang/bin/../x86_64-w64-mingw32/include/psdk_inc/intrin-impl.h:2031:6: note: previous definition is here
void __cpuidex(int CPUInfo[4], int function_id, int subfunction_id) {
     ^
1 error generated.

Although, for the latter, this was "fixed" in https://sourceforge.net/p/mingw-w64/mingw-w64/ci/2b6c9247613aa830374e3686e09d3b8d582a92a6/

[mstorsjo]

In D150646#4379543, @glandium wrote:

There seem to still be two problems with this change, with mingw:

• with -fms-extensions:

echo '#include "cpuid.h"' | ./clang/bin/clang++ --target=x86_64-w64-windows-gnu -xc++ - -fms-extensions
In file included from <stdin>:1:
/tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: definition of builtin function '__cpuidex'
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
1 error generated.

Ouch - this one is tricky, as -fms-extensions doesn't seem to set any define (it doesn't set _MSC_EXTENSIONS at least - and I don't know if it would unlock surprises all around if it started doing that?); the main noticable difference in the output of clang -E -dM - < /dev/null seems to be that __stdcall and __cdecl and other calling conventions no longer are defines. But that's a rather brittle thing to check against.

• conflict with mingw headers without the extensions:

$ echo '#include "intrin.h"' | ./clang/bin/x86_64-w64-mingw32-clang++ -xc++ -
In file included from <stdin>:1:
In file included from /tmp/clang/lib/clang/17/include/intrin.h:12:
In file included from /tmp/clang/bin/../x86_64-w64-mingw32/include/intrin.h:70:
/tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: static declaration of '__cpuidex' follows non-static declaration
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
/tmp/clang/bin/../x86_64-w64-mingw32/include/psdk_inc/intrin-impl.h:2031:6: note: previous definition is here
void __cpuidex(int CPUInfo[4], int function_id, int subfunction_id) {
     ^
1 error generated.

Although, for the latter, this was "fixed" in https://sourceforge.net/p/mingw-w64/mingw-w64/ci/2b6c9247613aa830374e3686e09d3b8d582a92a6/

Regarding "fixed" - do you see any better way of fixing it there? As it's a static inline function, there's no simple way of knowing whether it was already defined or not; I went with the same pattern used for the existing version check for GCC.

[glandium]

In D150646#4380058, @mstorsjo wrote:

Regarding "fixed" - do you see any better way of fixing it there? As it's a static inline function, there's no simple way of knowing whether it was already defined or not; I went with the same pattern used for the existing version check for GCC.

I mean, it fixes the issue if you apply the patch or update mingw headers. Whether that should be required or not is a good question.

[aidengrossman]

In D150646#4379543, @glandium wrote:

There seem to still be two problems with this change, with mingw:

• with -fms-extensions:

echo '#include "cpuid.h"' | ./clang/bin/clang++ --target=x86_64-w64-windows-gnu -xc++ - -fms-extensions
In file included from <stdin>:1:
/tmp/clang/lib/clang/17/include/cpuid.h:334:22: error: definition of builtin function '__cpuidex'
static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
                     ^
1 error generated.

Interesting. I would've thought -fms-extensions by itself would've declared that macro (which is what I gathered reading what caused the MSVC compatible builtins to get included in clang), but apparently not. For whatever reason, LangOpts.MicrosoftExt only seems to be set to true if -fms-compaitilibty-version (maybe on top of -fms-compatibility) are also passed (not 100% on this though). I'll have to do some more digging soon.

[glandium]

In D150646#4382763, @aidengrossman wrote:

Interesting. I would've thought -fms-extensions by itself would've declared that macro (which is what I gathered reading what caused the MSVC compatible builtins to get included in clang), but apparently not. For whatever reason, LangOpts.MicrosoftExt only seems to be set to true if -fms-compaitilibty-version (maybe on top of -fms-compatibility) are also passed (not 100% on this though). I'll have to do some more digging soon.

Did you find something?

[aidengrossman]

In D150646#4450551, @glandium wrote:

Did you find something?

Not yet. I just finished finals a week ago so I haven't had as much time as I would've liked to get through this. I'll contact some people tonight in regards to the Windows flag situation and ask for their thoughts on this and hopefully have some sort of solution at least by next Monday.

[aaron.ballman]

In D150646#4450915, @aidengrossman wrote:

In D150646#4450551, @glandium wrote:

Did you find something?

Not yet. I just finished finals a week ago so I haven't had as much time as I would've liked to get through this. I'll contact some people tonight in regards to the Windows flag situation and ask for their thoughts on this and hopefully have some sort of solution at least by next Monday.

I think these changes exacerbate an issue that we had but didn't know about, because I'm able to reproduce the redefinition problem in both Clang 16 and trunk: https://godbolt.org/z/8Yq1vzEnT

What's happening is that the aux-triple is not being honored when compiling for an offload device (OpenMP, SYCL, etc) and so the wrong builtins are being enabled, leading to exposing __cpuidex as a builtin on non-Windows targets. This means the workaround to look for _MSC_EXTENSIONS doesn't solve the underlying issue -- we already try to only expose the intrinsic when MS extensions are enabled: https://github.com/llvm/llvm-project/blob/40f3708205430a7a562d58f48fd9c294fb80d5e0/clang/include/clang/Basic/BuiltinsX86.def#L2098 and https://github.com/llvm/llvm-project/blob/40f3708205430a7a562d58f48fd9c294fb80d5e0/clang/lib/Basic/Builtins.cpp#L91

The changes in this patch are causing us problems with our downstream at Intel. I think we might want to consider reverting the change temporarily (and on the 17.x branch) because they're exacerbating an existing problem. That gives us time to solve the problem of exposing the wrong set of builtins, and hopefully means we can remove the _MSC_EXTENSIONS workaround in the header file. WDYT?

[aidengrossman]

I'm not opposed to a revert, but I know some downstream users like MinGW have already adapted to this change so I'm not sure how much headache it would cause them to do a revert.

Maybe I'm not understanding things correctly, but I originally _MSC_EXTENSIONS patch as people were enabling the microsoft extensions (thus enabling the builtin) and still including cpuid.h which caused an error due to redefining a builtin function. There seems to be another issue there where multiple flags need to be set (-fms-extensions, -fms-compatibility, and fms-compatibility-version) in order to get _MSC_EXTENSIONS defined while only passing fms-extensions gets the builtin defined, but not the macro. I'm not sure if just passing -fms-extensions is even a valid configuration, but it does error out then. I believe that's a separate issue to the auxiliary triple issue you mentioned.

[aaron.ballman]

In D150646#4558543, @aidengrossman wrote:

I'm not opposed to a revert, but I know some downstream users like MinGW have already adapted to this change so I'm not sure how much headache it would cause them to do a revert.

Maybe I'm not understanding things correctly, but I originally _MSC_EXTENSIONS patch as people were enabling the microsoft extensions (thus enabling the builtin) and still including cpuid.h which caused an error due to redefining a builtin function. There seems to be another issue there where multiple flags need to be set (-fms-extensions, -fms-compatibility, and fms-compatibility-version) in order to get _MSC_EXTENSIONS defined while only passing fms-extensions gets the builtin defined, but not the macro. I'm not sure if just passing -fms-extensions is even a valid configuration, but it does error out then. I believe that's a separate issue to the auxiliary triple issue you mentioned.

Separating threads a bit:

• Regarding removing the _MSC_EXTENSIONS check -- you're right, that was a think-o suggestion on my part, we still need that to guard the definition in the header file. Sorry for that!
• Regarding when _MSC_EXTENSIONS is defined -- I think it's possibly a mistake that we don't define that when passing -fms-extensions. -fms-extensions is intended to control whether we support extensions to MSVC, such as use of __declspec attributes. And -fms-compatibility is intended to control whether we're trying to be compatible with MSVC rather than be compatible with GCC or the standards. Passing -fms-compatibility automatically adds -fms-extensions. I think the issue is here: https://github.com/llvm/llvm-project/blob/a6d673070963ed0900bd33963a9b62add07339f4/clang/lib/Basic/Targets/OSTargets.cpp#L265 -- we're looking for MSVCCompat to decide what macros to define, and only if we're in compatibility mode and enabled extensions do we enable _MSC_EXTENSIONS here https://github.com/llvm/llvm-project/blob/a6d673070963ed0900bd33963a9b62add07339f4/clang/lib/Basic/Targets/OSTargets.cpp#L229 -- I think it might be reasonable for us to lift the _MSC_EXTENSIONS logic out so it's not controlled by MSVCCompat, wdyt @rnk? (That's a separate concern from this patch though.)
• A revert would be appreciated so that we have time to untangle the actual root cause of the problems we're seeing at Intel where the wrong set of builtins are being enabled for offload targets. That's not caused by your patch, but your patch is causing us to hit the issue when we previously hadn't been hitting it. However, if it's quicker/easier for someone to fix that problem instead of doing a revert, that'd be even better -- but I do worry about timing given that we just cut the Clang 17 branch. CC @ABataev @jdoerfert as they may have seen this problem (https://godbolt.org/z/8Yq1vzEnT) with offload targets before and have opinions/ideas as well.

[rnk]

I think MSVC defines _MSC_EXTENSIONS under one of its compatibility modes as well, so it's not a good indicator of "is this compiler MSVC-like". I think we should be exposing the __cpudex builtin any time we are being MSVC-like, not under fms-compatibility. Would that make things sensible again?

[aidengrossman]

The main issue here is that there's no way to reliably detect whether the built in is defined (can't check if a function is defined in the preprocessor, preprocessor macro isn't exposed correctly in all configurations), which ends up breaking some configurations. I wouldn't be opposed to exposing things more often, but I'm fine with the builtins being exposed under -fms-extensions, we just need to expose the preprocessor macro when we expose the builtins.

[mstorsjo]

In D150646#4581664, @rnk wrote:

I think we should be exposing the __cpudex builtin any time we are being MSVC-like, not under fms-compatibility. Would that make things sensible again?

I think that might sound reasonable - what do we do for other MSVC-specific builtins today?

In D150646#4581672, @aidengrossman wrote:

The main issue here is that there's no way to reliably detect whether the built in is defined (can't check if a function is defined in the preprocessor, preprocessor macro isn't exposed correctly in all configurations), which ends up breaking some configurations.

That's indeed the issue

I wouldn't be opposed to exposing things more often, but I'm fine with the builtins being exposed under -fms-extensions, we just need to expose the preprocessor macro when we expose the builtins.

As far as I can see, @rnk's suggestion is the opposite - to tie whether the builtin is enabled to the target only, not to -fms-extensions. I.e. mingw + -fms-extensions doesn't get the builtin. Then the preprocessor check for it would simply be !defined(_MSC_VER).

Unfortunately, for Clang tests that invoke clang -cc1 directly, with an MSVC target triple, don't get _MSC_VER defined automatically, only if they pass -fms-compatibility-version= (which the Driver usually passes), so in such test conditions, the main way of detecting MSVC right now is defined(_WIN32) && !defined(__MINGW32__) which isn't very pretty either.

I played with a patch to make Clang always define _MSC_VER for MSVC targets, even if -fms-compatibility-version= wasn't set. That broke quite a few tests, since _MSC_VER unlocks lots of codepaths that only work when -fms-extensions is set (which the Driver usually does for MSVC targets, but usually isn't set in %clang_cc1 tests). So if we want to default to defining _MSC_VER on the cc1 level without an explicit -fms-compatibility-version=, we'd also need to imply -fms-extensions for MSVC targets, which I guess would go against a lot of the driver/frontend logic split.

clang/lib/Headers/cpuid.h

static __inline int __get_cpuid_count (unsigned int __leaf,
 
     if (__max_leaf == 0 || __max_leaf < __leaf)
         return 0;
 
     __cpuid_count(__leaf, __subleaf, *__eax, *__ebx, *__ecx, *__edx);
     return 1;
 }
 
+// If MS extensions are enabled, __cpuidex is defined as a builtin which will
+// conflict with the __cpuidex definition below.
+#ifndef _MSC_EXTENSIONS
+static __inline void __cpuidex (int __cpu_info[4], int __leaf, int __subleaf)
+{
+  __cpuid_count(__leaf, __subleaf, __cpu_info[0], __cpu_info[1], __cpu_info[2],
+                __cpu_info[3]);
+}
+#endif
+
 #endif /* __CPUID_H */

clang/test/Headers/__cpuidex_conflict.c

+// Make sure that __cpuidex in cpuid.h doesn't conflict with the MS
+// compatibility built in by ensuring compilation succeeds:
+// RUN: %clang_cc1 %s -ffreestanding -fms-extensions -fms-compatibility \
+// RUN:  -fms-compatibility-version=19.00 -triple x86_64-pc-windows-msvc -emit-llvm -o -
+
+typedef __SIZE_TYPE__ size_t;
+
+#include <intrin.h>
+#include <cpuid.h>
+
+int cpuid_info[4];
+
+void test_cpuidex(unsigned level, unsigned count) {
+  __cpuidex(cpuid_info, level, count);
+}

clang/test/Headers/cpuid.c

 // RUN: %clang_cc1 %s -ffreestanding -triple x86_64-apple-darwin -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-64
 // RUN: %clang_cc1 %s -ffreestanding -triple i386 -emit-llvm -o - | FileCheck %s --check-prefix=CHECK-32
 
 #include <cpuid.h>
 #include <cpuid.h> // Make sure multiple inclusion protection works.
 
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A cpuid\0A xchgq %rbx,${1:q}", "={ax},=r,={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
+// CHECK-64: {{.*}} call { i32, i32, i32, i32 } asm "  xchgq  %rbx,${1:q}\0A  cpuid\0A  xchgq  %rbx,${1:q}", "={ax},=r,={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
 
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", "={ax},={bx},={cx},={dx},0,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}})
 // CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", "={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
+// CHECK-32: {{.*}} call { i32, i32, i32, i32 } asm "cpuid", "={ax},={bx},={cx},={dx},0,2,~{dirflag},~{fpsr},~{flags}"(i32 %{{[a-z0-9]+}}, i32 %{{[a-z0-9]+}})
 
 unsigned eax0, ebx0, ecx0, edx0;
 unsigned eax1, ebx1, ecx1, edx1;
 
+int cpuid_info[4];
+
 void test_cpuid(unsigned level, unsigned count) {
   __cpuid(level, eax1, ebx1, ecx1, edx1);
   __cpuid_count(level, count, eax0, ebx0, ecx0, edx0);
+  __cpuidex(cpuid_info, level, count);
 }