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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/LTO/LTOBackend.cpp:503
+        Mod->getModuleInlineAsm() == "" && !C.EmitEmptyObj)
+      HookOnly = true;
+    codegen(C, TM.get(), AddStream, 0, *Mod, HookOnly);
----------------
khchen wrote:
> 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.
> 
> 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.

What I meant was that I don't think we need to execute the hook, if this is an empty combined module that doesn't correspond to any incoming linked module and that we aren't going to codegen. I.e. don't even call codegen in this case.


================
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);
----------------
khchen wrote:
> 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?
> 
> 
That was an alternate suggestion, instead of changing LTO, since you mentioned lld worked fine. Since you are suppressing the object file in LTO, is any change needed here? AddStream isn't called until down in codegen(), and with your change that isn't being executed. So presumably no change is needed here to prevent the empty object from being passed back to gold. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78988