[ThinLTO] Distinguish symbols that are promoted (#181946)

Thinlink may decide some symbols with internal linkage should get promoted to external. Such a symbol, when being imported, would have its name changed by appending a suffix (`.llvm.<a hash>`) to avoid collisions - since internal linkage symbols have non-unique names.

Later, still during Thinlink, in `thinLTOResolvePrevailingGUID`, the fact that this symbol was promoted is not considered and we set its linkage to `AvailableExternally`(when reading `thinLTOResolvePrevailingGUID`, note that "prevailing-ness" is not a concept that the original symbol would have participated in)



This should result in a final (native) link error, because the symbol's definition may be elided. But we get lucky: in the post-thinlink backend, during import, in `llvm::thinLTOFinalizeInModule`, after this symbol's name was changed and its linkage also changed to `External` (see `FunctionImportGlobalProcessing::processGlobalForThinLTO`), we try to find it in the `DefinedGlobals`, fail (because its guid is computed from its changed name)) and leave its linkage as-is. Which happens to be correct.

This patch makes this outcome intentional rather than accidental. It becomes critical once we land [this RFC](https://discourse.llvm.org/t/rfc-keep-globalvalue-guids-stable/84801).

As a side-benefit, the extra attribute propagations that weren't happening in `llvm::thinLTOFinalizeInModule` now do.

llvm/include/llvm/IR/ModuleSummaryIndex.h

@@ -509,6 +509,10 @@ public:
     /// summary. The value is interpreted as 'ImportKind' enum defined above.
     unsigned ImportType : 1;
 
+    /// This symbol was promoted. Thinlink stages need to be aware of this
+    /// transition
+    unsigned Promoted : 1;
+
     /// Convenience Constructors
     explicit GVFlags(GlobalValue::LinkageTypes Linkage,
                      GlobalValue::VisibilityTypes Visibility,
@@ -517,7 +521,7 @@ public:
         : Linkage(Linkage), Visibility(Visibility),
           NotEligibleToImport(NotEligibleToImport), Live(Live),
           DSOLocal(IsLocal), CanAutoHide(CanAutoHide),
-          ImportType(static_cast<unsigned>(ImportType)) {}
+          ImportType(static_cast<unsigned>(ImportType)), Promoted(false) {}
   };
 
 private:
@@ -584,12 +588,28 @@ public:
     return static_cast<GlobalValue::LinkageTypes>(Flags.Linkage);
   }
 
+  bool wasPromoted() const { return Flags.Promoted; }
+
+  void promote() {
+    assert(GlobalValue::isLocalLinkage(linkage()) &&
+           "unexpected (re-)promotion of non-local symbol");
+    assert(!Flags.Promoted);
+    Flags.Promoted = true;
+    Flags.Linkage = GlobalValue::LinkageTypes::ExternalLinkage;
+  }
+
   /// Sets the linkage to the value determined by global summary-based
   /// optimization. Will be applied in the ThinLTO backends.
   void setLinkage(GlobalValue::LinkageTypes Linkage) {
+    assert(!wasPromoted());
+    assert(!GlobalValue::isExternalLinkage(Linkage) && "use `promote` instead");
     Flags.Linkage = Linkage;
   }
 
+  void setExternalLinkageForTest() {
+    Flags.Linkage = GlobalValue::LinkageTypes::ExternalLinkage;
+  }
+
   /// Return true if this global value can't be imported.
   bool notEligibleToImport() const { return Flags.NotEligibleToImport; }
 

llvm/lib/LTO/LTO.cpp

