[PATCH] D150646: [clang][X86] Add __cpuidex function to cpuid.h

Thu Aug 3 11:59:26 PDT 2023

aaron.ballman added a comment.

In D150646#4450915 <https://reviews.llvm.org/D150646#4450915>, @aidengrossman wrote:

> In D150646#4450551 <https://reviews.llvm.org/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?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150646/new/

https://reviews.llvm.org/D150646