[llvm] r328163 - [InstrProf] Encapsulates access to AddrToMD5Map.

Mircea Trofin via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 21 15:27:31 PDT 2018


Author: mtrofin
Date: Wed Mar 21 15:27:31 2018
New Revision: 328163

URL: http://llvm.org/viewvc/llvm-project?rev=328163&view=rev
Log:
[InstrProf] Encapsulates access to AddrToMD5Map.

Summary:
This fixes a unittest failure introduced by D44717

D44717 introduced lazy sorting of the internal data structures of the
symbol table. The AddrToMD5Map getter was potentially exposing
inconsistent (unsorted) state. We could sort in the accessor, however,
a client may store the pointer and thus bypass the internal state
management of the symbol table. The alternative in this CL blocks
direct access to the state, thus ensuring consistent
externally-observable state.

Reviewers: davidxl, xur, eraman

Reviewed By: xur

Subscribers: llvm-commits

Differential Revision: https://reviews.llvm.org/D44757

Modified:
    llvm/trunk/include/llvm/ProfileData/InstrProf.h
    llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
    llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
    llvm/trunk/lib/ProfileData/InstrProf.cpp
    llvm/trunk/lib/ProfileData/InstrProfReader.cpp
    llvm/trunk/unittests/ProfileData/InstrProfTest.cpp

Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Mar 21 15:27:31 2018
@@ -487,7 +487,8 @@ public:
     AddrToMD5Map.push_back(std::make_pair(Addr, MD5Val));
   }
 
-  AddrHashMap &getAddrHashMap() { return AddrToMD5Map; }
+  /// Return a function's hash, or 0, if the function isn't in this SymTab.
+  uint64_t getFunctionHashFromAddress(uint64_t Address);
 
   /// Return function's PGO name from the function name's symbol
   /// address in the object file. If an error occurs, return
@@ -643,8 +644,6 @@ struct InstrProfRecord {
     return *this;
   }
 
-  using ValueMapType = std::vector<std::pair<uint64_t, uint64_t>>;
-
   /// Return the number of value profile kinds with non-zero number
   /// of profile sites.
   inline uint32_t getNumValueKinds() const;
@@ -678,7 +677,7 @@ struct InstrProfRecord {
   /// Add ValueData for ValueKind at value Site.
   void addValueData(uint32_t ValueKind, uint32_t Site,
                     InstrProfValueData *VData, uint32_t N,
-                    ValueMapType *ValueMap);
+                    InstrProfSymtab *SymTab);
 
   /// Merge the counts in \p Other into this one.
   /// Optionally scale merged counts by \p Weight.
@@ -752,7 +751,7 @@ private:
 
   // Map indirect call target name hash to name string.
   uint64_t remapValue(uint64_t Value, uint32_t ValueKind,
-                      ValueMapType *HashKeys);
+                      InstrProfSymtab *SymTab);
 
   // Merge Value Profile data from Src record to this record for ValueKind.
   // Scale merged value counts by \p Weight.

Modified: llvm/trunk/include/llvm/ProfileData/InstrProfData.inc
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProfData.inc?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProfData.inc (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProfData.inc Wed Mar 21 15:27:31 2018
@@ -315,7 +315,7 @@ typedef struct ValueProfRecord {
    * \brief Read data from this record and save it to Record.
    */
   void deserializeTo(InstrProfRecord &Record,
-                     InstrProfRecord::ValueMapType *VMap);
+                     InstrProfSymtab *SymTab);
   /*
    * In-place byte swap:
    * Do byte swap for this instance. \c Old is the original order before
@@ -393,7 +393,7 @@ typedef struct ValueProfData {
    * Read data from this data and save it to \c Record.
    */
   void deserializeTo(InstrProfRecord &Record,
-                     InstrProfRecord::ValueMapType *VMap);
+                     InstrProfSymtab *SymTab);
   void operator delete(void *ptr) { ::operator delete(ptr); }
 #endif
 } ValueProfData;

Modified: llvm/trunk/include/llvm/ProfileData/InstrProfReader.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProfReader.h?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ProfileData/InstrProfReader.h (original)
+++ llvm/trunk/include/llvm/ProfileData/InstrProfReader.h Wed Mar 21 15:27:31 2018
@@ -199,8 +199,6 @@ private:
   uint32_t ValueKindLast;
   uint32_t CurValueDataSize;
 
-  InstrProfRecord::ValueMapType FunctionPtrToNameMap;
-
 public:
   RawInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer)
       : DataBuffer(std::move(DataBuffer)) {}

Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Mar 21 15:27:31 2018
@@ -360,6 +360,21 @@ Error InstrProfSymtab::create(Module &M,
   return Error::success();
 }
 
