[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–109

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.

207

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

6

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

44

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

49

Ditto for CHECK lines.

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

6

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

6

Same note about the RUN lines.

[davidxl]

lib/ProfileData/InstrProfReader.cpp

107–109

This was fixed already.

127

good suggestion.

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

44

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

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

113

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,
                      [](char c) { return ::isprint(c) || ::isspace(c); });
 }
 
+#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++;
+
+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++) {
+    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();
+}
+
 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 =

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

+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

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

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

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
+# 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