[ThinLTO] Fix crash: transitively referenced global shouldn't be promoted

[tejohnson]

Interesting, this is pretty much the opposite of a patch I sent yesterday (D26866) to fix a Chromium build issue (PR31052).

By removing this code, we can have more failures of the type described in that bug, because the backend will decide not to promote constant unnamed_addr constants (doPromoteLocalToGlobal).

I see two fixes to both of these issues (PR31052 and the issue you are trying to fix here):

1. Disable the constant unnamed_addr promotion optimization in doPromoteLocalToGlobal
2. Change the importing logic to prevent import of anything that will cause a non-renamable value to be exported via recursive analysis of exported globals (recursively invoke canBeExternallyReferenced when looking at global variable summaries)

The disadvantage of #1, while simpler, is that we would prevent optimizations. I.e. the situation in PR26866 is from constant propagation once we have imported since the parameters of the imported function were constants used to access the imported global constant array.

[mehdi_amini]

Whatever solution we adopt, it seems that logic should be in the *import* directly, so this code should go away and the importing could then be taught to walk refs (we don't right now I believe) and import constant.

[mehdi_amini]

So my current feeling is:

• the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.
• let this patch go
• work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)

[tejohnson]

In D26880#600745, @mehdi_amini wrote:

So my current feeling is:

• the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.

The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

• let this patch go

I think you would also need this patch to go in so globalvar refs wouldn't then be marked exported, which presumably is causing an issue when the exporting (defining) module tried to promote those with sections?

• work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)

This is the right long term fix. However I am concerned about performance we may give up in the meantime (PR31052 is fixing a case where we already are getting optimized code). I'd prefer to keep the current approach in the meantime, unless benchmarking shows that it doesn't have significant impact. It would mean changing this fix to instead simply do a recursive walk of global var refs in eligibleForImport, along with my current fix for PR31052.

[mehdi_amini]

In D26880#600909, @tejohnson wrote:

In D26880#600745, @mehdi_amini wrote:

So my current feeling is:

• the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.

The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

I don't get this part?
I expect the imported to *control* what is imported, and nothing that isn't marked for import should be imported.

[mehdi_amini]

I think you would also need this patch to go in so globalvar refs wouldn't then be marked exported, which presumably is causing an issue when the exporting (defining) module tried to promote those with sections?

Yes, this is the crash with the test-case in this patch.

• work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)

This is the right long term fix. However I am concerned about performance we may give up in the meantime (PR31052 is fixing a case where we already are getting optimized code). I'd prefer to keep the current approach in the meantime, unless benchmarking shows that it doesn't have significant impact. It would mean changing this fix to instead simply do a recursive walk of global var refs in eligibleForImport, along with my current fix for PR31052.

I have a different approach: 1) this should go in in any case, because correctness first, and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.

[tejohnson]

In D26880#600927, @mehdi_amini wrote:

In D26880#600909, @tejohnson wrote:

In D26880#600745, @mehdi_amini wrote:

So my current feeling is:

• the fix for PR31052 is to disable the "magic" in IRLinker that import definition we didn't ask for.

The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

I don't get this part?
I expect the imported to *control* what is imported, and nothing that isn't marked for import should be imported.

The IRLinker necessarily needs to link in the definition of any local referenced from the module being linked. This is necessary for both Full LTO, as well as for ThinLTO if we decided not to promote and want to import a copy. For ThinLTO we control this by promoting before the IRLinker is invoked (by invoking renameModuleForThinLTO) - so if it has been promoted then the IRLinker sees a non-local and does not force the def to be linked, but if it is still local at that point we need to import a local copy.

Note that this is still going to be the same when we change the local const heuristic to be decided in the thin link instead of during the backend's renaming logic, based on a new flag to be added indicating constness. In that case the local would not be marked as a global in the index, and we will not promote in renameModuleForThinLTO (superseding the old heuristic in doPromoteLocalToGlobal), and the IRLinker will import a local copy since it will still be local.

[tejohnson]

In D26880#600928, @mehdi_amini wrote:

I think you would also need this patch to go in so globalvar refs wouldn't then be marked exported, which presumably is causing an issue when the exporting (defining) module tried to promote those with sections?

Yes, this is the crash with the test-case in this patch.

• work on the importing logic to start considering "refs" to be imported when possible / profitable (i.e. catch the performance gain we hope to have with PR31052)

