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

[Kuan Hsu Chen (Zakk) via Phabricator via llvm-commits]

khchen marked 3 inline comments as done.
khchen added a comment.

Hi @tejohnson
I'm very appreciate your help and comment, 
because I'm not LTO expert, so if you have any good idea and concern in implementation
Please don't hesitate to correct me, thanks.



================
Comment at: llvm/include/llvm/LTO/Config.h:71
+  /// build system which want to know a priori all possible output files.
+  bool EmitEmptyObj = false;
+
----------------
tejohnson wrote:
> Since this only applies to the regular LTO object, would be better to call this something more specific, like AlwaysEmitRegularLTOObj.
okay, I will fix it.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:503
+        Mod->getModuleInlineAsm() == "" && !C.EmitEmptyObj)
+      HookOnly = true;
+    codegen(C, TM.get(), AddStream, 0, *Mod, HookOnly);
----------------
tejohnson wrote:
> Just to clarify my assumption, you are doing this here rather than down in codegen because for ThinLTO modules we want to produce an object even when an empty module is explicitly linked for some reason?
> 
> Rather than that series of checks, I think it would be better to just include a flag on the RegularLTOState to indicate whether addRegularLTO was ever called. It's clearer and probably less fragile.
> 
> Also - what is the value of calling the hook if it is an empty default module that wasn't added by the link, and that we aren't going to code gen?
> Just to clarify my assumption, you are doing this here rather than down in codegen because for ThinLTO modules we want to produce an object even when an empty module is explicitly linked for some reason?

There are 3 tests got failed if moving this code into codegen, 
1.  LLVM :: ThinLTO/X86/empty_module_with_cache.ll
https://github.com/llvm/llvm-project/blob/master/llvm/test/ThinLTO/X86/empty_module_with_cache.ll#L4
In current caching mechanism, it should work even if the module is empty.

2.  Clang :: CodeGen/thinlto_backend.ll
https://github.com/llvm/llvm-project/blob/master/clang/test/CodeGen/thinlto_backend.ll#L27    
The output is empty file (not empty obj file) which llvm-nm can not recognize it. 

3.  LLVM :: tools/gold/X86/cache.ll
https://github.com/llvm/llvm-project/blob/master/llvm/test/tools/gold/X86/cache.ll#L27
after opt phase, %t2 (cache.ll.tmp2.o.4.opt.bc) is empty module, so there is no %t2 cache file. 


> Rather than that series of checks, I think it would be better to just include a flag on the RegularLTOState to indicate whether addRegularLTO was ever called. It's clearer and probably less fragile.
okay, I will try it.

> Also - what is the value of calling the hook if it is an empty default module that wasn't added by the link, and that we aren't going to code gen?
Sorry, I don't understand the question, HookOnly is used to suppress codegen but still emit temp file for debugging, than the gold ld wasn't store empty Buf[Task] as file and link it.



================
Comment at: llvm/tools/gold/gold-plugin.cpp:1058
   size_t MaxTasks = Lto->getMaxTasks();
-  std::vector<std::pair<SmallString<128>, bool>> Files(MaxTasks);
-
-  auto AddStream =
-      [&](size_t Task) -> std::unique_ptr<lto::NativeObjectStream> {
-    Files[Task].second = !SaveTemps;
-    int FD = getOutputFileName(Filename, /* TempOutFile */ !SaveTemps,
-                               Files[Task].first, Task);
-    return std::make_unique<lto::NativeObjectStream>(
-        std::make_unique<llvm::raw_fd_ostream>(FD, true));
-  };
+  std::vector<SmallString<0>> Buf(MaxTasks);
+  std::vector<std::unique_ptr<MemoryBuffer>> CacheFiles(MaxTasks);
----------------
tejohnson wrote:
> Why are all these changes needed, since you are addressing this within LTO to not emit the empty object file?
original implementation always generates file including empty module which generate empty file. (not object file)

This implementation reference [[ https://github.com/llvm/llvm-project/blob/master/lld/ELF/LTO.cpp#L274 | lld ]].
In https://reviews.llvm.org/D78035#inline-719821 as you mentioned we can "possibly make the Files array a tuple with another bool flag indicating whether the obj file buffer was empty." but I could not find a good place to store empty info when calling lto::backend.

any suggestion?




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78988