[PATCH] D78988: [LTO] Suppress emission of the empty object file

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks, the changes make more sense to me now with the gold plugin changes removed. I have some more comments below. Once this is cleaned up a bit more I will give it a quick test or so internally to make sure it doesn't break our flow (I don't think so but worth a check).



================
Comment at: lld/test/ELF/lto/linker-script-symbols-assign.ll:6
 ; RUN: ld.lld %t.o -o %t2 --script %t.script -save-temps
-; RUN: llvm-nm %t2.lto.o | count 0
-
----------------
Is this change still valid with your new approach of keeping a flag on the RegularLTO state object? Or is lld not even sending the object to LTO? If so, probably should check now that this file isn't created.


================
Comment at: lld/test/ELF/lto/linker-script-symbols-assign.ll:8
-
-; CHECK-NOT: bar
-; CHECK-NOT: foo
----------------
Looks like these orphan checks were left behind when a recent change updated the test... 


================
Comment at: llvm/include/llvm/LTO/Config.h:69
 
+  /// Allow emit Regular LTO object from empty module. This option is usedful
+  /// for some build system which want to know a priori all possible output
----------------
Suggest making the first sentence "Always emit a Regular LTO object even when it is empty because no Regular LTO modules were linked."

Also, s/usedful/useful/


================
Comment at: llvm/lib/LTO/LTO.cpp:1033
+                  std::move(RegularLTO.CombinedModule), ThinLTO.CombinedIndex,
+                  RegularLTO.EmptyCombinedModule))
     return Err;
----------------
Actually, do we even need to call backend() if (!EmptyCombinedModule || C.AlwaysEmitRegularLTOObj) ? It doesn't seem like there should be any need to create a TM or call opt in that case either. And as I note in backend(), there's no need to do the splitCodeGen handling either.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:497
   if (ParallelCodeGenParallelismLevel == 1) {
-    codegen(C, TM.get(), AddStream, 0, *Mod);
+    if (!EmptyCombinedModule || C.AlwaysEmitRegularLTOObj)
+      codegen(C, TM.get(), AddStream, 0, *Mod);
----------------
Move this check out a level and guard both this and the splitCodeGen call the same way (split codegen isn't commonly used, but it makes sense to be consistent - splitCodeGen will ultimately call codegen()).


================
Comment at: llvm/test/ThinLTO/X86/empty-module.ll:6
 ; RUN: llvm-readobj -h %t2.0 | FileCheck %s
 ; RUN: llvm-nm %t2.0 2>&1 | count 0
 
----------------
Presumably %t2.0 is the empty combined module. Should it be emitted after your change? Oh I see, this is related to your llvm-lto2 change. I'll add a comment there.


================
Comment at: llvm/test/ThinLTO/X86/empty-object.ll:1
+; Test to ensure lto2 will not emit empty module
+; RUN: rm -f %t.1 %t.2
----------------
"empty combined module"

However, I'm unsure what this new test is trying to check that is different than the existing empty-module.ll. 

Are you trying to confirm that we emit an empty ThinLTO module (corresponding to %t2.bc) but not the empty combined module? If so, please clarify in description.


================
Comment at: llvm/test/ThinLTO/X86/empty-object.ll:2
+; Test to ensure lto2 will not emit empty module
+; RUN: rm -f %t.1 %t.2
+; RUN: opt -module-summary %s -o %t1.bc
----------------
rm -f %t.*
probably better


================
Comment at: llvm/test/ThinLTO/X86/empty-object.ll:9
+; Test to ensure emit empty module with -thinlto-distributed-indexes option
+; RUN: rm -f %t3
+; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t3 -r=%t1.bc,bar,px -r=%t1.bc,foo, -r=%t2.bc,foo,px -save-temps -thinlto-distributed-indexes
----------------
rm -f %t3.*
?


================
Comment at: llvm/test/ThinLTO/X86/empty-object.ll:14
+
+; CHECK-NOT: bar
+; CHECK-NOT: foo
----------------
Is there a missing FileCheck corresponding to these?


================
Comment at: llvm/tools/llvm-lto2/llvm-lto2.cpp:289
   Conf.PTO.SLPVectorization = Conf.OptLevel > 1;
+  Conf.AlwaysEmitRegularLTOObj = ThinLTODistributedIndexes;
 
----------------
Why this change? This is inconsistent with the linker behavior under the corresponding option (e.g. gold plugin's thinlto-index-only).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78988