This is the right long term fix. However I am concerned about performance we may give up in the meantime (PR31052 is fixing a case where we already are getting optimized code). I'd prefer to keep the current approach in the meantime, unless benchmarking shows that it doesn't have significant impact. It would mean changing this fix to instead simply do a recursive walk of global var refs in eligibleForImport, along with my current fix for PR31052.

I have a different approach: 1) this should go in in any case, because correctness first,

This patch will not be correct on it's own, it can't go in without removing the logic in doPromoteLocalToGlobal.

and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.

I'm concerned about the performance we lose in the meantime. Note the optimization is working in other cases (when the reference to a non-const local is directly referenced by the const local being imported). To keep it working until the constness is tracked and handled in the thin link, along with my patch for PR31052, I think this would need to be changed to do a similar recursive check in canBeExternallyReferenced, like the following:

• a/lib/Transforms/IPO/FunctionImport.cpp

+++ b/lib/Transforms/IPO/FunctionImport.cpp
@@ -135,7 +135,23 @@ static bool canBeExternallyReferenced(const ModuleSummaryIndex &Index,

// We don't need to check for the module path, because if it can't be
// externally referenced and we call it, it is necessarilly in the same
// module

• return canBeExternallyReferenced(**Summaries->second.begin());

+ if (!canBeExternallyReferenced(**Summaries->second.begin()))
+ return false;
+
+ auto GVS = dyn_cast<GlobalVarSummary>(Summaries->second.begin()->get());
+ if (!GVS)
+ return true;
+ FunctionImportGlobalProcessing::doPromoteLocalToGlobal() will always
+ trigger importing the initializer for constant unnamed addr globals that
+ are referenced. We conservatively check all the referenced symbols for
+ every global to workaround this.
+ // FIXME: with a "isConstant" flag in the summary we could be more targetted.
+ for (auto &Ref : GVS->refs()) {
+ auto GUID = Ref.getGUID();
+ if (!canBeExternallyReferenced(Index, GUID))
+ return false;
+ }
+ return true;
}

// Return true if the global described by \p Summary can be imported in another

[mehdi_amini]

In D26880#601056, @tejohnson wrote:

In D26880#600927, @mehdi_amini wrote:

In D26880#600909, @tejohnson wrote:

In D26880#600745, @mehdi_amini wrote:

The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

I don't get this part?
I expect the imported to *control* what is imported, and nothing that isn't marked for import should be imported.

The IRLinker necessarily needs to link in the definition of any local referenced from the module being linked.

Not necessarily: it can just abort and return an error.

This is necessary for both Full LTO, as well as for ThinLTO if we decided not to promote and want to import a copy.

If we decide to import a copy, we should ask the IRLinker to import this local.

For ThinLTO we control this by promoting before the IRLinker is invoked (by invoking renameModuleForThinLTO) - so if it has been promoted then the IRLinker sees a non-local and does not force the def to be linked, but if it is still local at that point we need to import a local copy.

What you're describing is that we *can't* express the copy at this point, so we leave the IRLinker implicitly handling it.
I'm against any form of implicit: it adds complexity and makes it too hard to reason about each components and the invariant of the system by "hiding" them from the APIs.
This is what lead to bugs like the one addressed in this current patch.

In D26880#601057, @tejohnson wrote:

In D26880#600928, @mehdi_amini wrote:

I have a different approach: 1) this should go in in any case, because correctness first,

This patch will not be correct on it's own, it can't go in without removing the logic in doPromoteLocalToGlobal.

Then we're missing a test case in the validation, as it is passing with this patch.

and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.

I'm concerned about the performance we lose in the meantime.

I'm not why it should be a priority? Development on trunk should focus on correctness of the system *before* optimizations concerns.

[tejohnson]

In D26880#601477, @mehdi_amini wrote:

In D26880#601056, @tejohnson wrote:

In D26880#600927, @mehdi_amini wrote:

In D26880#600909, @tejohnson wrote:

In D26880#600745, @mehdi_amini wrote:

The IRLinker shouldn't be changed - it would be removing the constant unnamed_addr handling from doPromoteLocalToGlobal. The IRLinker does the right thing based on the linkage.

I don't get this part?
I expect the imported to *control* what is imported, and nothing that isn't marked for import should be imported.

The IRLinker necessarily needs to link in the definition of any local referenced from the module being linked.

Not necessarily: it can just abort and return an error.

This is necessary for both Full LTO, as well as for ThinLTO if we decided not to promote and want to import a copy.

If we decide to import a copy, we should ask the IRLinker to import this local.

