[llvm-profdata] ProfileReader cleanup - preparation for MD5 refactoring - 3

[huangjd]

Summary

Cleanup profile reader classes to prepare for complex refactoring as propsed in D147740, continuing D148872
This is patch 3/n. This patch changes the behavior of function offset table.

Previously when reading ExtBinary profile, the funcOffsetTable (map) is always populated, and in addition if the profile is CS, the orderedFuncOffsets (list) is also populated. However when reading the function samples, only one of the container is being used, never both, so it's a huge waste of time to populate both. Added logic to select which one to use, and completely skip reading function offset table if we are in tool mode (all function samples are to be read sequentially regardless)

[davidxl]

llvm/include/llvm/ProfileData/SampleProfReader.h

689 ↗

(On Diff #516629)

does it need to check the section flag?

808 ↗

(On Diff #516629)

Is this a dead field?

[huangjd]

Added check for orderedFuncOffsets for CS profile back

[huangjd]

llvm/include/llvm/ProfileData/SampleProfReader.h

689 ↗

(On Diff #516629)

I added back the section flag to check if CS profile has ordered function offset table.

808 ↗

(On Diff #516629)

Updated.

[huangjd]

Combined similar code

[huangjd]

Fixed unused variable for release build

[hoy]

llvm/include/llvm/ProfileData/SampleProfReader.h

689 ↗

(On Diff #516629)

Does the function offset table for a remapper profile always come in sorted?

[wenlei]

llvm/include/llvm/ProfileData/SampleProfReader.h

673 ↗

(On Diff #517335)

all prefixes of the matched function can be loaded,

prefix is caller, but it's meant to read in all callees of matches functions.

687 ↗

(On Diff #517335)

For readability, i'd suggest expand the logic with inline comments for each category. The big chunk of comment above the function is harder to parse and more prone to be out dated. Something like this:

// category and reason
if ....
   return true;

// category and reason
if ...
   return true;

...

// category and reason
return false;

689 ↗

(On Diff #516629)

This function now controls whether list or map is used, which is orthogonal to whether it's ordered (still controlled by section flag). So useOrderedFuncOffsets() is more like useFuncOffsetList() (as opposed to map).

useOrderedFuncOffsets and OrderedFuncOffsets need to be renamed to remove "Ordered" because with this change there's no guarantee it's actually ordered.

[huangjd]

Rebase D149124 after compact binary deprecated

[huangjd]

llvm/include/llvm/ProfileData/SampleProfReader.h

640 ↗

(On Diff #519358)

This function is misleading and only used once. Data>=End does not imply at EOF in ExtBinary, so removing it.

[wenlei]

llvm/lib/ProfileData/SampleProfReader.cpp

730–740

we are using llvm-profdata tool

nit: as library code/comment, make no assumption of the specific tool as user. just say tools as opposed to compiler.

768

all prefixes of the matched function

You meant "all function profile with matching prefix/caller"?

773–776

Do we need a separate entry for this? Or merge it with the return false in the end?

[huangjd]

Rephrase some comments

[huangjd]

llvm/lib/ProfileData/SampleProfReader.cpp

773–776

I am keeping the logic in this order so that it is consistent with the control flow in readFuncProfiles because it is very complicated.

[davidxl]

llvm/lib/ProfileData/SampleProfReader.cpp

900

Add a comment here to reference to useFunctionOffsetList method to be in sync, or add assert of useFunctionOffsetList() in each branch below.

[wenlei]

llvm/lib/ProfileData/SampleProfReader.cpp

768

This comment is still inaccurate. The idea is to order the profiles so all profile sharing the same context prefix can be loaded together. i.e. If [A:1 @ B] is loaded, then [A:1 @ B:2 @ C], [A:1 @ B:3 @ D], [A:1 @ B:3 @ D:4 @ E] can all be loaded easily.

the profiles of all its prefix contexts (all callers on the call stack) are loaded

It's about loading callee profiles with matching caller context/prefix, rather than loading caller profiles.

773–776

sounds good.

[huangjd]

• Added comments and assert

[huangjd]

• Added comments and assert

[huangjd]

Included wrong diff

[wenlei]

lgtm, thanks.

[davidxl]

lgtm

llvm/lib/ProfileData/SampleProfReader.cpp

if (std::error_code EC = readCSNameTableSec())
       return EC;
     break;
   }
   case SecLBRProfile:
     if (std::error_code EC = readFuncProfiles())
       return EC;
     break;
   case SecFuncOffsetTable:
     // If module is absent, we are using LLVM tools, and need to read all
     // profiles, so skip reading the function offset table.
     if (!M) {
       Data = End;
     } else {
       assert((!ProfileIsCS ||
               hasSecFlag(Entry, SecFuncOffsetFlags::SecFlagOrdered)) &&
              "func offset table should always be sorted in CS profile");
       if (std::error_code EC = readFuncOffsetTable())
         return EC;
     }
     break;
   case SecFuncMetadata: {
     ProfileIsProbeBased =
         hasSecFlag(Entry, SecFuncMetadataFlags::SecFlagIsProbeBased);
     FunctionSamples::ProfileIsProbeBased = ProfileIsProbeBased;
     bool HasAttribute =
         hasSecFlag(Entry, SecFuncMetadataFlags::SecFlagHasAttribute);
     if (std::error_code EC = readFuncMetadata(HasAttribute))
std::error_code SampleProfileReaderExtBinaryBase::readOneSection(
   }
   return sampleprof_error::success;
 }
 
 bool SampleProfileReaderExtBinaryBase::useFuncOffsetList() const {
   // If profile is CS, the function offset section is expected to consist of
   // sequences of contexts in pre-order layout
   // (e.g. [A, A:1 @ B, A:1 @ B:2.3 @ C] [D, D:1 @ E]), so that when a matched
-  // context in the module is found, the profiles of all its prefix contexts (
-  // all callers on the call stack) are loaded. A list is needed since the order
-  // of profiles matters.
+  // context in the module is found, the profiles of all its callees are
+  // recursively loaded. A list is needed since the order of profiles matters.
   if (ProfileIsCS)
     return true;
 
   // If the profile is MD5, use the map container to lookup functions in
   // the module. A remapper has no use on MD5 names.
   if (useMD5())
     return false;
 
   // Profile is not MD5 and if a remapper is present, the remapped name of
   // every function needed to be matched against the module, so use the list
   // container since each entry is accessed.
   if (Remapper)
     return true;
 
   // Otherwise use the map container for faster lookup.
if (!LoadFuncsToBeUsed) {
     // Load function profiles on demand.
     if (Remapper) {
       for (auto Name : FuncsToUse) {
         Remapper->insert(Name);
       }
     }
 
     if (ProfileIsCS) {
+      assert(useFuncOffsetList());
       DenseSet<uint64_t> FuncGuidsToUse;
       if (useMD5()) {
         for (auto Name : FuncsToUse)
           FuncGuidsToUse.insert(Function::getGUID(Name));
       }
 
       // For each function in current module, load all context profiles for
       // the function as well as their callee contexts which can help profile
if (ProfileIsCS) {
             (CommonContext && CommonContext->IsPrefixOf(FContext))) {
           // Load profile for the current context which originated from
           // the common ancestor.
           const uint8_t *FuncProfileAddr = Start + NameOffset.second;
           if (std::error_code EC = readFuncProfile(FuncProfileAddr))
             return EC;
         }
       }
     } else if (useMD5()) {
+      assert(!useFuncOffsetList());
       for (auto Name : FuncsToUse) {
         auto GUID = std::to_string(MD5Hash(Name));
         auto iter = FuncOffsetTable.find(StringRef(GUID));
         if (iter == FuncOffsetTable.end())
           continue;
         const uint8_t *FuncProfileAddr = Start + iter->second;
         if (std::error_code EC = readFuncProfile(FuncProfileAddr))
           return EC;
       }
     } else if (Remapper) {
+      assert(useFuncOffsetList());
       for (auto NameOffset : FuncOffsetList) {
         SampleContext FContext(NameOffset.first);
         auto FuncName = FContext.getName();
         if (!FuncsToUse.count(FuncName) && !Remapper->exist(FuncName))
           continue;
         const uint8_t *FuncProfileAddr = Start + NameOffset.second;
         if (std::error_code EC = readFuncProfile(FuncProfileAddr))
           return EC;
       }
     } else {
+      assert(!useFuncOffsetList());
       for (auto Name : FuncsToUse) {
         auto iter = FuncOffsetTable.find(Name);
         if (iter == FuncOffsetTable.end())
           continue;
         const uint8_t *FuncProfileAddr = Start + iter->second;
         if (std::error_code EC = readFuncProfile(FuncProfileAddr))
           return EC;
       }