[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

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

127

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

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

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

43

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

48

Ditto for CHECK lines.

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

5

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

Same note about the RUN lines.

[davidxl]

lib/ProfileData/InstrProfReader.cpp

107

This was fixed already.

127

good suggestion.

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

43

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

173

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

174

These macro function arguments should be wrapped in parens.

[davidxl]

Done.

thanks,

David

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.

lib/ProfileData/InstrProfReader.cpp

if (Reader->readNextRecord(Record))
     *this = InstrProfIterator();
 }
 
 bool TextInstrProfReader::hasFormat(const MemoryBuffer &Buffer) {
   // Verify that this really looks like plain ASCII text by checking a
   // '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,
+  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) {
+
+  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++) {
+    if (Line.is_at_end())
+      return error(instrprof_error::truncated);
+    uint32_t ValueKind;
+    if ((Line++)->getAsInteger(10, ValueKind))
+      return error(instrprof_error::malformed);
+    if (ValueKind > IPVK_Last)
+      return error(instrprof_error::malformed);
+    if (Line.is_at_end())
+      return error(instrprof_error::truncated);
+    uint32_t NumValueSites;
+    if ((Line++)->getAsInteger(10, NumValueSites))
+      return error(instrprof_error::malformed);
+    if (!NumValueSites)
+      continue;
+    Record.reserveSites(VK, NumValueSites);
+    for (uint32_t S = 0; S < NumValueSites; S++) {
+      if (Line.is_at_end())
+        return error(instrprof_error::truncated);
+      uint32_t NumValueData;
+      if ((Line++)->getAsInteger(10, NumValueData))
+        return error(instrprof_error::malformed);
+      std::vector<InstrProfValueData> CurrentValues;
+      for (uint32_t V = 0; V < NumValueData; V++) {
+        if (Line.is_at_end())
+          return error(instrprof_error::truncated);
+        std::pair<StringRef, StringRef> VD = Line->split(':');
+        uint64_t TakenCount;
+        uint64_t Value;
+        if (VD.second.getAsInteger(10, TakenCount))
+          return error(instrprof_error::malformed);
+        if (VK == IPVK_IndirectCallTarget)
+          Value = (uint64_t)StringTable.insertString(VD.first);
+        else {
+          if (VD.first.getAsInteger(10, Value))
+            return error(instrprof_error::malformed);
+        }
+        CurrentValues.push_back({Value, TakenCount});
+        Line++;
+      }
+      Record.addValueData(VK, S, CurrentValues.data(), NumValueData, nullptr);
+    }
+  }
+  return success();
+}
+
 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);
 
   // Read the function name.
   Record.Name = *Line++;
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);
   }
 
+  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 =

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);
 }

test/tools/llvm-profdata/Inputs/vp-malform.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

test/tools/llvm-profdata/Inputs/vp-malform2.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
+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

test/tools/llvm-profdata/Inputs/vp-truncate.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

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

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