[nfc][pgo] const-ify some APIs in InstrProfSymtab

[mtrofin]

The main reason some const - sounding APIs weren't const was because their state is lazily updated (ensuring ordering).

[mtrofin]

• [nfc][pgo] const-ify some APIs in InstrProfSymtab #153284 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Mircea Trofin (mtrofin)

Changes

The main reason some const - sounding APIs weren't const was because their state is lazily updated (ensuring ordering).

---

Full diff: https://github.com/llvm/llvm-project/pull/153284.diff

3 Files Affected:

• (modified) llvm/include/llvm/ProfileData/InstrProf.h (+22-21)
• (modified) llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp (+1-1)
• (modified) llvm/lib/ProfileData/InstrProf.cpp (+2-2)

diff --git a/llvm/include/llvm/ProfileData/InstrProf.h b/llvm/include/llvm/ProfileData/InstrProf.h
index f53602424b583..070fe3bc0c3f0 100644
--- a/llvm/include/llvm/ProfileData/InstrProf.h
+++ b/llvm/include/llvm/ProfileData/InstrProf.h
@@ -526,24 +526,24 @@ class InstrProfSymtab {
   // so it doesn't use a StringSet for function names.
   StringSet<> VTableNames;
   // A map from MD5 keys to function name strings.
-  std::vector<std::pair<uint64_t, StringRef>> MD5NameMap;
+  mutable std::vector<std::pair<uint64_t, StringRef>> MD5NameMap;
   // A map from MD5 keys to function define. We only populate this map
   // when build the Symtab from a Module.
-  std::vector<std::pair<uint64_t, Function *>> MD5FuncMap;
+  mutable std::vector<std::pair<uint64_t, Function *>> MD5FuncMap;
   // A map from MD5 to the global variable. This map is only populated when
   // building the symtab from a module. Use separate container instances for
   // `MD5FuncMap` and `MD5VTableMap`.
   // TODO: Unify the container type and the lambda function 'mapName' inside
   // add{Func,VTable}WithName.
-  DenseMap<uint64_t, GlobalVariable *> MD5VTableMap;
+  mutable DenseMap<uint64_t, GlobalVariable *> MD5VTableMap;
   // A map from function runtime address to function name MD5 hash.
   // This map is only populated and used by raw instr profile reader.
-  AddrHashMap AddrToMD5Map;
+  mutable AddrHashMap AddrToMD5Map;
 
   AddrIntervalMap::Allocator VTableAddrMapAllocator;
   // This map is only populated and used by raw instr profile reader.
   AddrIntervalMap VTableAddrMap;
-  bool Sorted = false;
+  mutable bool Sorted = false;
 
   static StringRef getExternalSymbol() { return "** External Symbol **"; }
 
@@ -566,7 +566,7 @@ class InstrProfSymtab {
   // finalizeSymtab needs to be called before looking up function names.
   // This is required because the underlying map is a vector (for space
   // efficiency) which needs to be sorted.
-  inline void finalizeSymtab();
+  inline void finalizeSymtab() const;
 
 public:
   InstrProfSymtab() : VTableAddrMap(VTableAddrMapAllocator) {}
@@ -676,24 +676,25 @@ class InstrProfSymtab {
   }
 
   /// Return a function's hash, or 0, if the function isn't in this SymTab.
-  LLVM_ABI uint64_t getFunctionHashFromAddress(uint64_t Address);
+  LLVM_ABI uint64_t getFunctionHashFromAddress(uint64_t Address) const;
 
   /// Return a vtable's hash, or 0 if the vtable doesn't exist in this SymTab.
-  LLVM_ABI uint64_t getVTableHashFromAddress(uint64_t Address);
+  LLVM_ABI uint64_t getVTableHashFromAddress(uint64_t Address) const;
 
   /// Return function's PGO name from the function name's symbol
   /// address in the object file. If an error occurs, return
   /// an empty string.
-  LLVM_ABI StringRef getFuncName(uint64_t FuncNameAddress, size_t NameSize);
+  LLVM_ABI StringRef getFuncName(uint64_t FuncNameAddress,
+                                 size_t NameSize) const;
 
   /// Return name of functions or global variables from the name's md5 hash
   /// value. If not found, return an empty string.
-  inline StringRef getFuncOrVarName(uint64_t ValMD5Hash);
+  inline StringRef getFuncOrVarName(uint64_t ValMD5Hash) const;
 
   /// Just like getFuncOrVarName, except that it will return literal string
   /// 'External Symbol' if the function or global variable is external to
   /// this symbol table.
-  inline StringRef getFuncOrVarNameIfDefined(uint64_t ValMD5Hash);
+  inline StringRef getFuncOrVarNameIfDefined(uint64_t ValMD5Hash) const;
 
   /// True if Symbol is the value used to represent external symbols.
   static bool isExternalSymbol(const StringRef &Symbol) {
@@ -701,11 +702,11 @@ class InstrProfSymtab {
   }
 
   /// Return function from the name's md5 hash. Return nullptr if not found.
-  inline Function *getFunction(uint64_t FuncMD5Hash);
+  inline Function *getFunction(uint64_t FuncMD5Hash) const;
 
   /// Return the global variable corresponding to md5 hash. Return nullptr if
   /// not found.
-  inline GlobalVariable *getGlobalVariable(uint64_t MD5Hash);
+  inline GlobalVariable *getGlobalVariable(uint64_t MD5Hash) const;
 
   /// Return the name section data.
   inline StringRef getNameData() const { return Data; }
@@ -748,7 +749,7 @@ Error InstrProfSymtab::create(const FuncNameIterRange &FuncIterRange,
   return Error::success();
 }
 
-void InstrProfSymtab::finalizeSymtab() {
+void InstrProfSymtab::finalizeSymtab() const {
   if (Sorted)
     return;
   llvm::sort(MD5NameMap, less_first());
@@ -758,14 +759,14 @@ void InstrProfSymtab::finalizeSymtab() {
   Sorted = true;
 }
 
-StringRef InstrProfSymtab::getFuncOrVarNameIfDefined(uint64_t MD5Hash) {
-  StringRef ret = getFuncOrVarName(MD5Hash);
-  if (ret.empty())
+StringRef InstrProfSymtab::getFuncOrVarNameIfDefined(uint64_t MD5Hash) const {
+  StringRef Ret = getFuncOrVarName(MD5Hash);
+  if (Ret.empty())
     return InstrProfSymtab::getExternalSymbol();
-  return ret;
+  return Ret;
 }
 
-StringRef InstrProfSymtab::getFuncOrVarName(uint64_t MD5Hash) {
+StringRef InstrProfSymtab::getFuncOrVarName(uint64_t MD5Hash) const {
   finalizeSymtab();
   auto Result = llvm::lower_bound(MD5NameMap, MD5Hash,
                                   [](const std::pair<uint64_t, StringRef> &LHS,
@@ -775,7 +776,7 @@ StringRef InstrProfSymtab::getFuncOrVarName(uint64_t MD5Hash) {
   return StringRef();
 }
 
-Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {
+Function *InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) const {
   finalizeSymtab();
   auto Result = llvm::lower_bound(MD5FuncMap, FuncMD5Hash,
                                   [](const std::pair<uint64_t, Function *> &LHS,
@@ -785,7 +786,7 @@ Function* InstrProfSymtab::getFunction(uint64_t FuncMD5Hash) {
   return nullptr;
 }
 
-GlobalVariable *InstrProfSymtab::getGlobalVariable(uint64_t MD5Hash) {
+GlobalVariable *InstrProfSymtab::getGlobalVariable(uint64_t MD5Hash) const {
   return MD5VTableMap.lookup(MD5Hash);
 }
 
diff --git a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
index cdf4412c6477a..fc2577e6ada5d 100644
--- a/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
+++ b/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp
@@ -519,7 +519,7 @@ Error InstrProfSymtab::create(SectionRef &Section) {
   return Error::success();
 }
 
-StringRef InstrProfSymtab::getFuncName(uint64_t Pointer, size_t Size) {
+StringRef InstrProfSymtab::getFuncName(uint64_t Pointer, size_t Size) const {
   if (Pointer < Address)
     return StringRef();
   auto Offset = Pointer - Address;
diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index 542572975dc89..7885e127c3418 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -684,13 +684,13 @@ Error InstrProfSymtab::addFuncWithName(Function &F, StringRef PGOFuncName,
   return Error::success();
 }
 
-uint64_t InstrProfSymtab::getVTableHashFromAddress(uint64_t Address) {
+uint64_t InstrProfSymtab::getVTableHashFromAddress(uint64_t Address) const {
   // Given a runtime address, look up the hash value in the interval map, and
   // fallback to value 0 if a hash value is not found.
   return VTableAddrMap.lookup(Address, 0);
 }
 
-uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) {
+uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) const {
   finalizeSymtab();
   auto It = partition_point(AddrToMD5Map, [=](std::pair<uint64_t, uint64_t> A) {
     return A.first < Address;

[teresajohnson]

This seems like an odd one to be marked const, as the name implies mutation.

[mtrofin]

yes, but it's the API that's performing the mutation on the mutable state.

[mtrofin]

added a comment

[teresajohnson]

The description of the method says "If the symtab is created by a series of calls to \c addFuncName, \c
// finalizeSymtab needs to be called before looking up function names". This to me makes it sound like it is a user-visible action, notifying the symtab that the user is done adding functions. The usage is a little confusing though - it is called by both the create methods (clearly mutable), and by some get* methods (now non-mutable). Maybe the comment needs to be clarified?

[mtrofin]

it's a private API, I'm reading the documentation as "for implementers" - wdyt?

[teresajohnson]

lgtm

[mtrofin]

Merge activity

• Aug 13, 4:06 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Aug 13, 4:08 PM UTC: @mtrofin merged this pull request with Graphite.