ThinLTO/ModuleLinker: add a flag to not always pull-in linkonce when performing importing

[mehdi_amini]

Summary

The function importer already decided what symbols need to be pulled
in. Also these magically added ones will not be in the export list
for the source module, which can confuse the internalizer for
instance.

[mehdi_amini]

test/Transforms/FunctionImport/funcimport.ll

89–90

I'm not happy about the extern_weak, but apparently the IRMover does not have any option to control that, which is annoying.
Rafael, any input on that?

[mehdi_amini]

Update ThinLTO generator to take this into account

[tejohnson]

lib/Linker/LinkModules.cpp

421

Noting down the concern I had expressed on IRC yesterday:

This is only legal if the LinkOnce is being eventually changed to something non-discardable, e.g. with the outstanding patches for libLTO (D18908) and gold (D18674, which applies when launching the backend threads through gold). However, even with those there are currently issues because neither patch handles linkonce that are involved with an alias. Any that will remain linkonce, because they aren't involved with an alias, or aren't covered by one of the two noted patches (e.g. using distributed back ends for ThinLTO), still need to be force imported or there will be failures (because the linkonce is discardable in its original exporting module).

My suggestion for fixing this is:

1. Cover aliases, either by allowing them to be resolved (via the proposed copy technique) or force import them when we collect the imports/exports lists from the index;

and

