This is an archive of the discontinued LLVM Phabricator instance.

[libc][NFC] Detect host CPU features using try_compile instead of try_run.
ClosedPublic

Authored by gchatelet on Jan 19 2023, 6:05 AM.

Diff Detail

Event Timeline

gchatelet created this revision.Jan 19 2023, 6:05 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 19 2023, 6:05 AM
gchatelet requested review of this revision.Jan 19 2023, 6:05 AM

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?
lntue added a comment.Jan 19 2023, 8:20 AM

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.

sivachandra accepted this revision.Jan 19 2023, 10:21 AM

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.

  • Do we want to merge "architectures.h" and "cpu_features.h" since they usually are used together?

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 ..." ?

This revision is now accepted and ready to land.Jan 19 2023, 10:21 AM
gchatelet updated this revision to Diff 490784.Jan 20 2023, 4:19 AM
gchatelet marked 3 inline comments as done.

Address comments

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.

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.

lntue accepted this revision.Jan 20 2023, 9:36 AM

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.

👍
Submitting then.

gchatelet added a comment.EditedJan 23 2023, 3:56 AM

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.

libc/cmake/modules/cpu_features/check_FMA.cpp