[ThinLTO] Don't link module level assembly when importing

[tejohnson]

Summary

Module inline asm was always being linked/concatenated
when running the IRLinker. This is correct for full LTO but not when
we are importing for ThinLTO, as it can result in multiply defined
symbols when the module asm defines a global symbol.

In order to test with llvm-lto2, I had to work around PR30396,
where a symbol that is defined in module assembly but defined in the
LLVM IR appears twice. Added workaround to llvm-lto2 with a FIXME.

Depends on D25345.

Fixes PR30610.

[mehdi_amini]

LGTM.

[tejohnson]

As discussed on IRC, change this patch to just address the immediate issue reported for PR30610 (concatenation of module asm during importing).

We plan to address module asm and inline assembly a different way (per summary record flag to block importing).

llvm/trunk/include/llvm/Linker/IRMover.h

public:
 
   /// Move in the provide values in \p ValuesToLink from \p Src.
   ///
   /// - \p AddLazyFor is a call back that the IRMover will call when a global
   ///   value is referenced by one of the ValuesToLink (transitively) but was
   ///   not present in ValuesToLink. The GlobalValue and a ValueAdder callback
   ///   are passed as an argument, and the callback is expected to be called
   ///   if the GlobalValue needs to be added to the \p ValuesToLink and linked.
+  /// - \p LinkModuleInlineAsm is true if the ModuleInlineAsm string in Src
+  ///   should be linked with (concatenated into) the ModuleInlineAsm string
+  ///   for the destination module. It should be true for full LTO, but not
+  ///   when importing for ThinLTO, otherwise we can have duplicate symbols.
   Error move(std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
-             std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor);
+             std::function<void(GlobalValue &GV, ValueAdder Add)> AddLazyFor,
+             bool LinkModuleInlineAsm);
   Module &getModule() { return Composite; }
 
 private:
   Module &Composite;
   IdentifiedStructTypeSet IdentifiedStructTypes;
   MDMapT SharedMDs; ///< A Metadata map to use for all calls to \a move().
 };
 
 } // End llvm namespace
 
 #endif

llvm/trunk/lib/LTO/LTO.cpp

if (Sym.getFlags() & object::BasicSymbolRef::SF_Common) {
       CommonRes.Prevailing |= Res.Prevailing;
     }
 
     // FIXME: use proposed local attribute for FinalDefinitionInLinkageUnit.
   }
   assert(ResI == Res.end());
 
   return RegularLTO.Mover->move(Obj->takeModule(), Keep,
-                                [](GlobalValue &, IRMover::ValueAdder) {});
+                                [](GlobalValue &, IRMover::ValueAdder) {},
+                                /* LinkModuleInlineAsm */ true);
 }
 
 // Add a ThinLTO object to the link.
 Error LTO::addThinLTO(std::unique_ptr<InputFile> Input,
                       ArrayRef<SymbolResolution> Res) {
   Module &M = Input->Obj->getModule();
   SmallPtrSet<GlobalValue *, 8> Used;
   collectUsedGlobalVariables(M, Used, /*CompilerUsed*/ false);

llvm/trunk/lib/Linker/IRMover.cpp

class IRLinker {
   DenseSet<GlobalValue *> ValuesToLink;
   std::vector<GlobalValue *> Worklist;
 
   void maybeAdd(GlobalValue *GV) {
     if (ValuesToLink.insert(GV).second)
       Worklist.push_back(GV);
   }
 
+  /// Flag whether the ModuleInlineAsm string in Src should be linked with
+  /// (concatenated into) the ModuleInlineAsm string for the destination
+  /// module. It should be true for full LTO, but not when importing for
+  /// ThinLTO, otherwise we can have duplicate symbols.
+  bool LinkModuleInlineAsm;
+
   /// Set to true when all global value body linking is complete (including
   /// lazy linking). Used to prevent metadata linking from creating new
   /// references.
   bool DoneLinkingBodies = false;
 
   /// The Error encountered during materialization. We use an Optional here to
   /// avoid needing to manage an unconsumed success value.
   Optional<Error> FoundError;
class IRLinker {
   GlobalValue *copyGlobalAliasProto(const GlobalAlias *SGA);
 
   void linkNamedMDNodes();
 
 public:
   IRLinker(Module &DstM, MDMapT &SharedMDs,
            IRMover::IdentifiedStructTypeSet &Set, std::unique_ptr<Module> SrcM,
            ArrayRef<GlobalValue *> ValuesToLink,
-           std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor)
+           std::function<void(GlobalValue &, IRMover::ValueAdder)> AddLazyFor,
+           bool LinkModuleInlineAsm)
       : DstM(DstM), SrcM(std::move(SrcM)), AddLazyFor(std::move(AddLazyFor)),
         TypeMap(Set), GValMaterializer(*this), LValMaterializer(*this),
-        SharedMDs(SharedMDs),
+        SharedMDs(SharedMDs), LinkModuleInlineAsm(LinkModuleInlineAsm),
         Mapper(ValueMap, RF_MoveDistinctMDs | RF_IgnoreMissingLocals, &TypeMap,
                &GValMaterializer),
         AliasMCID(Mapper.registerAlternateMappingContext(AliasValueMap,
                                                          &LValMaterializer)) {
     ValueMap.getMDMap() = std::move(SharedMDs);
     for (GlobalValue *GV : ValuesToLink)
       maybeAdd(GV);
   }
emitWarning("Linking two modules of different target triples: " +
                 SrcM->getModuleIdentifier() + "' is '" +
                 SrcM->getTargetTriple() + "' whereas '" +
                 DstM.getModuleIdentifier() + "' is '" + DstM.getTargetTriple() +
                 "'\n");
 
   DstM.setTargetTriple(mergeTriples(SrcTriple, DstTriple));
 
   // Append the module inline asm string.
-  if (!SrcM->getModuleInlineAsm().empty()) {
+  if (LinkModuleInlineAsm && !SrcM->getModuleInlineAsm().empty()) {
     if (DstM.getModuleInlineAsm().empty())
       DstM.setModuleInlineAsm(SrcM->getModuleInlineAsm());
     else
       DstM.setModuleInlineAsm(DstM.getModuleInlineAsm() + "\n" +
                               SrcM->getModuleInlineAsm());
   }
 
   // Loop over all of the linked values to compute type mappings.
IRMover::IRMover(Module &M) : Composite(M) {
   // destination module may be reached from the source module.
   for (auto *MD : StructTypes.getVisitedMetadata()) {
     SharedMDs[MD].reset(const_cast<MDNode *>(MD));
   }
 }
 
 Error IRMover::move(
     std::unique_ptr<Module> Src, ArrayRef<GlobalValue *> ValuesToLink,
-    std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor) {
+    std::function<void(GlobalValue &, ValueAdder Add)> AddLazyFor,
+    bool LinkModuleInlineAsm) {
   IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
-                       std::move(Src), ValuesToLink, std::move(AddLazyFor));
+                       std::move(Src), ValuesToLink, std::move(AddLazyFor),
+                       LinkModuleInlineAsm);
   Error E = TheIRLinker.run();
   Composite.dropTriviallyDeadConstantArrays();
   return E;
 }

llvm/trunk/lib/Linker/LinkModules.cpp

bool ModuleLinker::run() {
   }
 
   // FIXME: Propagate Errors through to the caller instead of emitting
   // diagnostics.
   bool HasErrors = false;
   if (Error E = Mover.move(std::move(SrcM), ValuesToLink.getArrayRef(),
                            [this](GlobalValue &GV, IRMover::ValueAdder Add) {
                              addLazyFor(GV, Add);
-                           })) {
+                           },
+                           !isPerformingImport())) {
     handleAllErrors(std::move(E), [&](ErrorInfoBase &EIB) {
       DstM.getContext().diagnose(LinkDiagnosticInfo(DS_Error, EIB.message()));
       HasErrors = true;
     });
   }
   if (HasErrors)
     return true;
 

