[ThinLTO] Don't import GV which contains blockaddress

[evgeny777]

Summary

PR39241 uncovered an interesting problem with importing global variables containing blockaddress. Such variables can be imported only with function(s) referenced by blockaddress statements. As this is hard to check, I suggest simpler fix which does the following:

• Marks all GVs which contain blockaddrss statement as "not eligible to import".
• Instead of checking for dummy module in computeImportForReferencedGlobals check "not eligible to import" flag. This shouldn't break things as everything in non-thinlto module is marked as not eligible to import.

[tejohnson]

lib/Analysis/ModuleSummaryAnalysis.cpp

76

Document return value. Also, there are callers of findRefEdges in computeFunctionSummary. Are those ever expected to reference a blockaddress? From what I can tell in https://llvm.org/docs/LangRef.html#addresses-of-basic-blocks, it seems they could be referenced by an indirectbr instruction. Either those callsites need to do something similar with the return value, or they should assert findRefEdges returns false.

[evgeny777]

Are those ever expected to reference a blockaddress?

Yes, they are. But instruction in any given function can only reference basic block in the same function.
So we're not interested in return value of findRefEdges. I've reflected this in a comment to findRefEdges

[tejohnson]

LGTM with a minor comment fix noted below.

lib/Analysis/ModuleSummaryAnalysis.cpp

78

s/kbow/know/

lib/Analysis/ModuleSummaryAnalysis.cpp

cl::opt<FunctionSummary::ForceSummaryHotnessType, true> FSEC(
     cl::desc("Force all edges in the function summary to cold"),
     cl::values(clEnumValN(FunctionSummary::FSHT_None, "none", "None."),
                clEnumValN(FunctionSummary::FSHT_AllNonCritical,
                           "all-non-critical", "All non-critical edges."),
                clEnumValN(FunctionSummary::FSHT_All, "all", "All edges.")));
 
 // Walk through the operands of a given User via worklist iteration and populate
 // the set of GlobalValue references encountered. Invoked either on an
 // Instruction or a GlobalVariable (which walks its initializer).