It's possible that with the new LTO API the full LTO case would work with that handling removed, but I haven't walked through the usage in detail. not sure about the old libLTO case for full LTO.

For ThinLTO we control this by promoting before the IRLinker is invoked (by invoking renameModuleForThinLTO) - so if it has been promoted then the IRLinker sees a non-local and does not force the def to be linked, but if it is still local at that point we need to import a local copy.

What you're describing is that we *can't* express the copy at this point, so we leave the IRLinker implicitly handling it.
I'm against any form of implicit: it adds complexity and makes it too hard to reason about each components and the invariant of the system by "hiding" them from the APIs.
This is what lead to bugs like the one addressed in this current patch.

In D26880#601057, @tejohnson wrote:

In D26880#600928, @mehdi_amini wrote:

I have a different approach: 1) this should go in in any case, because correctness first,

This patch will not be correct on it's own, it can't go in without removing the logic in doPromoteLocalToGlobal.

Then we're missing a test case in the validation, as it is passing with this patch.

I looked at the test case that was added when the code you are removing here was added (D19102). I think if referencedbyglobal was a local it would have exposed the issue (we would have been importing it and promoting it, with this patch we would lose the promotion).

and 2) it does not seem that much "long term", it is even fairly easy to implement in the importer, but I would prioritize it depending on the benchmarks.

I'm concerned about the performance we lose in the meantime.

I'm not why it should be a priority? Development on trunk should focus on correctness of the system *before* optimizations concerns.

I normally would agree, except that this is an optimization that has been in for awhile, and maintaining it has a simple fix (the patch I sent last night should do it), until we have the longer term enhancement in.

[mehdi_amini]

In D26880#601521, @tejohnson wrote:

In D26880#601477, @mehdi_amini wrote:

I normally would agree, except that this is an optimization that has been in for awhile, and maintaining it has a simple fix (the patch I sent last night should do it), until we have the longer term enhancement in.

Right, but that's two separate discussions IMO: 1) the correctness fix, 2) adding a temporary and well scoped optimization patch waiting for a better solution (and a test-case will be needed).

[tejohnson]

In D26880#601530, @mehdi_amini wrote:

In D26880#601521, @tejohnson wrote:

In D26880#601477, @mehdi_amini wrote:

I normally would agree, except that this is an optimization that has been in for awhile, and maintaining it has a simple fix (the patch I sent last night should do it), until we have the longer term enhancement in.

Right, but that's two separate discussions IMO: 1) the correctness fix, 2) adding a temporary and well scoped optimization patch waiting for a better solution (and a test-case will be needed).

Not sure I understand - the "temporary" optimization patch is already in (and works in many cases, just not when the reference from the const global init to the referenced local that needs promotion is through some levels of indirection) - this change would necessitate removing that.

[mehdi_amini]

Correct: this optimization is incorrect as is, and not tested, which seems a good reason to remove it :)
How to recover the optimization is open to discussion though: it seems that we agree on the right thing to do, but you also seem to target a temporary patch along the same line of what I'm removing here. I'm not opposed to the latter, but writing a correct and tested patch to have this optimization seems independent from fixing the correctness issue in the first place.

[tejohnson]

In D26880#601841, @mehdi_amini wrote:

Correct: this optimization is incorrect as is, and not tested, which seems a good reason to remove it :)

The optimization is only incorrect in the transitively referenced case because the handling here and in exportGlobalInModule needs to be recursive. It's easy enough to fix (as shown in D26866 and my proposed patch to fix this case). My other patch D26866 adds a test case and it should be straightforward to modify the test added with the earlier patch that added the code here (D19102) to have regression testing.

How to recover the optimization is open to discussion though: it seems that we agree on the right thing to do, but you also seem to target a temporary patch along the same line of what I'm removing here. I'm not opposed to the latter, but writing a correct and tested patch to have this optimization seems independent from fixing the correctness issue in the first place.

I think the fix to make this work in the transitive reference case is pretty simple, as is adding tests for it. I am concerned about the cases where we lose performance where this is currently kicking in in the non-transitive case if we remove for some time before a summary-based approach is added.

[tejohnson]

In D26880#601859, @tejohnson wrote:

In D26880#601841, @mehdi_amini wrote:

How to recover the optimization is open to discussion though: it seems that we agree on the right thing to do, but you also seem to target a temporary patch along the same line of what I'm removing here. I'm not opposed to the latter, but writing a correct and tested patch to have this optimization seems independent from fixing the correctness issue in the first place.

