[PATCH] D102732: [CMake] Don't LTO optimize targets that aren't part of any distribution

[Petr Hosek via Phabricator via llvm-commits]

phosek marked an inline comment as not done.
phosek added inline comments.


================
Comment at: llvm/CMakeLists.txt:934
 
+include(LLVMDistributionSupport)
+
----------------
smeenai wrote:
> Is this moved up just to get the distribution target map built in time for the new use?
Yes, I need `LLVM_DISTRIBUTION_FOR_*` to be populated before creating any targets.


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:221
 function(add_link_opts target_name)
+  if(LLVM_DISTRIBUTION_COMPONENTS OR LLVM_DISTRIBUTIONS)
+    get_property(distribution GLOBAL PROPERTY LLVM_DISTRIBUTION_FOR_${target_name})
----------------
smeenai wrote:
> 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.
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).


================
Comment at: llvm/cmake/modules/AddLLVM.cmake:242
   # linker in a context where the optimizations are not important.
   if (NOT uppercase_CMAKE_BUILD_TYPE STREQUAL "DEBUG")
 
----------------
smeenai wrote:
> phosek wrote:
> > I'm wondering if we should avoid this branch for non-distribution targets as well to further reduce cost, what do you think?
> 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.
Makes sense, I can keep dead-stripping and just drop the `-Wl,-O3`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102732/new/

https://reviews.llvm.org/D102732