This implements the same behavior as D141997 but makes sure that the same detection mechanism is used between CMake and source code.
Details
- Reviewers
sivachandra lntue - Commits
- rGfd64482e3d1a: [libc][NFC] Detect host CPU features using try_compile instead of try_run.
rGc84d74f5bfe8: [reland][libc][NFC] Detect host CPU features using try_compile instead of…
rG9acc2f37bdfc: [libc][NFC] Detect host CPU features using try_compile instead of try_run.
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
I have two design questions:
- Do we want to merge "architectures.h" and "cpu_features.h" since they usually are used together?
- Do we want to autogenerate the C files since they are not quite minimal?
I think it is good to have "architectures.h" and "cpu_features.h" as 2 separate files, but maybe we can require user to only include one of them by like including "cpu_features.h" at the end of "architectures.h" (or the other way around).
About autogenerating those C files, for now I don't think we need it yet, as the number of CPU features are quite minimal and manageable. So for a few CPU features that we care right now, explicit check files are clearer IMO, and I don't expect them to be too many.
libc/cmake/modules/LLVMLibCCheckCpuFeatures.cmake | ||
---|---|---|
60 | Look like SOURCES argument needs to be right after output directory. |
Mostly LGTM but a few minor comments.
I don't see a CMakeLists.txt change and a corresponding Bazel change for the new header file.
I like the separation.
- Do we want to autogenerate the C files since they are not quite minimal?
I think the explicit listing is probably a better approach. As this get more complicated, the CMake scripting will get more complicated. I prefer simpler CMake scripting over increased verbosity/repetitiveness.
libc/cmake/modules/cpu_features/check_AVX2.c | ||
---|---|---|
1 ↗ | (On Diff #490484) | Nit: These checker sources are including a .h which is marked C++. Also, we need a C++ compiler anyway for the building the libc anyway. So, why not make these files have a .cpp extension? |
libc/src/__support/cpu_features.h | ||
9 | This comment seems incomplete; Perhaps, "This file **lists** target CPU features ..." ? |
Thx for the review.
I think the explicit listing is probably a better approach. As this get more complicated, the CMake scripting will get more complicated. I prefer simpler CMake scripting over increased verbosity/repetitiveness.
I fail to understand this comment. The first part seems to suggest to keep the patch as-is but AFAIU the second part suggests to use CMake scripting.
No, the second part is kind of reasoning out why I prefer keeping the patch as is - to keep the CMake scripting simpler.
For a reason that I don't understand yet, the build bots are not detecting the cpu features.
https://lab.llvm.org/buildbot/#/builders/90/builds/44634/steps/4/logs/stdio
-- Set COMPILER_RESOURCE_DIR to /usr/lib/llvm-11/lib/clang/11.0.1 using --print-resource-dir -- Building libc for x86_64 on linux -- Set CPU features: -- Skipping libc entrypoint libc.src.stdlib.getenv. -- Skipping libc entrypoint libc.src.stdio.fopen.
@sivachandra is there a way to log onto one of the build bots to try to reproduce the issue?
I haven't been able to reproduce it locally.
Look like SOURCES argument needs to be right after output directory.