@@ -393,6 +393,9 @@ static void thinLTOResolvePrevailingGUID(
     // FIXME: We may want to split the compile time and correctness
     // aspects into separate routines.
     if (isPrevailing(VI.getGUID(), S.get())) {
+      assert(!S->wasPromoted() &&
+             "promoted symbols used to be internal linkage and shouldn't have "
+             "a prevailing variant");
       if (GlobalValue::isLinkOnceLinkage(OriginalLinkage)) {
         S->setLinkage(GlobalValue::getWeakLinkage(
             GlobalValue::isLinkOnceODRLinkage(OriginalLinkage)));
@@ -415,8 +418,11 @@ static void thinLTOResolvePrevailingGUID(
     // When force-import-all is used, it indicates that object linking is not
     // supported by the target. In this case, we can't change the linkage as
     // well in case the global is converted to declaration.
+    // Also, if the symbol was promoted, it wouldn't have a prevailing variant,
+    // but also its linkage is set correctly (to External) already.
     else if (!isa<AliasSummary>(S.get()) &&
-             !GlobalInvolvedWithAlias.count(S.get()) && !ForceImportAll)
+             !GlobalInvolvedWithAlias.count(S.get()) && !ForceImportAll &&
+             !S->wasPromoted())
       S->setLinkage(GlobalValue::AvailableExternallyLinkage);
 
     // For ELF, set visibility to the computed visibility from summaries. We
@@ -485,7 +491,7 @@ static void thinLTOInternalizeAndPromoteGUID(
     // exported.
     if (isExported(S->modulePath(), VI)) {
       if (GlobalValue::isLocalLinkage(S->linkage()))
-        S->setLinkage(GlobalValue::ExternalLinkage);
+        S->promote();
       continue;
     }
 

llvm/lib/Transforms/IPO/FunctionAttrs.cpp

@@ -387,7 +387,7 @@ static FunctionSummary *calculatePrevailingSummary(
       }
       Local = FS;
     } else if (GlobalValue::isExternalLinkage(Linkage)) {
-      assert(IsPrevailing(VI.getGUID(), GVS.get()));
+      assert(IsPrevailing(VI.getGUID(), GVS.get()) || GVS->wasPromoted());
       Prevailing = FS;
       break;
     } else if (GlobalValue::isWeakODRLinkage(Linkage) ||

llvm/lib/Transforms/IPO/FunctionImport.cpp

@@ -2106,7 +2106,7 @@ static bool doImportingForModuleForTest(
   for (auto &I : *Index) {
     for (auto &S : I.second.getSummaryList()) {
       if (GlobalValue::isLocalLinkage(S->linkage()))
-        S->setLinkage(GlobalValue::ExternalLinkage);
+        S->setExternalLinkageForTest();
     }
   }
 

llvm/test/ThinLTO/X86/Inputs/export2.ll

@@ -0,0 +1,9 @@
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @main() {
+entry:
+  call void @callstaticfunc()
+  ret i32 0
+}
+
+declare void @callstaticfunc()

llvm/test/ThinLTO/X86/export2.ll

@@ -0,0 +1,27 @@
+; Do setup work for all below tests: generate bitcode and combined index
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/export2.ll -o %t2.bc
+; 
+; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t3 \
+; RUN:  -thinlto-distributed-indexes \
+; RUN:  -r=%t1.bc,callstaticfunc,px \
+; RUN:  -r=%t2.bc,main,px \
+; RUN:  -r=%t2.bc,callstaticfunc,
+
+; both functions must appear as external linkage in the index.
+; RUN: llvm-dis %t1.bc.thinlto.bc -o - | FileCheck %s
+; CHECK: linkage: external
+; CHECK: linkage: external
+
+target triple = "x86_64-unknown-linux-gnu"
+
+define void @callstaticfunc() {
+entry:
+  call void @staticfunc()
+  ret void
+}
+
+define internal void @staticfunc() {
+entry:
+  ret void
+}

llvm/test/ThinLTO/X86/funcattrs-prop-exported-internal.ll

@@ -27,7 +27,7 @@ define void @importer() {
 
 ; If somehow the caller doesn't get the attributes, we
 ; shouldn't propagate from the internal callee.
-; CHECK: define void @importer_noattr() {
+; CHECK: define void @importer_noattr() #0 {
 define void @importer_noattr() {
   call void @caller_noattr()
   ret void

llvm/test/ThinLTO/X86/module_asm2.ll

@@ -9,7 +9,7 @@
 ; RUN:  llvm-nm %t2.bc.thinlto.o | FileCheck  %s --check-prefix=NM1
 
 ; RUN: llvm-lto2 run %t1.bc %t2.bc -o %t.o -save-temps \
-; RUN:     -r=%t1.bc,foo,plx \
+; RUN:     -r=%t1.bc,foo,lx \
 ; RUN:     -r=%t1.bc,globalfunc,plx \
 ; RUN:     -r=%t1.bc,globalfunc,lx \
 ; RUN:     -r=%t1.bc,weakfunc,plx \

llvm/tools/llvm-link/llvm-link.cpp

@@ -422,7 +422,7 @@ static bool linkFiles(const char *argv0, LLVMContext &Context, Linker &L,
       for (auto &I : *Index) {
         for (auto &S : I.second.getSummaryList()) {
           if (GlobalValue::isLocalLinkage(S->linkage()))
-            S->setLinkage(GlobalValue::ExternalLinkage);
+            S->setExternalLinkageForTest();
         }
       }