[PGO] Value profiling text format reader/writer support

[davidxl]

Summary

This patch adds the missing functionality in parsable text format support for value profiling.

[davidxl]

Ping ..

[vsk]

Sorry for the latency, comments inline --

lib/ProfileData/InstrProfReader.cpp

107 ↗

(On Diff #41810)

I'd prefer to keep format changes for separate NFC commits.

127 ↗

(On Diff #41810)

This looks correct but a bit repetitive. What do you think of using a macro like:

#define AdvanceAndReadInt(Line, Dst) \
    if ((Line).is_at_end()) \
        return error(instrprof_error::truncated); \
    if (((Line)++)->getAsInteger(10, (Dst)))
        return error(instrprof_error::truncated); \

You could move the various "uint32_t" definitions up, or define them within the macro.

211 ↗

(On Diff #41810)

This could use a comment like "// Check if we have value profile data, and if so, read it.".

test/tools/llvm-profdata/Inputs/vp-malform.proftext

5 ↗

(On Diff #41810)

I don't think these input files should have RUN lines.

43 ↗

(On Diff #41810)

It's not obvious that this is the error. Please add notes to your tests which make it clear what's being tested.

48 ↗

(On Diff #41810)

Ditto for CHECK lines.

test/tools/llvm-profdata/Inputs/vp-malform2.proftext

5 ↗

(On Diff #41810)

Same comments from the last file.

Could you trim this file down a bit? It seems very similar to the last one.

test/tools/llvm-profdata/Inputs/vp-truncate.proftext

5 ↗

(On Diff #41810)

Same note about the RUN lines.

[davidxl]

lib/ProfileData/InstrProfReader.cpp

107 ↗

(On Diff #41810)

This was fixed already.

127 ↗

(On Diff #41810)

good suggestion.

test/tools/llvm-profdata/Inputs/vp-malform.proftext

43 ↗

(On Diff #41810)

Added a comment before the malformed line

[davidxl]

Update the patch addressing vsk's comments.

[vsk]

Thanks, lgtm with two minor changes.

lib/ProfileData/InstrProfReader.cpp

112 ↗

(On Diff #42643)

Could you move these inside readValueProfileData and #undef them at the end of the function?

113 ↗

(On Diff #42643)

These macro function arguments should be wrapped in parens.

[davidxl]

Done.

thanks,

David

llvm/trunk/include/llvm/ProfileData/InstrProfReader.h

 /// counters, and then each counter value, in that order.
 class TextInstrProfReader : public InstrProfReader {
 private:
   /// The profile data file contents.
   std::unique_ptr<MemoryBuffer> DataBuffer;
   /// Iterator over the profile data.
   line_iterator Line;
 
+  // String table for holding a unique copy of all the strings in the profile.
+  InstrProfStringTable StringTable;
+
   TextInstrProfReader(const TextInstrProfReader &) = delete;
   TextInstrProfReader &operator=(const TextInstrProfReader &) = delete;
+  std::error_code readValueProfileData(InstrProfRecord &Record);
+
 public:
   TextInstrProfReader(std::unique_ptr<MemoryBuffer> DataBuffer_)
       : DataBuffer(std::move(DataBuffer_)), Line(*DataBuffer, true, '#') {}
 
   /// Return true if the given buffer is in text instrprof format.
   static bool hasFormat(const MemoryBuffer &Buffer);
 
   /// Read the header.

llvm/trunk/lib/ProfileData/InstrProfReader.cpp

bool TextInstrProfReader::hasFormat(const MemoryBuffer &Buffer) {
   // 'reasonable' number of characters (up to profile magic size).
   size_t count = std::min(Buffer.getBufferSize(), sizeof(uint64_t));
   StringRef buffer = Buffer.getBufferStart();
   return count == 0 ||
          std::all_of(buffer.begin(), buffer.begin() + count,
                      [](char c) { return ::isprint(c) || ::isspace(c); });
 }
 
+std::error_code
+TextInstrProfReader::readValueProfileData(InstrProfRecord &Record) {
+
+#define CHECK_LINE_END(Line)                                                   \
+  if (Line.is_at_end())                                                        \
+    return error(instrprof_error::truncated);
+#define READ_NUM(Str, Dst)                                                     \
+  if ((Str).getAsInteger(10, (Dst)))                                           \
+    return error(instrprof_error::malformed);
+#define VP_READ_ADVANCE(Val)                                                   \
+  CHECK_LINE_END(Line);                                                        \
+  uint32_t Val;                                                                \
+  READ_NUM((*Line), (Val));                                                    \
+  Line++;
+
+  if (Line.is_at_end())
+    return success();
+  uint32_t NumValueKinds;
+  if (Line->getAsInteger(10, NumValueKinds)) {
+    // No value profile data
+    return success();
+  }
+  if (NumValueKinds == 0 || NumValueKinds > IPVK_Last + 1)
+    return error(instrprof_error::malformed);
+  Line++;
+
+  for (uint32_t VK = 0; VK < NumValueKinds; VK++) {
+    VP_READ_ADVANCE(ValueKind);
+    if (ValueKind > IPVK_Last)
+      return error(instrprof_error::malformed);
+    VP_READ_ADVANCE(NumValueSites);
+    if (!NumValueSites)
+      continue;
+
+    Record.reserveSites(VK, NumValueSites);
+    for (uint32_t S = 0; S < NumValueSites; S++) {
+      VP_READ_ADVANCE(NumValueData);
+
+      std::vector<InstrProfValueData> CurrentValues;
+      for (uint32_t V = 0; V < NumValueData; V++) {
+        CHECK_LINE_END(Line);
+        std::pair<StringRef, StringRef> VD = Line->split(':');
+        uint64_t TakenCount, Value;
+        READ_NUM(VD.second, TakenCount);
+        if (VK == IPVK_IndirectCallTarget)
+          Value = (uint64_t)StringTable.insertString(VD.first);
+        else {
+          READ_NUM(VD.first, Value);
+        }
+        CurrentValues.push_back({Value, TakenCount});
+        Line++;
+      }
+      Record.addValueData(VK, S, CurrentValues.data(), NumValueData, nullptr);
+    }
+  }
+  return success();
+
+#undef CHECK_LINE_END
+#undef READ_NUM
+#undef VP_READ_ADVANCE
+}
+
 std::error_code TextInstrProfReader::readNextRecord(InstrProfRecord &Record) {
   // Skip empty lines and comments.
   while (!Line.is_at_end() && (Line->empty() || Line->startswith("#")))
     ++Line;
   // If we hit EOF while looking for a name, we're done.
   if (Line.is_at_end())
     return error(instrprof_error::eof);
 
for (uint64_t I = 0; I < NumCounters; ++I) {
     if (Line.is_at_end())
       return error(instrprof_error::truncated);
     uint64_t Count;
     if ((Line++)->getAsInteger(10, Count))
       return error(instrprof_error::malformed);
     Record.Counts.push_back(Count);
   }
 
+  // Check if value profile data exists and read it if so.
+  if (std::error_code EC = readValueProfileData(Record))
+    return EC;
+
   return success();
 }
 
 template <class IntPtrT>
 bool RawInstrProfReader<IntPtrT>::hasFormat(const MemoryBuffer &DataBuffer) {
   if (DataBuffer.getBufferSize() < sizeof(uint64_t))
     return false;
   uint64_t Magic =

llvm/trunk/lib/ProfileData/InstrProfWriter.cpp

void InstrProfWriter::write(raw_fd_ostream &OS) {
 
   // Go back and fill in the hash table start.
   using namespace support;
   OS.seek(TableStart.first);
   // Now patch the HashOffset field previously reserved.
   endian::Writer<little>(OS).write<uint64_t>(TableStart.second);
 }
 
+static const char *ValueProfKindStr[] = {
+#define VALUE_PROF_KIND(Enumerator, Value) #Enumerator,
+#include "llvm/ProfileData/InstrProfData.inc"
+};
+
 void InstrProfWriter::writeRecordInText(const InstrProfRecord &Func,
                                         raw_fd_ostream &OS) {
   OS << Func.Name << "\n";
   OS << "# Func Hash:\n" << Func.Hash << "\n";
-  OS << "# Num Counters:\n" <<Func.Counts.size() << "\n";
+  OS << "# Num Counters:\n" << Func.Counts.size() << "\n";
   OS << "# Counter Values:\n";
   for (uint64_t Count : Func.Counts)
     OS << Count << "\n";
 
+  uint32_t NumValueKinds = Func.getNumValueKinds();
+  if (!NumValueKinds) {
+    OS << "\n";
+    return;
+  }
+
+  OS << "# Num Value Kinds:\n" << Func.getNumValueKinds() << "\n";
+  for (uint32_t VK = 0; VK < IPVK_Last + 1; VK++) {
+    uint32_t NS = Func.getNumValueSites(VK);
+    if (!NS)
+      continue;
+    OS << "# ValueKind = " << ValueProfKindStr[VK] << ":\n" << VK << "\n";
+    OS << "# NumValueSites:\n" << NS << "\n";
+    for (uint32_t S = 0; S < NS; S++) {
+      uint32_t ND = Func.getNumValueDataForSite(VK, S);
+      OS << ND << "\n";
+      std::unique_ptr<InstrProfValueData[]> VD = Func.getValueForSite(VK, S);
+      for (uint32_t I = 0; I < ND; I++) {
+        if (VK == IPVK_IndirectCallTarget)
+          OS << reinterpret_cast<const char *>(VD[I].Value) << ":"
+             << VD[I].Count << "\n";
+        else
+          OS << VD[I].Value << ":" << VD[I].Count << "\n";
+      }
+    }
+  }
+
   OS << "\n";
 }
 
 void InstrProfWriter::writeText(raw_fd_ostream &OS) {
   for (const auto &I : FunctionData)
     for (const auto &Func : I.getValue())
       writeRecordInText(Func.second, OS);
 }

llvm/trunk/test/tools/llvm-profdata/Inputs/vp-malform.proftext

+foo
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+999000
+359800
+
+foo2
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+1001000
+360200
+
+main
+# Func Hash:
+16650
+# Num Counters:
+4
+# Counter Values:
+2
+2000
+2000000
+999000
+# NumValueKinds
+1
+# Value Kind IPVK_IndirectCallTarget
+0
+# NumSites
+3
+# Values for each site
+0
+2
+# !!!! Malformed Value/Count pair
+foo+100
+foo2:1000
+1
+foo2:20000

llvm/trunk/test/tools/llvm-profdata/Inputs/vp-malform2.proftext

+foo
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+999000
+359800
+
+main
+# Func Hash:
+16650
+# Num Counters:
+4
+# Counter Values:
+2
+2000
+2000000
+999000
+# NumValueKinds
+1
+# Value Kind IPVK_IndirectCallTarget
+0
+# NumSites
+3
+# Values for each site
+0
+# !! Malformed value site, missing one value
+2
+foo:100
+1
+foo2:20000

llvm/trunk/test/tools/llvm-profdata/Inputs/vp-truncate.proftext

+foo
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+999000
+359800
+
+foo2
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+1001000
+360200
+
+main
+# Func Hash:
+16650
+# Num Counters:
+4
+# Counter Values:
+2
+2000
+2000000
+999000
+# NumValueKinds
+1
+# Value Kind IPVK_IndirectCallTarget
+0
+# NumSites
+3
+# Values for each site
+0

llvm/trunk/test/tools/llvm-profdata/text-format-errors.test

 3- Detect no counts
 RUN: not llvm-profdata show %p/Inputs/no-counts.proftext 2>&1 | FileCheck %s --check-prefix=NO-COUNTS
 NO-COUNTS: error: {{.*}}no-counts.proftext: Malformed instrumentation profile data
 
 4- Detect binary input
 RUN: not llvm-profdata show %p/Inputs/text-format-errors.text.bin 2>&1 | FileCheck %s --check-prefix=BINARY
 BINARY: error: {{.+}}: Unrecognized instrumentation profile encoding format
 BINARY: Perhaps you forgot to use the -sample option?
+
+5- Detect malformed value profile data
+RUN: not llvm-profdata show %p/Inputs/vp-malform.proftext 2>&1 | FileCheck %s --check-prefix=VP
+RUN: not llvm-profdata show %p/Inputs/vp-malform2.proftext 2>&1 | FileCheck %s --check-prefix=VP
+VP: Malformed instrumentation profile data
+
+6- Detect truncated value profile data
+RUN: not llvm-profdata show %p/Inputs/vp-truncate.proftext 2>&1 | FileCheck %s --check-prefix=VPTRUNC
+VPTRUNC: Truncated profile data

llvm/trunk/test/tools/llvm-profdata/value-prof.proftext

+# RUN: llvm-profdata show -ic-targets  -all-functions %s | FileCheck %s --check-prefix=IC
+# RUN: llvm-profdata show -ic-targets -counts -text -all-functions %s | FileCheck %s --check-prefix=ICTEXT
+# RUN: llvm-profdata merge -o %t.profdata  %s
+# RUN: llvm-profdata show -ic-targets  -all-functions %t.profdata | FileCheck %s --check-prefix=IC
+
+foo
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+999000
+359800
+
+foo2
+# Func Hash:
+10
+# Num Counters:
+2
+# Counter Values:
+1001000
+360200
+
+main
+# Func Hash:
+16650
+# Num Counters:
+4
+# Counter Values:
+2
+2000
+2000000
+999000
+# NumValueKinds
+1
+# Value Kind IPVK_IndirectCallTarget
+0
+# NumSites
+3
+# Values for each site
+0
+2
+foo:100
+foo2:1000
+1
+foo2:20000
+
+#IC: Indirect Call Site Count: 3
+#IC-NEXT:    Indirect Target Results: 
+#IC-NEXT: [ 1, foo, 100 ]
+#IC-NEXT: [ 1, foo2, 1000 ]
+#IC-NEXT: [ 2, foo2, 20000 ]
+
+#ICTEXT: foo:100
+#ICTEXT-NEXT: foo2:1000
+#ICTEXT-NEXT: 1
+#ICTEXT-NEXT: foo2:20000
+# RUN: llvm-profdata show -ic-targets  -all-functions %s | FileCheck %s --check-prefix=IC
+# RUN: llvm-profdata show -ic-targets -counts -text -all-functions %s | FileCheck %s --check-prefix=ICTEXT
+# RUN: llvm-profdata merge -o %t.profdata  %s
+# RUN: llvm-profdata show -ic-targets  -all-functions %t.profdata | FileCheck %s --check-prefix=IC