resolve review feedback

Author: mingmingl-llvm

llvm/include/llvm/ProfileData/DataAccessProf.h

@@ -70,9 +70,11 @@ struct DataAccessProfRecord {
 /// This class provides profile look-up, serialization and deserialization.
 class DataAccessProfData {
 public:
-  // SymbolID is either a string representing symbol name, or a uint64_t
-  // representing the content hash of a string literal.
-  using SymbolID = std::variant<StringRef, uint64_t>;
+  // SymbolID is either a string representing symbol name if the symbol has
+  // stable mangled name relative to source code, or a uint64_t representing the
+  // content hash of a string literal (with unstable name patterns like
+  // `.str.N[.llvm.hash]`). The StringRef is owned by the class's saver object.
+  using SymbolHandle = std::variant<StringRef, uint64_t>;
   using StringToIndexMap = llvm::MapVector<StringRef, uint64_t>;
 
   DataAccessProfData() : Saver(Allocator) {}
@@ -90,38 +92,32 @@ class DataAccessProfData {
   /// Returns a pointer of profile record for \p SymbolID, or nullptr if there
   /// isn't a record. Internally, this function will canonicalize the symbol
   /// name before the lookup.
-  const DataAccessProfRecord *getProfileRecord(const SymbolID SymID) const;
+  const DataAccessProfRecord *getProfileRecord(const SymbolHandle SymID) const;
 
   /// Returns true if \p SymID is seen in profiled binaries and cold.
-  bool isKnownColdSymbol(const SymbolID SymID) const;
+  bool isKnownColdSymbol(const SymbolHandle SymID) const;
 
-  /// Methods to add symbolized data access profile. Returns error if duplicated
+  /// Methods to set symbolized data access profile. Returns error if duplicated
   /// symbol names or content hashes are seen. The user of this class should
   /// aggregate counters that correspond to the same symbol name or with the
-  /// same string literal hash before calling 'add*' methods.
-  Error setDataAccessProfile(SymbolID SymbolID, uint64_t AccessCount);
+  /// same string literal hash before calling 'set*' methods.
+  Error setDataAccessProfile(SymbolHandle SymbolID, uint64_t AccessCount);
   /// Similar to the method above, for records with \p Locations representing
   /// the `filename:line` where this symbol shows up. Note because of linker's
   /// merge of identical symbols (e.g., unnamed_addr string literals), one
   /// symbol is likely to have multiple locations.
-  Error setDataAccessProfile(SymbolID SymbolID, uint64_t AccessCount,
-                             const llvm::SmallVector<DataLocation> &Locations);
-  Error addKnownSymbolWithoutSamples(SymbolID SymbolID);
+  Error setDataAccessProfile(SymbolHandle SymbolID, uint64_t AccessCount,
+                             ArrayRef<DataLocation> Locations);
+  Error addKnownSymbolWithoutSamples(SymbolHandle SymbolID);
 
   /// Returns an iterable StringRef for strings in the order they are added.
   /// Each string may be a symbol name or a file name.
   auto getStrings() const {
-    ArrayRef<std::pair<StringRef, uint64_t>> RefSymbolNames(
-        StrToIndexMap.begin(), StrToIndexMap.end());
-    return llvm::make_first_range(RefSymbolNames);
+    return llvm::make_first_range(StrToIndexMap.getArrayRef());
   }
 
   /// Returns array reference for various internal data structures.
-  ArrayRef<
-      std::pair<std::variant<StringRef, uint64_t>, DataAccessProfRecord>>
-  getRecords() const {
-    return Records.getArrayRef();
-  }
+  auto getRecords() const { return Records.getArrayRef(); }
   ArrayRef<StringRef> getKnownColdSymbols() const {
     return KnownColdSymbols.getArrayRef();
   }
@@ -137,23 +133,22 @@ class DataAccessProfData {
   /// start of the next payload.
   Error deserializeSymbolsAndFilenames(const unsigned char *&Ptr,
                                        const uint64_t NumSampledSymbols,
-                                       uint64_t NumColdKnownSymbols);
+                                       const uint64_t NumColdKnownSymbols);
 
   /// Decode the records and increment \p Ptr to the start of the next payload.
   Error deserializeRecords(const unsigned char *&Ptr);
 
   /// A helper function to compute a storage index for \p SymbolID.
-  uint64_t getEncodedIndex(const SymbolID SymbolID) const;
+  uint64_t getEncodedIndex(const SymbolHandle SymbolID) const;
 
   // Keeps owned copies of the input strings.
   // NOTE: Keep `Saver` initialized before other class members that reference
   // its string copies and destructed after they are destructed.
   llvm::BumpPtrAllocator Allocator;
   llvm::UniqueStringSaver Saver;
 
-  // `Records` stores the records and `SymbolToRecordIndex` maps a symbol ID to
-  // its record index.
-  MapVector<SymbolID, DataAccessProfRecord> Records;
+  // `Records` stores the records.
+  MapVector<SymbolHandle, DataAccessProfRecord> Records;
 
   // Use MapVector to keep input order of strings for serialization and
   // deserialization.

llvm/include/llvm/ProfileData/InstrProf.h

@@ -358,8 +358,9 @@ void createPGONameMetadata(GlobalObject &GO, StringRef PGOName);
 bool needsComdatForCounter(const GlobalObject &GV, const Module &M);
 
 /// \c NameStrings is a string composed of one or more possibly encoded
-/// sub-strings. The substrings are separated by 0 or more zero bytes. This
-/// method decodes the string and calls `NameCallback` for each substring.
+/// sub-strings. The substrings are separated by `\01` (returned by
+/// InstrProf.h:getInstrProfNameSeparator). This method decodes the string and
+/// calls `NameCallback` for each substring.
 Error readAndDecodeStrings(StringRef NameStrings,
                            std::function<Error(StringRef)> NameCallback);
 

llvm/lib/ProfileData/DataAccessProf.cpp

@@ -23,11 +23,22 @@ saveStringToMap(MapVector<StringRef, uint64_t> &Map,
   return Iter;
 }
 
+// Returns the canonical name or error.
+static Expected<StringRef> getCanonicalName(StringRef Name) {
+  if (Name.empty())
+    return make_error<StringError>("Empty symbol name",
+                                   llvm::errc::invalid_argument);
+  return InstrProfSymtab::getCanonicalName(Name);
+}
+
 const DataAccessProfRecord *
-DataAccessProfData::getProfileRecord(const SymbolID SymbolID) const {
+DataAccessProfData::getProfileRecord(const SymbolHandle SymbolID) const {
   auto Key = SymbolID;
-  if (std::holds_alternative<StringRef>(SymbolID))
-    Key = InstrProfSymtab::getCanonicalName(std::get<StringRef>(SymbolID));
+  if (std::holds_alternative<StringRef>(SymbolID)) {
+    StringRef Name = std::get<StringRef>(SymbolID);
+    assert(!Name.empty() && "Empty symbol name");
+    Key = InstrProfSymtab::getCanonicalName(Name);
+  }
 
   auto It = Records.find(Key);
   if (It != Records.end())
@@ -36,30 +47,27 @@ DataAccessProfData::getProfileRecord(const SymbolID SymbolID) const {
   return nullptr;
 }
 
-bool DataAccessProfData::isKnownColdSymbol(const SymbolID SymID) const {
+bool DataAccessProfData::isKnownColdSymbol(const SymbolHandle SymID) const {
   if (std::holds_alternative<uint64_t>(SymID))
     return KnownColdHashes.contains(std::get<uint64_t>(SymID));
   return KnownColdSymbols.contains(std::get<StringRef>(SymID));
 }
 
-Error DataAccessProfData::setDataAccessProfile(SymbolID Symbol,
+Error DataAccessProfData::setDataAccessProfile(SymbolHandle Symbol,
                                                uint64_t AccessCount) {
   uint64_t RecordID = -1;
   bool IsStringLiteral = false;
-  SymbolID Key;
+  SymbolHandle Key;
   if (std::holds_alternative<uint64_t>(Symbol)) {
     RecordID = std::get<uint64_t>(Symbol);
     Key = RecordID;
     IsStringLiteral = true;
   } else {
-    StringRef SymbolName = std::get<StringRef>(Symbol);
-    if (SymbolName.empty())
-      return make_error<StringError>("Empty symbol name",
-                                     llvm::errc::invalid_argument);
-
-    StringRef CanonicalName = InstrProfSymtab::getCanonicalName(SymbolName);
-    Key = CanonicalName;
-    RecordID = saveStringToMap(StrToIndexMap, Saver, CanonicalName)->second;
+    auto CanonicalName = getCanonicalName(std::get<StringRef>(Symbol));
+    if (!CanonicalName)
+      return CanonicalName.takeError();
+    std::tie(Key, RecordID) =
+        *saveStringToMap(StrToIndexMap, Saver, *CanonicalName);
     IsStringLiteral = false;
   }
 
@@ -75,8 +83,8 @@ Error DataAccessProfData::setDataAccessProfile(SymbolID Symbol,
 }
 
 Error DataAccessProfData::setDataAccessProfile(
-    SymbolID SymbolID, uint64_t AccessCount,
-    const llvm::SmallVector<DataLocation> &Locations) {
+    SymbolHandle SymbolID, uint64_t AccessCount,
+    ArrayRef<DataLocation> Locations) {
   if (Error E = setDataAccessProfile(SymbolID, AccessCount))
     return E;
 
@@ -89,17 +97,15 @@ Error DataAccessProfData::setDataAccessProfile(
   return Error::success();
 }
 
-Error DataAccessProfData::addKnownSymbolWithoutSamples(SymbolID SymbolID) {
+Error DataAccessProfData::addKnownSymbolWithoutSamples(SymbolHandle SymbolID) {
   if (std::holds_alternative<uint64_t>(SymbolID)) {
     KnownColdHashes.insert(std::get<uint64_t>(SymbolID));
     return Error::success();
   }
-  StringRef SymbolName = std::get<StringRef>(SymbolID);
-  if (SymbolName.empty())
-    return make_error<StringError>("Empty symbol name",
-                                   llvm::errc::invalid_argument);
-  StringRef CanonicalSymName = InstrProfSymtab::getCanonicalName(SymbolName);
-  KnownColdSymbols.insert(CanonicalSymName);
+  auto CanonicalName = getCanonicalName(std::get<StringRef>(SymbolID));
+  if (!CanonicalName)
+    return CanonicalName.takeError();
+  KnownColdSymbols.insert(*CanonicalName);
   return Error::success();
 }
 
@@ -142,7 +148,7 @@ Error DataAccessProfData::serializeSymbolsAndFilenames(ProfOStream &OS) const {
   OS.write(CompressedStringLen);
   // Write the chars in compressed strings.
   for (char C : CompressedStrings)
-    OS.writeByte(static_cast<uint8_t>(c));
+    OS.writeByte(static_cast<uint8_t>(C));
   // Pad up to a multiple of 8.
   // InstrProfReader could read bytes according to 'CompressedStringLen'.
   const uint64_t PaddedLength = alignTo(CompressedStringLen, 8);
@@ -151,11 +157,15 @@ Error DataAccessProfData::serializeSymbolsAndFilenames(ProfOStream &OS) const {
   return Error::success();
 }
 
-uint64_t DataAccessProfData::getEncodedIndex(const SymbolID SymbolID) const {
+uint64_t
+DataAccessProfData::getEncodedIndex(const SymbolHandle SymbolID) const {
   if (std::holds_alternative<uint64_t>(SymbolID))
     return std::get<uint64_t>(SymbolID);
 
-  return StrToIndexMap.find(std::get<StringRef>(SymbolID))->second;
+  auto Iter = StrToIndexMap.find(std::get<StringRef>(SymbolID));
+  assert(Iter != StrToIndexMap.end() &&
+         "String literals not found in StrToIndexMap");
+  return Iter->second;
 }
 
 Error DataAccessProfData::serialize(ProfOStream &OS) const {
@@ -179,13 +189,13 @@ Error DataAccessProfData::serialize(ProfOStream &OS) const {
 }
 
 Error DataAccessProfData::deserializeSymbolsAndFilenames(
-    const unsigned char *&Ptr, uint64_t NumSampledSymbols,
-    uint64_t NumColdKnownSymbols) {
+    const unsigned char *&Ptr, const uint64_t NumSampledSymbols,
+    const uint64_t NumColdKnownSymbols) {
   uint64_t Len =
       support::endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
 
-  // With M=NumSampledSymbols and N=NumColdKnownSymbols, the first M strings are
-  // symbols with samples, and next N strings are known cold symbols.
+  // The first NumSampledSymbols strings are symbols with samples, and next
+  // NumColdKnownSymbols strings are known cold symbols.
   uint64_t StringCnt = 0;
   std::function<Error(StringRef)> addName = [&](StringRef Name) {
     if (StringCnt < NumSampledSymbols)
@@ -219,7 +229,7 @@ Error DataAccessProfData::deserializeRecords(const unsigned char *&Ptr) {
     uint64_t AccessCount =
         support::endian::readNext<uint64_t, llvm::endianness::little>(Ptr);
 
-    SymbolID SymbolID;
+    SymbolHandle SymbolID;
     if (IsStringLiteral)
       SymbolID = ID;
     else

llvm/unittests/ProfileData/CMakeLists.txt

@@ -10,6 +10,7 @@ set(LLVM_LINK_COMPONENTS
 add_llvm_unittest(ProfileDataTests
   BPFunctionNodeTest.cpp
   CoverageMappingTest.cpp
+  DataAccessProfTest.cpp
   InstrProfDataTest.cpp
   InstrProfTest.cpp
   ItaniumManglingCanonicalizerTest.cpp