llvm/trunk/test/ThinLTO/X86/Inputs/module_asm.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main({ i64, { i64, i8* }* } %unnamed) #0 {
+  %1 = call i32 @_simplefunction() #1
+  ret i32 %1
+}
+declare i32 @_simplefunction() #1

llvm/trunk/test/ThinLTO/X86/module_asm_glob.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/module_asm.ll -o %t2.bc
+
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=main %t1.bc %t2.bc
+; RUN: llvm-nm %t1.bc.thinlto.o | FileCheck  %s --check-prefix=NM0
+; RUN: llvm-nm %t2.bc.thinlto.o | FileCheck  %s --check-prefix=NM1
+
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,foo,plx \
+; RUN:     -r=%t1.bc,_simplefunction,pl \
+; RUN:     -r=%t2.bc,main,plx \
+; RUN:     -r=%t2.bc,_simplefunction,l
+; RUN: llvm-nm %t.o.0 | FileCheck  %s --check-prefix=NM0
+; RUN: llvm-nm %t.o.1 | FileCheck  %s --check-prefix=NM1
+
+; NM0: T foo
+; NM1-NOT: foo
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+module asm "\09.text"
+module asm "\09.globl\09foo"
+module asm "\09.align\0916, 0x90"
+module asm "\09.type\09foo,@function"
+module asm "foo:"
+module asm "\09ret "
+module asm "\09.size\09foo, .-foo"
+module asm ""
+
+declare zeroext i16 @foo() #0
+
+define i32 @_simplefunction() #1 {
+  ret i32 1
+}

llvm/trunk/tools/llvm-lto2/llvm-lto2.cpp

int main(int argc, char **argv) {
 
   bool HasErrors = false;
   for (std::string F : InputFilenames) {
     std::unique_ptr<MemoryBuffer> MB = check(MemoryBuffer::getFile(F), F);
     std::unique_ptr<InputFile> Input =
         check(InputFile::create(MB->getMemBufferRef()), F);
 
     std::vector<SymbolResolution> Res;
+    // FIXME: Workaround PR30396 which means that a symbol can appear
+    // more than once if it is defined in module-level assembly and
+    // has a GV declaration. Keep track of the resolutions found in this
+    // file and remove them from the CommandLineResolutions map afterwards,
+    // so that we don't flag the second one as missing.
+    std::map<std::string, SymbolResolution> CurrentFileSymResolutions;
     for (const InputFile::Symbol &Sym : Input->symbols()) {
       auto I = CommandLineResolutions.find({F, Sym.getName()});
       if (I == CommandLineResolutions.end()) {
         llvm::errs() << argv[0] << ": missing symbol resolution for " << F
                      << ',' << Sym.getName() << '\n';
         HasErrors = true;
       } else {
         Res.push_back(I->second);
-        CommandLineResolutions.erase(I);
+        CurrentFileSymResolutions[Sym.getName()] = I->second;
       }
     }
+    for (auto I : CurrentFileSymResolutions) {
+      auto NumErased = CommandLineResolutions.erase({F, I.first});
+      assert(NumErased > 0);
+    }
 
     if (HasErrors)
       continue;
 
     MBs.push_back(std::move(MB));
     check(Lto.add(std::move(Input), Res), F);
   }