ThinLTO: Correctly follow aliasee references when dead stripping.

[pcc]

Summary

We were previously handling aliases during dead stripping by adding
the aliased global's "original name" GUID to the worklist. This will
lead to incorrect behaviour if the global has local linkage because
the original name GUID will not correspond to the global's GUID in
the summary.

Because an alias is just another name for the global that it
references, there is no need to mark the referenced global as used,
or to follow references from any other copies of the global. So all
we need to do is to follow references from the aliasee's summary
instead of the alias.

[tejohnson]

LGTM, not sure how we didn't hit this before!

This got me thinking - AliasSummary shouldn't have any refs, and in fact while the constructor takes a list of refs, we always pass in an empty list. Short of restructuring the class hierarchy so that AliasSummary doesn't have the refs member, I should at least remove that parameter from the constructor, and probably add an assert in the base class constructor that we don't have a non-empty list for an alias summary type. I'll go ahead and do that in a separate patch...

[pcc]

In D37789#869660, @tejohnson wrote:

LGTM, not sure how we didn't hit this before!

Short of restructuring the class hierarchy so that AliasSummary doesn't have the refs member

Alternatively, represent alias summaries using a copy of the aliasee's FunctionSummary or GlobalVariableSummary, and remove the AliasSummary class altogether.

I should at least remove that parameter from the constructor, and probably add an assert in the base class constructor that we don't have a non-empty list for an alias summary type. I'll go ahead and do that in a separate patch...

OK, sounds good.

llvm/lib/Transforms/IPO/FunctionImport.cpp

for (auto &S : VI.getSummaryList())
       S->setLive(true);
     ++LiveSymbols;
     Worklist.push_back(VI);
   };
 
   while (!Worklist.empty()) {
     auto VI = Worklist.pop_back_val();
     for (auto &Summary : VI.getSummaryList()) {
-      for (auto Ref : Summary->refs())
+      GlobalValueSummary *Base = Summary.get();
+      if (auto *AS = dyn_cast<AliasSummary>(Base))
+        Base = &AS->getAliasee();
+      for (auto Ref : Base->refs())
         visit(Ref);
-      if (auto *FS = dyn_cast<FunctionSummary>(Summary.get()))
+      if (auto *FS = dyn_cast<FunctionSummary>(Base))
         for (auto Call : FS->calls())
           visit(Call.first);
-      if (auto *AS = dyn_cast<AliasSummary>(Summary.get())) {
-        auto AliaseeGUID = AS->getAliasee().getOriginalName();
-        ValueInfo AliaseeVI = Index.getValueInfo(AliaseeGUID);
-        if (AliaseeVI)
-          visit(AliaseeVI);
-      }
     }
   }
   Index.setWithGlobalValueDeadStripping();
 
   unsigned DeadSymbols = Index.size() - LiveSymbols;
   DEBUG(dbgs() << LiveSymbols << " symbols Live, and " << DeadSymbols
                << " symbols Dead \n");
   NumDeadSymbols += DeadSymbols;

llvm/test/LTO/Resolution/X86/Inputs/dead-strip-alias.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@external = global i8 42

llvm/test/LTO/Resolution/X86/dead-strip-alias.ll

+; RUN: opt -module-summary -o %t %s
+; RUN: opt -module-summary -o %t2 %S/Inputs/dead-strip-alias.ll
+; RUN: llvm-lto2 run %t -r %t,main,px -r %t,alias,p -r %t,external, \
+; RUN:               %t2 -r %t2,external,p \
+; RUN: -save-temps -o %t3
+; RUN: llvm-nm %t3.1 | FileCheck %s
+
+; CHECK: D external
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@alias = alias i8*, i8** @internal
+
+@internal = internal global i8* @external
+@external = external global i8
+
+define i8** @main() {
+  ret i8** @alias
+}