2. Indicate to the module linker when it is safe to ignore linkonce here, either by noting the new linkage in the index (which needs to be passed in here somehow), or use a flag to tell the module linker that it is safe to ignore linkonce here when importing (only for libLTO for now, when #1 above is addressed).

[mehdi_amini]

Update: resolve alias correctly now. Use a ModuleLinker flag to control the behavior.

[mehdi_amini]

lib/Linker/LinkModules.cpp

421

Yes this is only legal if the client code handles it correctly.
I'd like to remove totally any "magic" in general: it adds coupling and make the design too monolithic/complex. Here the client is passing a list of globals to import, I don't expect the "module linker" to import anything else.

I think we talked a few weeks ago about that: ThinLTO importing should be using only the IRMover, not the Linker.
You were concerned about decision that would be taken in the backend that wouldn't match what the ThinLink decided: this is such a good case. The key point is that you have two different logic for the decision (summary based and non-summary based). This is quite unsafe (and unsound)!

I updated the patch to add a ModuleLinker flag that controls if referenced linkonce are imported or not, and I also improve the handling for aliases.

[tejohnson]

Still think we have a potential issue, see comments embedded in the odr_resolution test case.

include/llvm/Linker/Linker.h

33

Suggest as clearer:

/// Don't force link referenced linkonce definitions.

(i.e. "linkonce definitions" instead of "definition ... linkonce")

34

s/DontLazyAddLinkone/DontLazyAddLinkonce/ (s/one/once)

lib/LTO/ThinLTOCodeGenerator.cpp

162 ↗

(On Diff #53980)

Missing space after 'if'.

163 ↗

(On Diff #53980)

It took me some time to convince myself that this was going to be correct in all cases. Specifically, I was concerned about the case where an imported function referenced an aliasee that remained linkonce (wasn't first def for linker), and we ended up trying to create a linkonce declaration (illegal) due to the change in addLazyFor. Then I saw the code in the IRMover that turns such declarations into ExternalWeak (as you imply in your question to Rafael in the test case, it doesn't need to be extern_weak, but the important point is that it isn't left linkonce_odr, and we will have a copy emitted in one of the modules for the linker to resolve it with).

I think it would be helpful to add a comment here about how this works. I.e. something like "If aliasee that remains linkonce is referenced by an imported function, the IRMover will convert the declaration to ExternalWeak, which will be satisfied at link time with the copy that was resolved to WeakODR."

lib/Linker/LinkModules.cpp

421

Yes this is only legal if the client code handles it correctly.
I'd like to remove totally any "magic" in general: it adds coupling and make the design too
monolithic/complex. Here the client is passing a list of globals to import, I don't expect the
"module linker" to import anything else.

I think we talked a few weeks ago about that: ThinLTO importing should be using only the
IRMover, not the Linker.
You were concerned about decision that would be taken in the backend that wouldn't match what
the ThinLink decided: this is such a good case. The key point is that you have two different logic
for the decision (summary based and non-summary based). This is quite unsafe (and unsound)!

It is unsound once the thin link step starts making optimizations based on the importing decisions, which is a good direction to go in. But first we have to get all of the ThinLTO clients to handle all linkonce cases correctly. This patch is a good start, and similar handling can be added to the gold-plugin and distributed backend cases. Eventually if the resolved linkage is updated in the index in all cases, we can add some assertion checking to ensure that we don't end up importing a reference to something that remains linkonce but doesn't have its definition in the GlobalsToLink set.

test/ThinLTO/X86/odr_resolution.ll

13 ↗

(On Diff #53980)

Can you remind me how we prevent the alias from being optimized this way? We have AliasSummary in DefinedGlobals, but only the aliasee are added to the GlobalInvolvedWithAlias set. I don't see where it is preventing the ODR resolution for the alias itself. If we did try to do the alias resolution, we would end up with an available_externally alias in MOD2, which is illegal.

If we do in fact prevent the ODR resolution in this case, then I think there is a different issue. If we have a reference to the alias in an imported routine, with the change to addLazyFor() we would no longer import this linkonce alias as a definition, and the IRMover will convert the imported declaration to ExternalWeak. But with all the copies of the defintion remaining linkonce, we aren't guaranteed to emit a copy anywhere, they could all be eliminated after inlining in their original modules, and we have an undef at link time.

[mehdi_amini]

test/ThinLTO/X86/odr_resolution.ll

13 ↗

(On Diff #53980)

Yes, it is done late in fixupODR() does not iterate over aliases in ThinLTOCodeGenerator, and yeah it is bad...
I'll write a test case with a linkonce_odr alias

[mehdi_amini]

Work in progress: start resolving alias ODR. Add a test that should provide better coverage.

Still need to update the function importer to remove the "magic" there, and think a bit more about every possible case.

[• rafael]

test/ThinLTO/X86/alias_resolution.ll

49 ↗

(On Diff #54170)

Is this valid? I would expect extern_weak to be only for declarations and aliases are always definitions.

[mehdi_amini]

Should be in better shape now.

[mehdi_amini]

Reintroduce the linker flag I remove inadvertently in the previous patch

[• rafael]

test/ThinLTO/X86/alias_resolution.ll

57 ↗

(On Diff #54237)

Are we still producing an extern_weak alias? This should be failing the verifier...

[tejohnson]

lib/LTO/ThinLTOCodeGenerator.cpp

230 ↗

(On Diff #54237)

Doesn't this mean we will end up with available_externally aliases, which would be illegal? I.e. alias was linkonce or weak and is not the first definition for linker, so NewLinkage is available_externally.

lib/Transforms/IPO/FunctionImport.cpp

184–187

The fix to move this check here instead of being later in importFunctions() seems independent from the rest of this patch - can/should it be committed separately?

tools/llvm-link/llvm-link.cpp

280

Doesn't the ODR resolution need to be done to set this flag? I don't see it being done in llvm-link.

[mehdi_amini]

lib/LTO/ThinLTOCodeGenerator.cpp

160 ↗

(On Diff #54234)

(This code was removed)

tools/llvm-link/llvm-link.cpp

280

I'm not sure what you'd see to resolve here, this is the mode where llvm-link is instructed on the command line to import a specific list of functions.

[tejohnson]

tools/llvm-link/llvm-link.cpp

280

Ah yes, nevermind. Just the other two comments/questions then.

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

184–187

r266845

[mehdi_amini]

Update, after splitting as much as possible in D19308.

This now only adds the flag to the Linker.

[tejohnson]

Thanks for splitting this out - it made me see what I missed before about where the new flag is being set. See comments below.

Also, should this be marked as dependent on D19308?

lib/Transforms/IPO/FunctionImport.cpp

431

This should be passed down from the client, not hardwired here. E.g. this resolution is not yet done in gold. E.g. pass down to importFunctions() or to FunctionImporter constructor.

test/Linker/funcimport2.ll

6

Looks like this change should be part of D19308 instead?

test/ThinLTO/X86/Inputs/alias_resolution.ll

5 ↗

(On Diff #54319)

Looks like this was accidentally left in this patch (test/ThinLTO/X86/alias_resolution.ll is gone). Should this test now be part of D19308?

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

431

Will update

test/Linker/funcimport2.ll

6

I don't think so: it is testing that we don't import foo (we're still seeing the original definition). So it is not about the resolution in D19308

test/ThinLTO/X86/Inputs/alias_resolution.ll

5 ↗

(On Diff #54319)

Yes,

[tejohnson]

test/Linker/funcimport2.ll

6

Ah, I see. Thought this was testing a linkage change, missed the fact that it is already available_externally in this file.

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

431

I've been trying to add a flag to ComputeCrossModuleImport to force-import referenced discardable function, but it is very intrusive to handle.
I also tried to do it as a post-pass, but this does not work (it really can't).

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

At this point, the easiest route is to force turn linkonce into weak unconditionally in clients.

[tejohnson]

lib/Transforms/IPO/FunctionImport.cpp

431

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

Why? I thought the very reason for adding the flag was because some clients don't currently do the linkonce resolution (and therefore can't do optimization based on the import/export list in the thin link step).

Otherwise there was no reason to add a flag and change addLazyFor to check that instead of whether we are doing importing.

[mehdi_amini]

Thread the flag to the client of the importer, and default it to true for the functionimportpass.

(also remove the wrong alias test case from this patch)

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

431

I thought the very reason for adding the flag was because some clients don't currently do the linkonce resolution (and therefore can't do optimization based on the import/export list in the thin link step).
Otherwise there was no reason to add a flag and change addLazyFor to check that instead of whether we are doing importing.

That's a good point indeed: I should not add the flag to the ModuleLinker at all... (I am less reluctant to special-case the ModuleLinker, because it is already special-cased all around).

Passing the flag to the FunctionImporter is wrong: this conflicts with the Import/Export list, so I don't want to go this route.

Why?

Here is the 3 components I try to isolate: Linker level resolution, Importing decision, and actual Importing

1. The linker layer is specific (gold plugin, ThinLTOCodeGenerator, etc.), even though at some point I think we could have a common interface that'd call back into the linker somehow.

This layer resolves weak and linkonce (TODO: this is not communicated to the next layer right now), and indicates what can be internalized and what needs to be preserved *outside of the ThinLTO partition*. libLTO (include/llvm-c/lto.h) should implement the API to pass information from this layer to the next one.

2. The ThinLTO importing decision layer, which is the *only* place were you decide what is imported/exported from a module

3. Compute linkage adjustments using information from 1 and 2.

4. The actual IR moving/update, using information from 1/2/3. I see it as a "thin" FunctionImporter that drives the IRMover using the information from the previous layer.

We're not there yet, but going in this direction at least, and I keep fighting the existing machinery to split out these layer.
The ThinLTOCodeGenerator is somehow piling work-around for these, but I hope to be able to refactor it heavily to extract pieces at some point.

This patch is moving something that happens during "4" towards the upper layers, the flag forces to keep this "feature" in the layer 4 (without good reason actually).

[tejohnson]

LGTM thanks

[mehdi_amini]

Rebase/restore the test

[tejohnson]

In D19096#406930, @joker.eph wrote:

Rebase/restore the test

I thought test/ThinLTO/X86/alias_resolution.ll was supposed to move to D19308?

[mehdi_amini]

Oh I moved the test in the wrong patch...

[mehdi_amini]

Re-upload the right patch, remove the wrong test!

[tejohnson]

In D19096#406940, @joker.eph wrote:

Re-upload the right patch, remove the wrong test!

You might have noticed this already but FYI it is still not the right version (looks like an old version)

[mehdi_amini]

Hopefully this is the right one...

[tejohnson]

In D19096#406958, @joker.eph wrote:

Hopefully this is the right one...

LGTM

[mehdi_amini]

test/Transforms/FunctionImport/funcimport.ll

90

This check is broken because of the flag now, needs to add a command line flag probably...

[tejohnson]

test/Transforms/FunctionImport/funcimport.ll

90

Or check for this behavior through one of the llvm-lto based tests in test/ThinLTO/X86, not with an opt test here.

[mehdi_amini]

Update with a command line flag to allows the test to pass.

[mehdi_amini]

Add a check that without the flag, the linkonce_odr is imported as expected

[mehdi_amini]

test/Transforms/FunctionImport/funcimport.ll

90

Already implemented the flag in the meantime, are you OK with the current patch?

[tejohnson]

test/Transforms/FunctionImport/funcimport.ll

90

Yep, thanks.

[mehdi_amini]

r266948

[mehdi_amini]

r266948

include/llvm/Linker/Linker.h

 class Linker {
   IRMover Mover;
 
 public:
   enum Flags {
     None = 0,
     OverrideFromSrc = (1 << 0),
     LinkOnlyNeeded = (1 << 1),
-    InternalizeLinkedSymbols = (1 << 2)
+    InternalizeLinkedSymbols = (1 << 2),
+    /// Don't force link referenced linkonce definitions, import declaration.
+    DontForceLinkLinkonceODR = (1 << 3)
+
   };
 
   Linker(Module &M);
 
   /// \brief Link \p Src into the composite.
   ///
   /// Passing OverrideSymbols as true will have symbols from Src
   /// shadow those in the Dest.

include/llvm/Transforms/IPO/FunctionImport.h

public:
 
   /// Create a Function Importer.
   FunctionImporter(
       const ModuleSummaryIndex &Index,
       std::function<std::unique_ptr<Module>(StringRef Identifier)> ModuleLoader)
       : Index(Index), ModuleLoader(ModuleLoader) {}
 
   /// Import functions in Module \p M based on the supplied import list.
-  bool importFunctions(Module &M, const ImportMapTy &ImportList);
+  /// \p ForceImportReferencedDiscardableSymbols will set the ModuleLinker in
+  /// a mode where referenced discarable symbols in the source modules will be
+  /// imported as well even if they are not present in the ImportList.
+  bool importFunctions(Module &M, const ImportMapTy &ImportList,
+                       bool ForceImportReferencedDiscardableSymbols = false);
 
 private:
   /// The summaries index used to trigger importing.
   const ModuleSummaryIndex &Index;
 
   /// Factory function to load a Module for a given identifier
   std::function<std::unique_ptr<Module>(StringRef Identifier)> ModuleLoader;
 };

lib/Linker/LinkModules.cpp

class ModuleLinker {
   DenseSet<const GlobalValue *> *GlobalsToImport;
 
   /// Used as the callback for lazy linking.
   /// The mover has just hit GV and we have to decide if it, and other members
   /// of the same comdat, should be linked. Every member to be linked is passed
   /// to Add.
   void addLazyFor(GlobalValue &GV, IRMover::ValueAdder Add);
 
+  bool shouldLinkReferencedLinkOnce() {
+    return !(Flags & Linker::DontForceLinkLinkonceODR);
+  }
   bool shouldOverrideFromSrc() { return Flags & Linker::OverrideFromSrc; }
   bool shouldLinkOnlyNeeded() { return Flags & Linker::LinkOnlyNeeded; }
   bool shouldInternalizeLinkedSymbols() {
     return Flags & Linker::InternalizeLinkedSymbols;
   }
 
   bool shouldLinkFromSource(bool &LinkFromSrc, const GlobalValue &Dest,
                             const GlobalValue &Src);
bool ModuleLinker::linkIfNeeded(GlobalValue &GV) {
   if (DGV && shouldLinkFromSource(LinkFromSrc, *DGV, GV))
     return true;
   if (LinkFromSrc)
     ValuesToLink.insert(&GV);
   return false;
 }
 
 void ModuleLinker::addLazyFor(GlobalValue &GV, IRMover::ValueAdder Add) {
+  if (!shouldLinkReferencedLinkOnce())
+    // For ThinLTO we don't import more than what was required.
+    // The client has to guarantee that the linkonce will be availabe at link
+    // time (by promoting it to weak for instance).
+    return;
+
   // Add these to the internalize list
   if (!GV.hasLinkOnceLinkage())
     return;
 
   if (shouldInternalizeLinkedSymbols())
     Internalize.insert(GV.getName());
   Add(GV);
 

lib/Transforms/IPO/FunctionImport.cpp

ImportInstrFactor("import-instr-evolution-factor", cl::init(0.7),
                       cl::Hidden, cl::value_desc("x"),
                       cl::desc("As we import functions, multiply the "
                                "`import-instr-limit` threshold by this factor "
                                "before processing newly imported functions"));
 
 static cl::opt<bool> PrintImports("print-imports", cl::init(false), cl::Hidden,
                                   cl::desc("Print imported functions"));
 
+// Temporary allows the function import pass to disable always linking
+// referenced discardable symbols.
+static cl::opt<bool>
+    DontForceImportReferencedDiscardableSymbols("disable-force-link-odr",
+                                                cl::init(false), cl::Hidden);
+
 // Load lazily a module from \p FileName in \p Context.
 static std::unique_ptr<Module> loadFile(const std::string &FileName,
                                         LLVMContext &Context) {
   SMDiagnostic Err;
   DEBUG(dbgs() << "Loading '" << FileName << "'\n");
   // Metadata isn't loaded until functions are imported, to minimize
   // the memory overhead.
   std::unique_ptr<Module> Result =
if (!CalleeSummary) {
       DEBUG(dbgs() << "ignored! No qualifying callee with summary found.\n");
       continue;
     }
     // "Resolve" the summary, traversing alias,
     const FunctionSummary *ResolvedCalleeSummary;
     if (isa<AliasSummary>(CalleeSummary)) {
       ResolvedCalleeSummary = cast<FunctionSummary>(
           &cast<AliasSummary>(CalleeSummary)->getAliasee());
       assert(
           GlobalValue::isLinkOnceODRLinkage(ResolvedCalleeSummary->linkage()) &&
           "Unexpected alias to a non-linkonceODR in import list");
     } else
       ResolvedCalleeSummary = cast<FunctionSummary>(CalleeSummary);
 
     assert(ResolvedCalleeSummary->instCount() <= Threshold &&
            "selectCallee() didn't honor the threshold");
 
     auto ExportModulePath = ResolvedCalleeSummary->modulePath();
     auto &ProcessedThreshold = ImportsForModule[ExportModulePath][GUID];
     /// Since the traversal of the call graph is DFS, we can revisit a function
#ifndef NDEBUG
   }
 #endif
 }
 
 // Automatically import functions in Module \p DestModule based on the summaries
 // index.
 //
 bool FunctionImporter::importFunctions(
-    Module &DestModule, const FunctionImporter::ImportMapTy &ImportList) {
+    Module &DestModule, const FunctionImporter::ImportMapTy &ImportList,
+    bool ForceImportReferencedDiscardableSymbols) {
   DEBUG(dbgs() << "Starting import for Module "
                << DestModule.getModuleIdentifier() << "\n");
   unsigned ImportedCount = 0;
 
   // Linker that will be used for importing function
   Linker TheLinker(DestModule);
   // Do the actual import of functions now, one Module at a time
   std::set<StringRef> ModuleNameOrderedList;
if (renameModuleForThinLTO(*SrcModule, Index, &GlobalsToImport))
       return true;
 
     if (PrintImports) {
       for (const auto *GV : GlobalsToImport)
         dbgs() << DestModule.getSourceFileName() << ": Import " << GV->getName()
                << " from " << SrcModule->getSourceFileName() << "\n";
     }
 
-    if (TheLinker.linkInModule(std::move(SrcModule), Linker::Flags::None,
-                               &GlobalsToImport))
+    // Instruct the linker that the client will take care of linkonce resolution
+    unsigned Flags = Linker::Flags::None;
+    if (!ForceImportReferencedDiscardableSymbols)
+      Flags |= Linker::Flags::DontForceLinkLinkonceODR;
+
+    if (TheLinker.linkInModule(std::move(SrcModule), Flags, &GlobalsToImport))
       report_fatal_error("Function Import: link error");
 
     ImportedCount += GlobalsToImport.size();
   }
 
   NumImported += ImportedCount;
 
   DEBUG(dbgs() << "Imported " << ImportedCount << " functions for Module "
if (renameModuleForThinLTO(M, *Index, nullptr)) {
       return false;
     }
 
     // Perform the import now.
     auto ModuleLoader = [&M](StringRef Identifier) {
       return loadFile(Identifier, M.getContext());
     };
     FunctionImporter Importer(*Index, ModuleLoader);
-    return Importer.importFunctions(M, ImportList);
+    return Importer.importFunctions(
+        M, ImportList, !DontForceImportReferencedDiscardableSymbols);
   }
 };
 } // anonymous namespace
 
 char FunctionImportPass::ID = 0;
 INITIALIZE_PASS_BEGIN(FunctionImportPass, "function-import",
                       "Summary Based Function Import", false, false)
 INITIALIZE_PASS_END(FunctionImportPass, "function-import",
                     "Summary Based Function Import", false, false)
 
 namespace llvm {
 Pass *createFunctionImportPass(const ModuleSummaryIndex *Index = nullptr) {
   return new FunctionImportPass(Index);
 }
 }

test/Linker/funcimport2.ll

 ; RUN: opt -module-summary %s -o %t1.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport2.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -o %t3 %t1.bc %t2.bc
 ; RUN: llvm-link -import=bar:%t2.bc %t1.bc -summary-index=%t3.thinlto.bc -S | FileCheck %s
 
-; CHECK: define linkonce_odr hidden void @foo() {
+; CHECK: define available_externally hidden void @foo() {
 define available_externally hidden void @foo() {
     ret void
 }
 
 declare void @bar()
 
 define void @caller() {
   call void @bar()
   call void @foo()
   ret void
 }

test/Transforms/FunctionImport/Inputs/funcimport.ll

entry:
   ret void
 }
 
 define internal void @staticfunc2() #0 {
 entry:
   ret void
 }
 
+define void @referencelargelinkonce() #0 {
+entry:
+  call void @linkonceodr()
+  ret void
+}
+
+; A large enough linkonce_odr function that should never be imported
+define linkonce_odr void @linkonceodr() #0 {
+entry:
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  call void @globalfunc2()
+  ret void
+}
+
 

test/Transforms/FunctionImport/funcimport.ll

 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/funcimport.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -print-summary-global-ids -o %t3 %t.bc %t2.bc 2>&1 | FileCheck %s --check-prefix=GUID
 
 ; Do the import now
-; RUN: opt -function-import -stats -print-imports -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIMDEF
+; RUN: opt -disable-force-link-odr -function-import -stats -print-imports -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIMDEF
 ; "-stats" requires +Asserts.
 ; REQUIRES: asserts
 
 ; Test import with smaller instruction limit
-; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=5 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM5
+; RUN: opt -disable-force-link-odr -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=5 -S | FileCheck %s --check-prefix=CHECK --check-prefix=INSTLIM5
 ; INSTLIM5-NOT: @staticfunc.llvm.
 
+; Test import with smaller instruction limit and without the -disable-force-link-odr
+; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -import-instr-limit=5 -S | FileCheck %s --check-prefix=INSTLIM5ODR
+; INSTLIM5ODR: define linkonce_odr void @linkonceodr()
+
+
 define i32 @main() #0 {
 entry:
   call void (...) @weakalias()
   call void (...) @analias()
   call void (...) @linkoncealias()
   %call = call i32 (...) @referencestatics()
   %call1 = call i32 (...) @referenceglobals()
   %call2 = call i32 (...) @referencecommon()
   call void (...) @setfuncptr()
   call void (...) @callfuncptr()
   call void (...) @weakfunc()
+  call void (...) @referencelargelinkonce()
   ret i32 0
 }
 
 ; Won't import weak alias
 ; CHECK-DAG: declare void @weakalias
 declare void @weakalias(...) #1
 
 ; Cannot create an alias to available_externally

 declare void @callfuncptr(...) #1
 
 ; Ensure that all uses of local variable @P which has used in setfuncptr
 ; and callfuncptr are to the same promoted/renamed global.
 ; CHECK-DAG: @P.llvm.{{.*}} = external hidden global void ()*
 ; CHECK-DAG: %0 = load void ()*, void ()** @P.llvm.
 ; CHECK-DAG: store void ()* @staticfunc2.llvm.{{.*}}, void ()** @P.llvm.
 
+; Ensure that @referencelargelinkonce definition is pulled in, but later we
+; also check that the linkonceodr function is not.
+; CHECK-DAG: define available_externally void @referencelargelinkonce()
+; INSTLIM5-DAG: declare void @linkonceodr()
+declare void @referencelargelinkonce(...)
+
 ; Won't import weak func
 ; CHECK-DAG: declare void @weakfunc(...)
 declare void @weakfunc(...) #1
 
 ; INSTLIMDEF-DAG: Import funcwithpersonality
 ; INSTLIMDEF-DAG: define available_externally hidden void @funcwithpersonality.llvm.{{.*}}() personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
 ; INSTLIM5-DAG: declare hidden void @funcwithpersonality.llvm.{{.*}}()
 
 ; INSTLIMDEF-DAG: Import globalfunc2
-; INSTLIMDEF-DAG: 11 function-import - Number of functions imported
+; INSTLIMDEF-DAG: 13 function-import - Number of functions imported
 
 ; The actual GUID values will depend on path to test.
 ; GUID-DAG: GUID {{.*}} is weakalias
 ; GUID-DAG: GUID {{.*}} is referenceglobals
 ; GUID-DAG: GUID {{.*}} is weakfunc
 ; GUID-DAG: GUID {{.*}} is main
 ; GUID-DAG: GUID {{.*}} is referencecommon
 ; GUID-DAG: GUID {{.*}} is analias

tools/llvm-link/llvm-link.cpp

for (auto &GlobalsToImportPerModule : ModuleToGlobalsToImportMap) {
     // now, before linking it (otherwise this will be a noop).
     SrcModule->materializeMetadata();
     UpgradeDebugInfo(*SrcModule);
 
     // Linkage Promotion and renaming
     if (renameModuleForThinLTO(*SrcModule, *Index, &GlobalsToImport))
       return true;
 
-    if (L.linkInModule(std::move(SrcModule), Linker::Flags::None,
-                       &GlobalsToImport))
+    // Instruct the linker to not automatically import linkonce defintion.
+    unsigned Flags = Linker::Flags::DontForceLinkLinkonceODR;
+
+    if (L.linkInModule(std::move(SrcModule), Flags, &GlobalsToImport))
       return false;
   }
 
   return true;
 }
 
 static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
                       const cl::list<std::string> &Files,