[lib/Analysis] - Mark personality functions as live.

[grimar]

Summary

This is PR33245.

Case I am fixing is next:
Imagine we have 2 BC files, one defines and uses personality routine,
second has only declaration and also uses it.

Previously algorithm comuting dead symbols (llvm::computeDeadSymbols) did
not know about personality routines and leaved them dead even if function that
has routine was live.

As a result thinLTOInternalizeAndPromoteGUID() method changed binding for
such symbol to local. Later when LLD tried to link these objects it failed
because one object had undefined global symbol for routine and second
object contained local definition instead of global.

Patch set the live root flag on the corresponding FunctionSummary
for personality routines when we build the per-module summaries
during the compile step.

[tejohnson]

Rather than modifying the index format, can you just set the live root flag on the corresponding FunctionSummary for personality routines when we build the per-module summaries during the compile step? I.e. here: http://llvm-cs.pcc.me.uk/lib/Analysis/ModuleSummaryAnalysis.cpp#278?

[grimar]

• Addressed review comment.

[davide]

This is a step in the right direction.

lib/Analysis/ModuleSummaryAnalysis.cpp

406–408 ↗

(On Diff #111658)

Needs a comment.

[tejohnson]

lib/Analysis/ModuleSummaryAnalysis.cpp

408 ↗

(On Diff #111658)

Can the personality function ever have local linkage? If so, this particular interface won't work, since it will compute the GUID used to key into the index via just the name (and for locals the GUID is computed a little differently). The current setLiveRoot method was added for the case of a set of special globals accessed by name, so doesn't currently support the local case. What you can do is simply add another setLiveRoot method that takes the result of getPersonalityFn (which I see returns a Constant, so casted to a GlobalValue - would it ever not be a GV?), and invokes GV->getGUID() on the supplied GlobalValue (instead of the static getGUID invoked with on the supplied name in the current method).

If the personality fn should never be local, please add an assert here to that effect with a comment about the GUID computation in this setLiveRoot method not being correct for locals.

[grimar]

lib/Analysis/ModuleSummaryAnalysis.cpp

408 ↗

(On Diff #111658)

Thanks a lot for providing these details !

I performed an experiment and found that it is possible for personality function to be local, even though it
is probably uncommon situation in general.

I took little c++ sample that throws exception and modified it slightly:
added my own personality routine personality_routine with internal binding to replace
__gxx_personality_v0 one used by default.
In my routine I just printf() some test text to check it is called.
(shared code here FWIW: https://justpaste.it/1a88c)

And it works, I see my text and personality function is local as expected:

13: 0000000000201180    16 FUNC    LOCAL  DEFAULT   14 personality_routine

I'll update the patch to fix this case at monday.

[grimar]

lib/Analysis/ModuleSummaryAnalysis.cpp

408 ↗

(On Diff #111658)

After closer look it seems to me it is correct to use setLiveRoot here.

Imagine we have code:
define internal i32 @personality_routine(i32, i32, i64, i8*, i8*) {

call void @foo()
ret i32 0

}

personality_routine is local here. But computeFunctionSummary() adds all functions to index using
'f.getName()' and not 'F.getGUID()':
https://github.com/llvm-mirror/llvm/blob/master/lib/Analysis/ModuleSummaryAnalysis.cpp#L317

So it keeps it name like it is global and not local. That looks intentional logic, because
ModuleSummaruIndexBitcodeReader has code that calculates local GUID, but also stores
global GUID as OriginalNameID
(https://github.com/llvm-mirror/llvm/blob/master/lib/Bitcode/Reader/BitcodeReader.cpp#L4774)

And if I would use GV->getGUID() for doing setLiveRoot, It would not find value info for such
GUID, because computeFunctionSummary() adds it as by simple name. So code would be unable
to find VI and set live flag.

I'll update the diff with new testcase to show that local personality routines work with current version.

[grimar]

• Added comment and testcase.

[tejohnson]

LGTM

lib/Analysis/ModuleSummaryAnalysis.cpp

408 ↗

(On Diff #111658)

Sorry for the noise - you are right, it is only in the combined index that we use the GUID! Thanks for adding the test case showing that this will work for local personality routines.

[grimar]

lib/Analysis/ModuleSummaryAnalysis.cpp

408 ↗

(On Diff #111658)

Thanks for review ! I'll commit this tomorrow.

[davide]

Thanks for this fix. A couple of minors, otherwise LGTM.

lib/Analysis/ModuleSummaryAnalysis.cpp

406–407 ↗

(On Diff #111955)

Minor: The common terminology is DCE rather than "dead value analysis".

408 ↗

(On Diff #111955)

Can you use the explicit type here? (up to you).

[grimar]

lib/Analysis/ModuleSummaryAnalysis.cpp

406–407 ↗

(On Diff #111955)

Ok, will use it, thanks.
FWIW I stole "dead value analysis" from comment below initially:

// The linker doesn't know about these LLVM produced values, so we need
// to flag them as live in the index to ensure index-based dead value
// analysis treats them as live roots of the analysis.

408 ↗

(On Diff #111955)

Will do.

include/llvm/IR/ModuleSummaryIndex.h

struct TypeIdInfo {
     /// llvm.assume(llvm.type.test) or llvm.type.checked.load intrinsics with
     /// all constant integer arguments.
     std::vector<ConstVCall> TypeTestAssumeConstVCalls,
         TypeCheckedLoadConstVCalls;
   };
 
   std::unique_ptr<TypeIdInfo> TIdInfo;
 
+  llvm::Optional<ValueInfo> PersonalityInfo;
+
 public:
   FunctionSummary(GVFlags Flags, unsigned NumInsts, FFlags FunFlags,
                   std::vector<ValueInfo> Refs, std::vector<EdgeTy> CGEdges,
                   std::vector<GlobalValue::GUID> TypeTests,
                   std::vector<VFuncId> TypeTestAssumeVCalls,
                   std::vector<VFuncId> TypeCheckedLoadVCalls,
                   std::vector<ConstVCall> TypeTestAssumeConstVCalls,
                   std::vector<ConstVCall> TypeCheckedLoadConstVCalls)
public:
   FFlags &fflags() { return FunFlags; }
 
   /// Get the instruction count recorded for this function.
   unsigned instCount() const { return InstCount; }
 
   /// Return the list of <CalleeValueInfo, CalleeInfo> pairs.
   ArrayRef<EdgeTy> calls() const { return CallGraphEdgeList; }
 
+  llvm::Optional<ValueInfo> getPersonality() { return PersonalityInfo; }
+
+  void setPersonality(ValueInfo Personality) { PersonalityInfo = Personality; }
+
   /// Returns the list of type identifiers used by this function in
   /// llvm.type.test intrinsics other than by an llvm.assume intrinsic,
   /// represented as GUIDs.
   ArrayRef<GlobalValue::GUID> type_tests() const {
     if (TIdInfo)
       return TIdInfo->TypeTests;
     return {};
   }

lib/Bitcode/Reader/BitcodeReader.cpp

void setValueGUID(uint64_t ValueID, StringRef ValueName,
                     StringRef SourceFileName);
   Error parseValueSymbolTable(
       uint64_t Offset,
       DenseMap<unsigned, GlobalValue::LinkageTypes> &ValueIdToLinkageMap);
   std::vector<ValueInfo> makeRefList(ArrayRef<uint64_t> Record);
   std::vector<FunctionSummary::EdgeTy> makeCallList(ArrayRef<uint64_t> Record,
                                                     bool IsOldProfileFormat,
                                                     bool HasProfile);
-  Error parseEntireSummary(unsigned ID);
+  Error parseEntireSummary(unsigned ID,
+                           DenseMap<unsigned, uint64_t> &PersonalityMap);
   Error parseModuleStringTable();
 
   std::pair<ValueInfo, GlobalValue::GUID>
   getValueInfoFromValueId(unsigned ValueId);
 
   ModuleSummaryIndex::ModuleInfo *addThisModule();
 };
 

 // At the end of this routine the module Index is populated with a map
 // from global value id to GlobalValueSummary objects.
 Error ModuleSummaryIndexBitcodeReader::parseModule() {
   if (Stream.EnterSubBlock(bitc::MODULE_BLOCK_ID))
     return error("Invalid record");
 
   SmallVector<uint64_t, 64> Record;
   DenseMap<unsigned, GlobalValue::LinkageTypes> ValueIdToLinkageMap;
+  DenseMap<unsigned, uint64_t> PersonalityMap;
   unsigned ValueId = 0;
 
   // Read the index for this module.
   while (true) {
     BitstreamEntry Entry = Stream.advance();
 
     switch (Entry.Kind) {
     case BitstreamEntry::Error:
case BitstreamEntry::SubBlock:
         // performing ThinLTO compiles so we don't later invoke
         // the regular LTO process on them.
         if (VSTOffset > 0) {
           if (Error Err = parseValueSymbolTable(VSTOffset, ValueIdToLinkageMap))
             return Err;
           SeenValueSymbolTable = true;
         }
         SeenGlobalValSummary = true;
-        if (Error Err = parseEntireSummary(Entry.ID))
+        if (Error Err = parseEntireSummary(Entry.ID, PersonalityMap))
           return Err;
         break;
       case bitc::MODULE_STRTAB_BLOCK_ID:
         if (Error Err = parseModuleStringTable())
           return Err;
         break;
       }
       continue;
case BitstreamEntry::Record: {
         case bitc::MODULE_CODE_GLOBALVAR:
         case bitc::MODULE_CODE_FUNCTION:
         case bitc::MODULE_CODE_ALIAS: {
           StringRef Name;
           ArrayRef<uint64_t> GVRecord;
           std::tie(Name, GVRecord) = readNameFromStrtab(Record);
           if (GVRecord.size() <= 3)
             return error("Invalid record");
+
+          // Remember personality routine if exist. If non-zero, the value in
+          // GVRecord is index of the personality function routine plus 1.
+          if (BitCode == bitc::MODULE_CODE_FUNCTION)
+            if (GVRecord.size() >= 15 && GVRecord[14] != 0)
+              PersonalityMap[ValueId] = GVRecord[14] - 1;
+
           uint64_t RawLinkage = GVRecord[3];
           GlobalValue::LinkageTypes Linkage = getDecodedLinkage(RawLinkage);
           if (!UseStrtab) {
             ValueIdToLinkageMap[ValueId++] = Linkage;
             break;
           }
 
           setValueGUID(ValueId++, Name, Linkage, SourceFileName);
if (IsOldProfileFormat) {
       Hotness = static_cast<CalleeInfo::HotnessType>(Record[++I]);
     Ret.push_back(FunctionSummary::EdgeTy{Callee, CalleeInfo{Hotness}});
   }
   return Ret;
 }
 
 // Eagerly parse the entire summary block. This populates the GlobalValueSummary
 // objects in the index.
-Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(unsigned ID) {
+Error ModuleSummaryIndexBitcodeReader::parseEntireSummary(
+    unsigned ID, DenseMap<unsigned, uint64_t> &PersonalityMap) {
   if (Stream.EnterSubBlock(ID))
     return error("Invalid record");
   SmallVector<uint64_t, 64> Record;
 
   // Parse version
   {
     BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
     if (Entry.Kind != BitstreamEntry::Record)
case bitc::FS_PERMODULE_PROFILE: {
       PendingTypeTests.clear();
       PendingTypeTestAssumeVCalls.clear();
       PendingTypeCheckedLoadVCalls.clear();
       PendingTypeTestAssumeConstVCalls.clear();
       PendingTypeCheckedLoadConstVCalls.clear();
       auto VIAndOriginalGUID = getValueInfoFromValueId(ValueID);
       FS->setModulePath(addThisModule()->first());
       FS->setOriginalName(VIAndOriginalGUID.second);
+      auto It = PersonalityMap.find(ValueID);
+      if (It != PersonalityMap.end())
+        FS->setPersonality(getValueInfoFromValueId(It->second).first);
       TheIndex.addGlobalValueSummary(VIAndOriginalGUID.first, std::move(FS));
       break;
     }
     // FS_ALIAS: [valueid, flags, valueid]
     // Aliases must be emitted (and parsed) after all FS_PERMODULE entries, as
     // they expect all aliasee summaries to be available.
     case bitc::FS_ALIAS: {
       unsigned ValueID = Record[0];

lib/Transforms/IPO/FunctionImport.cpp

auto visit = [&](ValueInfo VI) {
     Worklist.push_back(VI);
   };
 
   while (!Worklist.empty()) {
     auto VI = Worklist.pop_back_val();
     for (auto &Summary : VI.getSummaryList()) {
       for (auto Ref : Summary->refs())
         visit(Ref);
-      if (auto *FS = dyn_cast<FunctionSummary>(Summary.get()))
+      if (auto *FS = dyn_cast<FunctionSummary>(Summary.get())) {
         for (auto Call : FS->calls())
           visit(Call.first);
+        if (Optional<ValueInfo> P = FS->getPersonality())
+          visit(*P);
+      }
       if (auto *AS = dyn_cast<AliasSummary>(Summary.get())) {
         auto AliaseeGUID = AS->getAliasee().getOriginalName();
         ValueInfo AliaseeVI = Index.getValueInfo(AliaseeGUID);
         if (AliaseeVI)
           visit(AliaseeVI);
       }
     }
   }

test/ThinLTO/X86/Inputs/personality.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+define void @bar() personality i32 (i32, i32, i64, i8*, i8*)* @personality_routine {
+ ret void
+}
+
+define protected i32 @personality_routine(i32, i32, i64, i8*, i8*) {
+  ret i32 0
+}

test/ThinLTO/X86/personality.ll

+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %S/Inputs/personality.ll -o %t2.bc
+
+; RUN: llvm-lto2 run -o %t.o %t1.bc %t2.bc -save-temps \
+; RUN:   -r %t2.bc,bar,p \
+; RUN:   -r %t2.bc,personality_routine,p \
+; RUN:   -r %t1.bc,foo,p \
+; RUN:   -r %t1.bc,personality_routine,l \
+; RUN:   -r %t1.bc,main,xp \
+; RUN:   -r %t1.bc,bar,l
+; RUN: llvm-readobj -t %t.o.1 | FileCheck %s --check-prefix=BINDING
+
+; BINDING:     Symbol {
+; BINDING:       Name: personality_routine
+; BINDING-NEXT:  Value:
+; BINDING-NEXT:  Size:
+; BINDING-NEXT:  Binding: Global
+; BINDING-NEXT:  Type: Function
+; BINDING-NEXT:  Other [
+; BINDING-NEXT:    STV_PROTECTED
+; BINDING-NEXT:  ]
+; BINDING-NEXT:  Section: .text
+; BINDING-NEXT: }
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+declare protected i32 @personality_routine(i32, i32, i64, i8*, i8*)
+declare void @bar()
+
+define void @foo() personality i32 (i32, i32, i64, i8*, i8*)* @personality_routine {
+  ret void
+}
+
+define i32 @main() {
+entry:
+  call void @foo()
+  call void @bar()
+  ret i32 0
+}
+