I think the fix to make this work in the transitive reference case is pretty simple, as is adding tests for it. I am concerned about the cases where we lose performance where this is currently kicking in in the non-transitive case if we remove for some time before a summary-based approach is added.

I collected SPEC performance data with the current optimization on (default) and disabled (in doPromoteLocalToGlobal), and there was no consistent performance delta. So I'll drop my objection to removing this optimization for now until we can do it in the thin link via the summary.

However, please update this patch to remove that handling from doPromoteLocalToGlobal and add a test case that would fail with the current patch until the doPromoteLocalToGlobal handling is removed (I suggest start with the test case added in D19102 but make referencedbyglobal a local so that the referencing global var would be imported as a copy instead of promoted - then referencedbyglobal should be referenced but not promoted).

[tejohnson]

In D26880#603146, @tejohnson wrote:

In D26880#601859, @tejohnson wrote:

In D26880#601841, @mehdi_amini wrote:

How to recover the optimization is open to discussion though: it seems that we agree on the right thing to do, but you also seem to target a temporary patch along the same line of what I'm removing here. I'm not opposed to the latter, but writing a correct and tested patch to have this optimization seems independent from fixing the correctness issue in the first place.

I think the fix to make this work in the transitive reference case is pretty simple, as is adding tests for it. I am concerned about the cases where we lose performance where this is currently kicking in in the non-transitive case if we remove for some time before a summary-based approach is added.

I collected SPEC performance data with the current optimization on (default) and disabled (in doPromoteLocalToGlobal), and there was no consistent performance delta. So I'll drop my objection to removing this optimization for now until we can do it in the thin link via the summary.

However, please update this patch to remove that handling from doPromoteLocalToGlobal and add a test case that would fail with the current patch until the doPromoteLocalToGlobal handling is removed (I suggest start with the test case added in D19102 but make referencedbyglobal a local so that the referencing global var would be imported as a copy instead of promoted - then referencedbyglobal should be referenced but not promoted).

Ping. Want to get a fix in to unblock the Chromium build. Mehdi, do you want me to take this over or can you get the fix in soon?

[tejohnson]

Complete the fix by removing special casing for static consts from
the promotion logic. Update tests.

[mehdi_amini]

LGTM.
Thanks Teresa!

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

