Page MenuHomePhabricator

[CMake] Don't LTO optimize targets that aren't part of any distribution
ClosedPublic

Authored by phosek on May 18 2021, 4:17 PM.

Details

Summary

When using distributions, targets that aren't included in any
distribution don't need to be as optimized as targets that are
included since those targets are typically only used for tests.

We might consider avoiding LTO for these targets altogether, see
https://lists.llvm.org/pipermail/llvm-dev/2021-April/149843.html

Diff Detail

Event Timeline

phosek created this revision.May 18 2021, 4:17 PM
phosek requested review of this revision.May 18 2021, 4:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 4:17 PM
phosek added inline comments.May 18 2021, 4:27 PM
llvm/cmake/modules/AddLLVM.cmake
240

I'm wondering if we should avoid this branch for non-distribution targets as well to further reduce cost, what do you think?

This is a neat idea.

llvm/CMakeLists.txt
934

Is this moved up just to get the distribution target map built in time for the new use?

llvm/cmake/modules/AddLLVM.cmake
221

In https://reviews.llvm.org/D89177?vs=on&id=343946#change-XYKH4tDojBz0, I had a get_llvm_distribution target in LLVMDistributionSupport that would tell you whether a target was in a distribution or not. I removed it from the final version of that diff because I added a more specific function instead that handled all the use cases I had, but for this use case, I think adding that function back would be nicer, so that e.g. the LLVM_DISTRIBUTION_FOR_* property naming scheme remains centralized inside LLVMDistributionSupport.

The other thing that function handled was umbrella targets (e.g. making sure llvm-libraries pulled in all LLVM libraries). Do you care about that support here? I haven't actually thought about how LTO would work if you're distributing (static) libraries as part of your distribution, as in would you wanna build them as bitcode or regular archives.

240

I think it'd make sense. Not related to your diff though, but depending on where the linker handles dead-stripping, I imagine it might actually make links *faster*, since it's reducing the amount of data the linker has to process. I think @thakis might have observed that when implementing dead-stripping in LLD for Mach-O.

phosek marked an inline comment as not done.May 18 2021, 4:38 PM
phosek added inline comments.
llvm/CMakeLists.txt
934

Yes, I need LLVM_DISTRIBUTION_FOR_* to be populated before creating any targets.

llvm/cmake/modules/AddLLVM.cmake
221

I like that idea so I'll go ahead do that. I haven't thought of including static libraries when using LTO and I'm not sure how useful that's going to be since those libraries would contain just bitcode. This could be another potential use case for -fembed-bitcode where the static library would contain both machine code and bitcode and consumers can decide which of the two to use (at the expense of increased binary size).

240

Makes sense, I can keep dead-stripping and just drop the -Wl,-O3.

phosek updated this revision to Diff 346306.May 18 2021, 5:00 PM
phosek marked 2 inline comments as done.May 18 2021, 5:00 PM
smeenai added inline comments.May 18 2021, 5:08 PM
llvm/cmake/modules/LLVMDistributionSupport.cmake
59–62

The second sentence of this comment "By convention, ..." should be removed ... that's handled by get_target_export_arg now.

131

I think we need an else here to preserve the previous behavior of this function?

phosek updated this revision to Diff 346313.May 18 2021, 5:13 PM
phosek marked 2 inline comments as done.
smeenai accepted this revision.May 18 2021, 5:15 PM

LGTM; thanks for cleaning up get_target_export_arg as well :)

This revision is now accepted and ready to land.May 18 2021, 5:15 PM
bogner added a subscriber: bogner.May 24 2021, 3:57 PM
bogner added inline comments.
llvm/CMakeLists.txt
934

I'm seeing test failures if I do ninja distribution test-depends and then ninja check and I suspect moving this is related. It's an llvm-config test that fails because lib/libLLVMObjCMetadata.a wasn't built though, so it's entirely possible that moving this just exposed a missing dependency somewhere else.

Do you happen to have any insight here?

phosek added inline comments.May 24 2021, 4:02 PM
llvm/CMakeLists.txt
934

Is it possible to reproduce that failure? We're using ninja distribution with https://github.com/llvm/llvm-project/blob/main/clang/cmake/caches/Fuchsia-stage2.cmake and haven't seen that issue but our distribution may contain a different set of targets.

bogner added inline comments.May 24 2021, 8:05 PM
llvm/CMakeLists.txt
934

This might be a problem on my side. Sorry for the noise.