-static void findRefEdges(ModuleSummaryIndex &Index, const User *CurUser,
+// Return true if any of the operands contains blockaddress. This is important
+// to kbow when computing summary for global var, because if global variable
+// references basic block address we can't import it separately from function
+// containing that basic block. For simplicity we currently don't import such
+// global vars at all. When importing function we aren't interested if any 
+// instruction in it takes an address of any basic block, because instruction
+// can only take an address of basic block located in the same function.
+static bool findRefEdges(ModuleSummaryIndex &Index, const User *CurUser,
                          SetVector<ValueInfo> &RefEdges,
                          SmallPtrSet<const User *, 8> &Visited) {
+  bool HasBlockAddress = false;
   SmallVector<const User *, 32> Worklist;
   Worklist.push_back(CurUser);
 
   while (!Worklist.empty()) {
     const User *U = Worklist.pop_back_val();
 
     if (!Visited.insert(U).second)
       continue;
 
     ImmutableCallSite CS(U);
 
     for (const auto &OI : U->operands()) {
       const User *Operand = dyn_cast<User>(OI);
       if (!Operand)
         continue;
-      if (isa<BlockAddress>(Operand))
+      if (isa<BlockAddress>(Operand)) {
+        HasBlockAddress = true;
         continue;
+      }
       if (auto *GV = dyn_cast<GlobalValue>(Operand)) {
         // We have a reference to a global value. This should be added to
         // the reference set unless it is a callee. Callees are handled
         // specially by WriteFunction and are added to a separate list.
         if (!(CS && CS.isCallee(&OI)))
           RefEdges.insert(Index.getOrInsertValueInfo(GV));
         continue;
       }
       Worklist.push_back(Operand);
     }
   }
+  return HasBlockAddress;
 }
 
 static CalleeInfo::HotnessType getHotness(uint64_t ProfileCount,
                                           ProfileSummaryInfo *PSI) {
   if (!PSI)
     return CalleeInfo::HotnessType::Unknown;
   if (PSI->isHotCount(ProfileCount))
     return CalleeInfo::HotnessType::Hot;
static void computeFunctionSummary(
   Index.addGlobalValueSummary(F, std::move(FuncSummary));
 }
 
 static void
 computeVariableSummary(ModuleSummaryIndex &Index, const GlobalVariable &V,
                        DenseSet<GlobalValue::GUID> &CantBePromoted) {
   SetVector<ValueInfo> RefEdges;
   SmallPtrSet<const User *, 8> Visited;
-  findRefEdges(Index, &V, RefEdges, Visited);
+  bool HasBlockAddress = findRefEdges(Index, &V, RefEdges, Visited);
   bool NonRenamableLocal = isNonRenamableLocal(V);
   GlobalValueSummary::GVFlags Flags(V.getLinkage(), NonRenamableLocal,
                                     /* Live = */ false, V.isDSOLocal());
   auto GVarSummary =
       llvm::make_unique<GlobalVarSummary>(Flags, RefEdges.takeVector());
   if (NonRenamableLocal)
     CantBePromoted.insert(V.getGUID());
+  if (HasBlockAddress)
+    GVarSummary->setNotEligibleToImport();
   Index.addGlobalValueSummary(V, std::move(GVarSummary));
 }
 
 static void
 computeAliasSummary(ModuleSummaryIndex &Index, const GlobalAlias &A,
                     DenseSet<GlobalValue::GUID> &CantBePromoted) {
   bool NonRenamableLocal = isNonRenamableLocal(A);
   GlobalValueSummary::GVFlags Flags(A.getLinkage(), NonRenamableLocal,

lib/Transforms/IPO/FunctionImport.cpp

if (DefinedGVSummaries.count(VI.getGUID())) {
           dbgs() << "Ref ignored! Target already in destination module.\n");
       continue;
     }
 
     LLVM_DEBUG(dbgs() << " ref -> " << VI << "\n");
 
     for (auto &RefSummary : VI.getSummaryList())
       if (RefSummary->getSummaryKind() == GlobalValueSummary::GlobalVarKind &&
-          // Don't try to import regular LTO summaries added to dummy module.
-          !RefSummary->modulePath().empty() &&
+          !RefSummary->notEligibleToImport() &&
           !GlobalValue::isInterposableLinkage(RefSummary->linkage()) &&
           RefSummary->refs().empty()) {
         ImportList[RefSummary->modulePath()].insert(VI.getGUID());
         if (ExportLists)
           (*ExportLists)[RefSummary->modulePath()].insert(VI.getGUID());
         break;
       }
   }

test/ThinLTO/X86/Inputs/globals-import-blockaddr.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@label_addr = internal constant [1 x i8*] [i8* blockaddress(@foo, %lb)], align 8
+
+; Function Attrs: noinline norecurse nounwind optnone uwtable
+define dso_local [1 x i8*]* @foo() {
+  br label %lb
+
+lb:
+  ret [1 x i8*]* @label_addr
+}

test/ThinLTO/X86/globals-import-blockaddr.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/globals-import-blockaddr.ll -o %t2.bc
+; RUN: llvm-lto2 run -save-temps %t1.bc -r=%t1.bc,foo,l -r=%t1.bc,main,pl %t2.bc -r=%t2.bc,foo,pl -o %t3
+; RUN: llvm-dis %t3.1.3.import.bc -o - | FileCheck %s
+
+; Verify that we haven't imported GV containing blockaddress
+; CHECK: @label_addr.llvm.0 = external hidden constant
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare dso_local [1 x i8*]* @foo();
+
+define dso_local i32 @main() {
+  %p = call [1 x i8*]* @foo()
+  %v = ptrtoint [1 x i8*]* %p to i32
+  ret i32 %v
+}