Page MenuHomePhabricator

[libc] Allow target architecture customization
ClosedPublic

Authored by gchatelet on May 6 2021, 6:01 AM.

Details

Summary

This patch provides a way to specify the default target cpu optimizations to use when compiling llvm-libc.
This ensures we don't rely on current compiler's default and allows compiling and cross compiling for a particular target.

Diff Detail

Event Timeline

gchatelet created this revision.May 6 2021, 6:01 AM
gchatelet requested review of this revision.May 6 2021, 6:01 AM
gchatelet updated this revision to Diff 343382.May 6 2021, 6:04 AM
  • Fix formatting
gchatelet updated this revision to Diff 343383.May 6 2021, 6:04 AM
  • include both commits
sivachandra added inline comments.May 6 2021, 8:44 AM
libc/cmake/modules/LLVMLibCLibraryRules.cmake
85

We should not need to set compile options here. If you notice, the add_library rule on line 79 does not compile anything. It just packs a bunch of object files into a static archive.

114

Same here as well. We should not need to set compile options.

libc/cmake/modules/LLVMLibCObjectRules.cmake
151

While you are at it, can you fix this: we should use the same default compile options for add_entrypoint_object and add_object_library above. So, essentially, I am saying that we should be sharing common_compiler_options between these two rules.

gchatelet updated this revision to Diff 343601.May 7 2021, 1:00 AM
gchatelet marked an inline comment as done.
  • Remove unnecessary target_compile_options
  • Use same common compile options between add_entrypoint_object and add_object_library
gchatelet marked 2 inline comments as done.May 7 2021, 1:01 AM
sivachandra accepted this revision.May 7 2021, 9:20 AM
sivachandra added inline comments.
libc/cmake/modules/LLVMLibCObjectRules.cmake
44–45

Instead of _set_*, I think a function with name _get_* will be more appropriate here. The function should also take an additional arg for the result:

_get_all_compile_options(all_opts ...)
target_compile_options(${fq_target_name} PRIVATE ${all_opts})

This probably a nitty comment but motivated by the fact that common is not suitable in add_object_library.

This revision is now accepted and ready to land.May 7 2021, 9:20 AM
gchatelet added inline comments.May 10 2021, 12:53 AM
libc/cmake/modules/LLVMLibCObjectRules.cmake
44–45

I was pondering whether get or set was more suitable and I've been lazy : )
Thx for the comment it makes sense to go with get indeed.

This revision was automatically updated to reflect the committed changes.