static void exportGlobalInModule(const ModuleSummaryIndex &Index,
   if (!Summary)
     return;
   // We found it in the current module, mark as exported
   ExportList.insert(GUID);
 
   auto GVS = dyn_cast<GlobalVarSummary>(Summary);
   if (!GVS)
     return;
-  // FunctionImportGlobalProcessing::doPromoteLocalToGlobal() will always
-  // trigger importing  the initializer for `constant unnamed addr` globals that
-  // are referenced. We conservatively export all the referenced symbols for
-  // every global to workaround this, so that the ExportList is accurate.
-  // FIXME: with a "isConstant" flag in the summary we could be more targetted.
-  for (auto &Ref : GVS->refs()) {
-    auto GUID = Ref.getGUID();
-    auto *RefSummary = FindGlobalSummaryInModule(GUID);
-    if (RefSummary)
-      // Found a ref in the current module, mark it as exported
-      ExportList.insert(GUID);
-  }
 }
 
 using EdgeInfo = std::pair<const FunctionSummary *, unsigned /* Threshold */>;
 
 /// Compute the list of functions to import for a given caller. Mark these
 /// imported functions and the symbols they reference in their source module as
 /// exported from their source module.
 static void computeImportForFunction(

llvm/trunk/lib/Transforms/Utils/FunctionImportUtils.cpp

 bool FunctionImportGlobalProcessing::shouldPromoteLocalToGlobal(
     const GlobalValue *SGV) {
   assert(SGV->hasLocalLinkage());
   // Both the imported references and the original local variable must
   // be promoted.
   if (!isPerformingImport() && !isModuleExporting())
     return false;
 
-  // Local const variables never need to be promoted unless they are address
-  // taken. The imported uses can simply use the clone created in this module.
-  // For now we are conservative in determining which variables are not
-  // address taken by checking the unnamed addr flag. To be more aggressive,
-  // the address taken information must be checked earlier during parsing
-  // of the module and recorded in the summary index for use when importing
-  // from that module.
-  auto *GVar = dyn_cast<GlobalVariable>(SGV);
-  if (GVar && GVar->isConstant() && GVar->hasGlobalUnnamedAddr())
-    return false;
-
   // If we are exporting, we need to see whether this value is marked
   // as NoRename in the summary. If we are importing, we may not have
   // a summary in the distributed backend case (only summaries for values
   // importes as defs, not references, are included in the index passed
   // to the distributed backends).
   if (isPerformingImport()) {
     // We don't know for sure yet if we are importing this value (as either
     // a reference or a def), since we are simply walking all values in the

llvm/trunk/test/Linker/funcimport.ll

 ; First ensure that the ThinLTO handling in llvm-link and llvm-lto handles
 ; bitcode without summary sections gracefully.
 ; RUN: opt %s -o %t.bc
 ; RUN: opt %p/Inputs/funcimport.ll -o %t2.bc
 ; RUN: llvm-link %t.bc -summary-index=%t.bc -S
 ; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
 
 ; 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 -o %t3 %t.bc %t2.bc
 
 ; Ensure statics are promoted/renamed correctly from this file (all but
 ; constant variable need promotion).
 ; RUN: llvm-link %t.bc -summary-index=%t3.thinlto.bc -S | FileCheck %s --check-prefix=EXPORTSTATIC
 ; EXPORTSTATIC-DAG: @staticvar.llvm.{{.*}} = hidden global
-; EXPORTSTATIC-DAG: @staticconstvar = internal unnamed_addr constant
+; Eventually @staticconstvar can be exported as a copy and not promoted
+; EXPORTSTATIC-DAG: @staticconstvar.llvm.0 = hidden unnamed_addr constant
 ; EXPORTSTATIC-DAG: @P.llvm.{{.*}} = hidden global void ()* null
 ; EXPORTSTATIC-DAG: define hidden i32 @staticfunc.llvm.
 ; EXPORTSTATIC-DAG: define hidden void @staticfunc2.llvm.
 
 ; Ensure that both weak alias to an imported function and strong alias to a
 ; non-imported function are correctly turned into declarations.
 ; Also ensures that alias to a linkonce function is turned into a declaration
 ; and that the associated linkonce function is not in the output, as it is

 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=linkoncefunc:%t.bc -S | FileCheck %s --check-prefix=IMPORTGLOB5
 ; IMPORTGLOB5-DAG: linkoncealias = alias void (...), bitcast (void ()* @linkoncefunc to void (...)*)
 ; IMPORTGLOB5-DAG: define linkonce_odr void @linkoncefunc()
 
 ; Ensure that imported static variable and function references are correctly
 ; promoted and renamed (including static constant variable).
 ; RUN: llvm-link %t2.bc -summary-index=%t3.thinlto.bc -import=referencestatics:%t.bc -S | FileCheck %s --check-prefix=IMPORTSTATIC
 ; IMPORTSTATIC-DAG: @staticvar.llvm.{{.*}} = external hidden global
-; IMPORTSTATIC-DAG: @staticconstvar.llvm.{{.*}} = internal unnamed_addr constant
+; Eventually @staticconstvar can be imported as a copy
+; IMPORTSTATIC-DAG: @staticconstvar.llvm.{{.*}} = external hidden unnamed_addr constant
 ; IMPORTSTATIC-DAG: define available_externally i32 @referencestatics
 ; IMPORTSTATIC-DAG: %call = call i32 @staticfunc.llvm.
 ; IMPORTSTATIC-DAG: %0 = load i32, i32* @staticvar.llvm.
 ; IMPORTSTATIC-DAG: declare hidden i32 @staticfunc.llvm.
 
 ; Ensure that imported global (external) function and variable references
 ; are handled correctly (including referenced variable imported as
 ; available_externally definition)

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

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+declare i8 **@foo()
+define i32 @main() {
+    call i8 **@foo()
+  ret i32 0
+}

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

 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define void @referencedbyglobal() {
     ret void
 }
 
+define internal void @localreferencedbyglobal() {
+    ret void
+}
+
 @someglobal = internal unnamed_addr constant i8* bitcast (void ()* @referencedbyglobal to i8*)
+@someglobal2 = internal unnamed_addr constant i8* bitcast (void ()* @localreferencedbyglobal to i8*)
 @ptr = global i8** null
+@ptr2 = global i8** null
 
 define  void @bar() #0 align 2 {
   store i8** getelementptr inbounds (i8*, i8** @someglobal, i64 0) , i8*** @ptr, align 8
+  store i8** getelementptr inbounds (i8*, i8** @someglobal2, i64 0) , i8*** @ptr2, align 8
   ret void
 }
-

llvm/trunk/test/ThinLTO/X86/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-action=thinlink -o %t3.bc %t.bc %t2.bc
 
 ; RUN: llvm-lto -thinlto-index-stats %t3.bc | FileCheck %s -check-prefix=STATS
 ; STATS: Index {{.*}} contains 24 nodes (13 functions, 3 alias, 8 globals) and 19 edges (8 refs and 11 calls)
 
 ; Ensure statics are promoted/renamed correctly from this file (all but
 ; constant variable need promotion).
 ; RUN: llvm-lto -thinlto-action=promote %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o - | FileCheck %s --check-prefix=EXPORTSTATIC
 ; EXPORTSTATIC-DAG: @staticvar.llvm.0 = hidden global
-; EXPORTSTATIC-DAG: @staticconstvar = internal unnamed_addr constant
+; Eventually @staticconstvar can be exported as a copy and not promoted
+; EXPORTSTATIC-DAG: @staticconstvar.llvm.0 = hidden unnamed_addr constant
 ; EXPORTSTATIC-DAG: @P.llvm.0 = hidden global void ()* null
 ; EXPORTSTATIC-DAG: define hidden i32 @staticfunc.llvm.0
 ; EXPORTSTATIC-DAG: define hidden void @staticfunc2.llvm.0
 
 ; Ensure that both weak alias to an imported function and strong alias to a
 ; non-imported function are correctly turned into declarations.
 ; Also ensures that alias to a linkonce function is turned into a declaration
 ; and that the associated linkonce function is not in the output, as it is

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

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/reference_non_importable.ll -o %t2.bc
+
+; RUN: llvm-lto2 %t1.bc %t2.bc -o %t.o -save-temps \
+; RUN:     -r=%t1.bc,_foo,pxl \
+; RUN:     -r=%t1.bc,_b,pxl \
+; RUN:     -r=%t2.bc,_main,pxl \
+; RUN:     -r=%t2.bc,_foo,xl
+
+
+
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+; We shouldn't promote the private because it has a section
+; RUN: llvm-dis < %t.o.0.2.internalize.bc | FileCheck  %s --check-prefix=PROMOTE
+; PROMOTE: @a = private global i8 0, section "__TEXT,__cstring,cstring_literals"
+@a = private global i8 0, section "__TEXT,__cstring,cstring_literals"
+@b = global i8 *@a
+
+
+; We want foo to be imported in the main module!
+; RUN: llvm-dis < %t.o.1.3.import.bc  | FileCheck  %s --check-prefix=IMPORT
+; IMPORT: define available_externally i8** @foo()
+define i8 **@foo() {
+  ret i8 **@b
+}

llvm/trunk/test/ThinLTO/X86/referenced_by_constant.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/referenced_by_constant.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto-action=thinlink -o %t3.bc %t.bc %t2.bc
 
-; Check the import side: we import bar() and @someglobal, but not @referencedbyglobal()
+; Check the import side: we currently only import bar() (with a future
+; enhancement to identify constants in the summary, we should mark
+; @someglobal/@someglobal2 for import as a local copy, which would
+; cause @referencedbyglobal and @localreferencedbyglobal to be exported
+; and promoted).
 ; RUN: llvm-lto -thinlto-action=import %t.bc -thinlto-index=%t3.bc -o - | llvm-dis -o -   | FileCheck %s --check-prefix=IMPORT
-; IMPORT: @someglobal.llvm.0 =
+; IMPORT: @someglobal.llvm.0 = external hidden unnamed_addr constant
+; IMPORT: @someglobal2.llvm.0 = external hidden unnamed_addr constant
 ; IMPORT: define available_externally void @bar()
-; IMPORT: declare void @referencedbyglobal()
+
+; Check the export side: we currently only export bar(), which causes
+; @someglobal and @someglobal2 to be promoted (see above).
+; RUN: llvm-lto -thinlto-action=promote %t2.bc -thinlto-index=%t3.bc -o - | llvm-dis -o -   | FileCheck %s --check-prefix=EXPORT
+; EXPORT: @someglobal.llvm.0 = hidden unnamed_addr constant
+; EXPORT: @someglobal2.llvm.0 = hidden unnamed_addr constant
+; EXPORT: define void @referencedbyglobal()
+; EXPORT: define internal void @localreferencedbyglobal()
 
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 declare void @bar()
 
 define void @foo() {
     call void @bar()
     ret void
 }
-