+uint64_t InstrProfSymtab::getFunctionHashFromAddress(uint64_t Address) {
+  finalizeSymtab();
+  auto Result =
+      std::lower_bound(AddrToMD5Map.begin(), AddrToMD5Map.end(), Address,
+                       [](const std::pair<uint64_t, uint64_t> &LHS,
+                          uint64_t RHS) { return LHS.first < RHS; });
+  // Raw function pointer collected by value profiler may be from
+  // external functions that are not instrumented. They won't have
+  // mapping data to be used by the deserializer. Force the value to
+  // be 0 in this case.
+  if (Result != AddrToMD5Map.end() && Result->first == Address)
+    return (uint64_t)Result->second;
+  return 0;
+}
+
 Error collectPGOFuncNameStrings(ArrayRef<std::string> NameStrs,
                                 bool doCompression, std::string &Result) {
   assert(!NameStrs.empty() && "No name data to emit");
@@ -560,32 +575,19 @@ void InstrProfRecord::scale(uint64_t Wei
 
 // Map indirect call target name hash to name string.
 uint64_t InstrProfRecord::remapValue(uint64_t Value, uint32_t ValueKind,
-                                     ValueMapType *ValueMap) {
-  if (!ValueMap)
+                                     InstrProfSymtab *SymTab) {
+  if (!SymTab)
     return Value;
-  switch (ValueKind) {
-  case IPVK_IndirectCallTarget: {
-    auto Result =
-        std::lower_bound(ValueMap->begin(), ValueMap->end(), Value,
-                         [](const std::pair<uint64_t, uint64_t> &LHS,
-                            uint64_t RHS) { return LHS.first < RHS; });
-   // Raw function pointer collected by value profiler may be from 
-   // external functions that are not instrumented. They won't have
-   // mapping data to be used by the deserializer. Force the value to
-   // be 0 in this case.
-    if (Result != ValueMap->end() && Result->first == Value)
-      Value = (uint64_t)Result->second;
-    else
-      Value = 0;
-    break;
-  }
-  }
+
+  if (ValueKind == IPVK_IndirectCallTarget)
+    return SymTab->getFunctionHashFromAddress(Value);
+
   return Value;
 }
 
 void InstrProfRecord::addValueData(uint32_t ValueKind, uint32_t Site,
                                    InstrProfValueData *VData, uint32_t N,
-                                   ValueMapType *ValueMap) {
+                                   InstrProfSymtab *ValueMap) {
   for (uint32_t I = 0; I < N; I++) {
     VData[I].Value = remapValue(VData[I].Value, ValueKind, ValueMap);
   }
@@ -665,13 +667,13 @@ ValueProfData::serializeFrom(const Instr
 }
 
 void ValueProfRecord::deserializeTo(InstrProfRecord &Record,
-                                    InstrProfRecord::ValueMapType *VMap) {
+                                    InstrProfSymtab *SymTab) {
   Record.reserveSites(Kind, NumValueSites);
 
   InstrProfValueData *ValueData = getValueProfRecordValueData(this);
   for (uint64_t VSite = 0; VSite < NumValueSites; ++VSite) {
     uint8_t ValueDataCount = this->SiteCountArray[VSite];
-    Record.addValueData(Kind, VSite, ValueData, ValueDataCount, VMap);
+    Record.addValueData(Kind, VSite, ValueData, ValueDataCount, SymTab);
     ValueData += ValueDataCount;
   }
 }
@@ -705,13 +707,13 @@ void ValueProfRecord::swapBytes(support:
 }
 
 void ValueProfData::deserializeTo(InstrProfRecord &Record,
-                                  InstrProfRecord::ValueMapType *VMap) {
+                                  InstrProfSymtab *SymTab) {
   if (NumValueKinds == 0)
     return;
 
   ValueProfRecord *VR = getFirstValueProfRecord(this);
   for (uint32_t K = 0; K < NumValueKinds; K++) {
-    VR->deserializeTo(Record, VMap);
+    VR->deserializeTo(Record, SymTab);
     VR = getValueProfRecordNext(VR);
   }
 }

Modified: llvm/trunk/lib/ProfileData/InstrProfReader.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProfReader.cpp?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/lib/ProfileData/InstrProfReader.cpp (original)
+++ llvm/trunk/lib/ProfileData/InstrProfReader.cpp Wed Mar 21 15:27:31 2018
@@ -438,7 +438,7 @@ Error RawInstrProfReader<IntPtrT>::readV
   // Note that besides deserialization, this also performs the conversion for
   // indirect call targets.  The function pointers from the raw profile are
   // remapped into function name hashes.
-  VDataPtrOrErr.get()->deserializeTo(Record, &Symtab->getAddrHashMap());
+  VDataPtrOrErr.get()->deserializeTo(Record, Symtab.get());
   CurValueDataSize = VDataPtrOrErr.get()->getSize();
   return success();
 }

Modified: llvm/trunk/unittests/ProfileData/InstrProfTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ProfileData/InstrProfTest.cpp?rev=328163&r1=328162&r2=328163&view=diff
==============================================================================
--- llvm/trunk/unittests/ProfileData/InstrProfTest.cpp (original)
+++ llvm/trunk/unittests/ProfileData/InstrProfTest.cpp Wed Mar 21 15:27:31 2018
@@ -770,7 +770,7 @@ TEST_P(MaybeSparseInstrProfTest, value_p
   Symtab.mapAddress(uint64_t(callee4), 0x4000ULL);
   // Missing mapping for callee5
 
-  VPData->deserializeTo(Record, &Symtab.getAddrHashMap());
+  VPData->deserializeTo(Record, &Symtab);
 
   // Now read data from Record and sanity check the data
   ASSERT_EQ(5U, Record.getNumValueSites(IPVK_IndirectCallTarget));




More information about the llvm-commits mailing list