[PATCH] D23600: [LTO] Stop always creating and running an LTO compilation if there is not a single LTO object

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23600#522256, @mehdi_amini wrote:

> What the current status for this one, with all the recent patches that got in?


The commons fix is in, so this one should be good to go with the change noted below. Why don't I test it right now with SPEC just to be sure...


================
Comment at: lib/LTO/LTO.cpp:710
@@ -719,2 +709,3 @@
   // partitions.
-  unsigned Task = RegularLTO.ParallelCodeGenParallelismLevel;
+  unsigned Task = RegularLTO.CombinedModule
+                      ? RegularLTO.ParallelCodeGenParallelismLevel
----------------
tejohnson wrote:
> This change causes a different failure. We assert in thinLTOInternalizeModule/MustPreserveGV for the first ThinLTO module. The internalizer calls back to MustPreserveGV for the added common, but we assert because it is not in the DefinedGlobals since there was no summary for it in that module.
> 
> However, I found that with my internalize fix for commons we don't need this change with your patch. We simply keep the common symbol in the module where it was originally defined, as a common, which is what happened before the new API. Let me do some more testing with that fix, and see if it fixes the rest of the cpu2006 benchmark failures I was getting. If that works, then you could submit this patch (minus this particular change) afterwards.
> 
> However, while testing things I encountered another issue where the new API is going to cause problems for the distributed build system case. I first reverted this particular part of your patch and tested it on 403.gcc without my internalization fix. I expected to get undefs for the common symbols, thinking that they wouldn't be defined anywhere (we internalize the original copy and remove it since it didn't have uses there, and we would presumably not add it to any combined module which we shouldn't have with ThinLTO). However, we did end up with a combined module - there are several modules that don't have summary indexes, I believe because they contain inline assembly and we are currently preventing issues there by suppressing index generation). The test in LTO::add causes them to be added to a single combined module. This will be a problem for a distributed build where we compile with the thinlto-index-only plugin option as we will not get any index files for these files. The distributed build system is going to barf because it wants those as input to backend actions for those modules. I already hit a related issue since with the new API we stopped generating these files for the modules the linker decided not to include in the link, which is a change from the earlier behavior where gold would always emit the individual indexes (and imports files if requested) for all modules unconditionally, and I was planning to make a change to address this. I will also change things so we add modules as ThinLTO unconditionally under an option passed from the client.
> However, I found that with my internalize fix for commons we don't need this change with your patch.
> We simply keep the common symbol in the module where it was originally defined, as a common, which is
> what happened before the new API. Let me do some more testing with that fix, and see if it fixes the rest of
> the cpu2006 benchmark failures I was getting. If that works, then you could submit this patch (minus this
> particular change) afterwards.

Since the commons fix is in, this change can be reverted.


https://reviews.llvm.org/D23600