This is an archive of the discontinued LLVM Phabricator instance.

Revert "[clang][X86] Add __cpuidex function to cpuid.h"
ClosedPublic

Authored by aidengrossman on Aug 4 2023, 9:26 AM.

Details

Summary

This reverts commit 2df77ac20a1ed996706b164b0c4ed5ad140f635f.

This has been causing some issues with some windows builds as _MSC_EXTENSIONS isn't defined when only -fms-extensions is set, but the builtin that conflicts with __cpuidex is. This was also causing problems as it exposed some latent issues with how auxiliary triples are handled in clang.

Diff Detail

Event Timeline

aidengrossman created this revision.Aug 4 2023, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 9:26 AM
aidengrossman requested review of this revision.Aug 4 2023, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 4 2023, 9:26 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Assuming everyone agrees to this going in, I'll land it and then open a backport for the 17 release and reland once the auxiliary triple issues are resolved.

Thank you for this! The revert LGTM, but please wait for @mstorsjo to confirm this won't cause problems for MinGW folks.

In terms of solving the underlying issue with aux triple so that we can re-land this, I went digging to see if there's an existing issue tracking this and I could not find one. However, related to the functionality in this patch, I did see: https://github.com/llvm/llvm-project/issues/60383. I filed https://github.com/llvm/llvm-project/issues/64436 to track the aux-triple issue; hopefully we can find someone with the bandwidth to look into that issue (I'll see if I can get someone at Intel to look into it, but if someone else wanted to tackle it first, that would be great).

Thank you for this! The revert LGTM, but please wait for @mstorsjo to confirm this won't cause problems for MinGW folks.

I guess this is fine.

We should probably sync mingw-w64 and revert https://github.com/mingw-w64/mingw-w64/commit/2b6c9247613aa830374e3686e09d3b8d582a92a6 after that. There's less rush in reverting it though (not many projects use __cpuidex so it's not so bad to not have it defined it at all - while having a double conflicting definition is fatal), but I guess I should update it at least once the 17.0.0 release gets closer and we see where we're settled (maybe bump the version threshold to 18 instead of 17, if we're somewhat sure we'll have it in main even if it's reverted in 17.x).

For mingw, I'd at least appreciate if we don't switch it back and forth further on the 17.x release branch after 17.0.0 is released.

Thanks for the quick response!

Sorry for all the churn with this patch. I'm fine leaving this out of LLVM 17, so I'll land this patch, backport the commit to the 17 release branch, and then once we have the aux triple and MS extensions flag fixes I'll reland in main and won't backport it.

This revision was not accepted when it landed; it landed in state Needs Review.Aug 4 2023, 1:29 PM
This revision was automatically updated to reflect the committed changes.

Thank you for the revert -- I've confirmed it fixed the issues for our downstream. Also, @eandrews said she may try to tackle the underlying issue with builtins in the relatively near future.

clang/lib/Headers/cpuid.h