[LTO] Prevent asm undefined references to be dropped from the output

[davide]

Summary

As discussed on the list (incidentally I already had a patch for that).

[mehdi_amini]

Test?
ThinLTO Path?

[tejohnson]

Thanks! With a few changes noted below this can be fixed for ThinLTO as well.

lib/LTO/LTO.cpp

353 ↗

(On Diff #71526)

Do this in addSymbolToGlobalRes instead, so it is added for ThinLTO as well

lib/LTO/LTOBackend.cpp

291 ↗

(On Diff #71526)

Ditto in thinBackend below

[mehdi_amini]

Also it is not clear to me why we track this globally instead of per-file? What is an ASM reference a local symbol for instance?

[davide]

lib/LTO/LTOBackend.cpp

291 ↗

(On Diff #71526)

thinBackend() is also used by clang and it's not entirely clear to me what should I pass to it as AsmUndefinedReferences. If you know that off of your mind, I can change it. Otherwise I can commit the patch as is for full LTO and you can modify for thin accordingly (sorry, my knowledge of ThinLTO is still rather limited, and I don't want to delay this further).

[tejohnson]

In D24617#543921, @mehdi_amini wrote:

Also it is not clear to me why we track this globally instead of per-file? What is an ASM reference a local symbol for instance?

Yeah at least for ThinLTO perhaps we should call IRObjectFile::CollectAsmUndefinedRefs in the thinBackend to collect this for each module? That would address the issue Davide mentioned about that being called from clang (distributed backends case). For regular LTO it is essentially global since we merge the module.

lib/LTO/LTO.cpp

310 ↗

(On Diff #71526)

Shouldn't this be: an undefined reference "of" a symbol "used" in asm?

[mehdi_amini]

The problem is that after merge, if two local symbols have a name conflict in LTO, one of them will be renamed.
I'd like to see a test with such cases.

[tejohnson]

In D24617#543995, @mehdi_amini wrote:

The problem is that after merge, if two local symbols have a name conflict in LTO, one of them will be renamed.
I'd like to see a test with such cases.

Hmm, so how would we catch/handle this? The uses from module level assembly won't be renamed, right? (I know that is an issue for inline assembly). That seems orthogonal to the issue of making sure their defs are not dropped by updating llvm.compiler.used appropriately (which could be fixed by doing what I suggested for ThinLTO - call CollectAsmUndefinedRefs from the backend).

[mehdi_amini]

That seems orthogonal to the issue of making sure their defs are not dropped by updating llvm.compiler.used appropriately which could be fixed by doing what I suggested for ThinLTO - call CollectAsmUndefinedRefs from the backend).

Right, this would solves it (but for the lack of renaming).

Another way is to do it on the individual modules when they're loaded, pre-merge.

[davide]

Teresa, is this what you had in mind? Also added a ThinLTO test, hopefully it's correct.

[mehdi_amini]

lib/LTO/LTO.cpp

368 ↗

(On Diff #71552)

Why don't you update compiler.used here before merging? This way the StringSet could be local to this function and not global to the LTO class/lifetime I believe.

[tejohnson]

In D24617#544082, @davide wrote:

Teresa, is this what you had in mind? Also added a ThinLTO test, hopefully it's correct.

Yes, thanks. One suggestion to merge the new tests below.

test/tools/gold/X86/asm_undefined2_thin.ll

6 ↗

(On Diff #71552)

I suppose since the test is basically the same you could copy these RUN lines into asm_undefined2.ll so it would test both regular and thin LTO.

[davide]

Unified tests as per Teresa's request.

[davide]

Update after Teresa's comments.

[mehdi_amini]

LGTM.

With a llvm-lto2 version of the test, it'll be perfect ;)

[davide]

In D24617#544936, @mehdi_amini wrote:

LGTM.

With a llvm-lto2 version of the test, it'll be perfect ;)

I tried to write that but there are issues providing the resolution because IRObjectFile will report the same symbol twice. I added a comment to https://llvm.org/bugs/show_bug.cgi?id=30396 so that we can add a test for this once we fix IRObjectFile =)

[mehdi_amini]

Thanks for adding the tracking of this!

[tejohnson]

Thanks!

llvm/trunk/lib/LTO/LTOBackend.cpp

 #include "llvm/Analysis/LoopPassManager.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Bitcode/ReaderWriter.h"
 #include "llvm/IR/LegacyPassManager.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/IR/Verifier.h"
 #include "llvm/LTO/LTO.h"
+#include "llvm/LTO/legacy/UpdateCompilerUsed.h"
 #include "llvm/MC/SubtargetFeature.h"
 #include "llvm/Passes/PassBuilder.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/TargetRegistry.h"
 #include "llvm/Support/ThreadPool.h"
 #include "llvm/Target/TargetMachine.h"
 #include "llvm/Transforms/IPO.h"
Expected<const Target *> initAndLookupTarget(Config &C, Module &Mod) {
   const Target *T = TargetRegistry::lookupTarget(Mod.getTargetTriple(), Msg);
   if (!T)
     return make_error<StringError>(Msg, inconvertibleErrorCode());
   return T;
 }
 
 }
 
+static void handleAsmUndefinedRefs(Module &Mod, TargetMachine &TM) {
+  // Collect the list of undefined symbols used in asm and update
+  // llvm.compiler.used to prevent optimization to drop these from the output.
+  StringSet<> AsmUndefinedRefs;
+  object::IRObjectFile::CollectAsmUndefinedRefs(
+      Triple(Mod.getTargetTriple()), Mod.getModuleInlineAsm(),
+      [&AsmUndefinedRefs](StringRef Name, object::BasicSymbolRef::Flags Flags) {
+        if (Flags & object::BasicSymbolRef::SF_Undefined)
+          AsmUndefinedRefs.insert(Name);
+      });
+  updateCompilerUsed(Mod, TM, AsmUndefinedRefs);
+}
+
 Error lto::backend(Config &C, AddOutputFn AddOutput,
                    unsigned ParallelCodeGenParallelismLevel,
                    std::unique_ptr<Module> Mod) {
   Expected<const Target *> TOrErr = initAndLookupTarget(C, *Mod);
   if (!TOrErr)
     return TOrErr.takeError();
 
   std::unique_ptr<TargetMachine> TM =
       createTargetMachine(C, Mod->getTargetTriple(), *TOrErr);
 
+  handleAsmUndefinedRefs(*Mod, *TM);
+
   if (!C.CodeGenOnly)
     if (!opt(C, TM.get(), 0, *Mod, /*IsThinLto=*/false))
       return Error();
 
   if (ParallelCodeGenParallelismLevel == 1) {
     codegen(C, TM.get(), getUncachedOutputWrapper(AddOutput, 0), 0, *Mod);
   } else {
     splitCodeGen(C, TM.get(), AddOutput, ParallelCodeGenParallelismLevel,
Error lto::thinBackend(Config &Conf, unsigned Task, AddOutputFn AddOutput,
                        MapVector<StringRef, MemoryBufferRef> &ModuleMap) {
   Expected<const Target *> TOrErr = initAndLookupTarget(Conf, Mod);
   if (!TOrErr)
     return TOrErr.takeError();
 
   std::unique_ptr<TargetMachine> TM =
       createTargetMachine(Conf, Mod.getTargetTriple(), *TOrErr);
 
+  handleAsmUndefinedRefs(Mod, *TM);
+
   if (Conf.CodeGenOnly) {
     codegen(Conf, TM.get(), AddOutput, Task, Mod);
     return Error();
   }
 
   if (Conf.PreOptModuleHook && !Conf.PreOptModuleHook(Task, Mod))
     return Error();
 

llvm/trunk/test/tools/gold/X86/asm_undefined2.ll

+; RegularLTO testcase
+; RUN: llvm-as %s -o %t.o
+; RUN: %gold -shared -m elf_x86_64 -o %t2 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN: %t.o --plugin-opt=save-temps -upatatino
+; RUN: llvm-dis < %t2.0.5.precodegen.bc | FileCheck %s
+
+; ThinLTO testcase
+; RUN: opt -module-summary %s -o %t.o
+; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold.so \
+; RUN:     --plugin-opt=save-temps \
+; RUN:     --plugin-opt=thinlto -o %t2 %t.o
+; RUN: llvm-dis < %t.o.5.precodegen.bc | FileCheck %s
+
+; Check that foo is properly appended to llvm.compiler.used
+; CHECK: @llvm.compiler.used = appending global [1 x i8*] [i8* bitcast (void ()* @foo to i8*)], section "llvm.metadata"
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm ".global patatino"
+module asm ".equ patatino, foo"
+
+declare void @patatino()
+
+define void @foo() {
+  call void @patatino()
+  ret void
+}