[compiler-rt][X86] Use functions in cpuid.h instead of inline assembly #97877
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This patch makes the host/feature detection in compiler-rt and LLVM use the functions provided in cpuid.h(__get_cpuid, __get_cpuid_count) instead of inline assembly. This simplifies the implementation and moves any inline assembly away to a more common place. A while ago, some similar cleanup was attempted, but this ended up resulting in some compilation errors due to toolchain minimum version issues (https://bugs.llvm.org/show_bug.cgi?id=30384). After the reversion landed, there have been no attempts since then to clean up the code, even though the minimum supported compilers now support the relevant functions (https://godbolt.org/z/o1Mjz8ndv).
boomanaiden154
requested review from
echristo,
MaskRay,
alinas,
RKSimon and
phoebewang
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
clang/lib/Headers/cpuid.h did not define |
MaskRay
approved these changes
Jul 6, 2024
Yep. I was a bit surprised when I saw everything was included in the minimum supported compiler versions as well. It definitely makes the code a lot cleaner and hopefully more robust as now there's one less inline assembly implementation. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/71/builds/1636 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/162/builds/1578 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/129/builds/1509 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/89/builds/1597 Here is the relevant piece of the build log for the reference: |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/157/builds/1991 Here is the relevant piece of the build log for the reference: |
aaryanshukla
pushed a commit
to aaryanshukla/llvm-project
that referenced
this pull request
Jul 14, 2024
… assembly (llvm#97877)" This reverts commit f6616e9. Was causing buildbot failures on Windows. I also remember seeing a AMDGPU buildbot failing somewhere on a warning as they have -Werror enabled.
boomanaiden154
added a commit
that referenced
this pull request
Jul 24, 2024
… assembly (#97877)" This reverts commit f1905f0. This relands commit 19cf8de. There were issues with the preprocessor includes that should have excluded MSVC still including clang functions building on windows and using intrin.h. This relanding fixes this behavior by additionally wrapping the uses of __get_cpuid and __get_cpuid_count in _MSC_VER so that clang in MSVC mode, which includes intrin.h, does not have any conflicts.
yuxuanchen1997
pushed a commit
that referenced
this pull request
Jul 25, 2024
… assembly (#97877)" Summary: This reverts commit f1905f0. This relands commit 19cf8de. There were issues with the preprocessor includes that should have excluded MSVC still including clang functions building on windows and using intrin.h. This relanding fixes this behavior by additionally wrapping the uses of __get_cpuid and __get_cpuid_count in _MSC_VER so that clang in MSVC mode, which includes intrin.h, does not have any conflicts. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60250754
|
@boomanaiden154 this is causing a build regression when compiling standalone compiler-rt for x86_64 in a cross build environment if I manually add |
|
You're going to have to provide more details about your environment. The compiler invocation should theoretically pickup the builtin headers. I'm not sure why it isn't in your environment. That (to me) seems like an issue with your environment rather than an issue with the patch though. |
its possible, let me do a bit more diving, if I revert this patch then it starts to build which is perhaps that it does not need to include the header so it works. |
yeah pilot error ! a rebase of local patch went wrong. sorry for noise. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch makes the host/feature detection in compiler-rt and LLVM use the functions provided in cpuid.h(__get_cpuid, __get_cpuid_count) instead of inline assembly. This simplifies the implementation and moves any inline assembly away to a more common place.
A while ago, some similar cleanup was attempted, but this ended up resulting in some compilation errors due to toolchain minimum version issues (https://bugs.llvm.org/show_bug.cgi?id=30384). After the reversion landed, there have been no attempts since then to clean up the code, even though the minimum supported compilers now support the relevant functions (https://godbolt.org/z/o1Mjz8ndv).