[PGO] Add support for writing previous indexed format

[teresajohnson]

Enable temporary support to ease use of new llvm-profdata with slightly
older indexed profiles after 16e74fd,
which bumped the indexed format for type profiling.

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Teresa Johnson (teresajohnson)

Changes

Enable temporary support to ease use of new llvm-profdata with slightly
older indexed profiles after 16e74fd,
which bumped the indexed format for type profiling.

---

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

4 Files Affected:

• (modified) llvm/include/llvm/ProfileData/InstrProfWriter.h (+9-1)
• (modified) llvm/lib/ProfileData/InstrProfWriter.cpp (+68-39)
• (added) llvm/test/tools/llvm-profdata/profile-version.test (+9)
• (modified) llvm/tools/llvm-profdata/llvm-profdata.cpp (+11-2)

diff --git a/llvm/include/llvm/ProfileData/InstrProfWriter.h b/llvm/include/llvm/ProfileData/InstrProfWriter.h
index 7a806fd7fcf345..baa8b3aa1d3d1f 100644
--- a/llvm/include/llvm/ProfileData/InstrProfWriter.h
+++ b/llvm/include/llvm/ProfileData/InstrProfWriter.h
@@ -68,10 +68,18 @@ class InstrProfWriter {
   // Use raw pointer here for the incomplete type object.
   InstrProfRecordWriterTrait *InfoObj;
 
+  // Temporary support for writing the previous version of the format, to enable
+  // some forward compaitibility. Currently this suppresses the writing of the
+  // new vtable names section and header fields.
+  // TODO: Consider enabling this with future version changes as well, to ease
+  // deployment of newer versions of llvm-profdata.
+  bool WritePrevVersion = false;
+
 public:
   InstrProfWriter(bool Sparse = false,
                   uint64_t TemporalProfTraceReservoirSize = 0,
-                  uint64_t MaxTemporalProfTraceLength = 0);
+                  uint64_t MaxTemporalProfTraceLength = 0,
+                  bool WritePrevVersion = true);
   ~InstrProfWriter();
 
   StringMap<ProfilingData> &getProfileData() { return FunctionData; }
diff --git a/llvm/lib/ProfileData/InstrProfWriter.cpp b/llvm/lib/ProfileData/InstrProfWriter.cpp
index 3e0a0e0d70116d..9eeb00d99f5dde 100644
--- a/llvm/lib/ProfileData/InstrProfWriter.cpp
+++ b/llvm/lib/ProfileData/InstrProfWriter.cpp
@@ -181,10 +181,12 @@ class InstrProfRecordWriterTrait {
 
 InstrProfWriter::InstrProfWriter(bool Sparse,
                                  uint64_t TemporalProfTraceReservoirSize,
-                                 uint64_t MaxTemporalProfTraceLength)
+                                 uint64_t MaxTemporalProfTraceLength,
+                                 bool WritePrevVersion)
     : Sparse(Sparse), MaxTemporalProfTraceLength(MaxTemporalProfTraceLength),
       TemporalProfTraceReservoirSize(TemporalProfTraceReservoirSize),
-      InfoObj(new InstrProfRecordWriterTrait()) {}
+      InfoObj(new InstrProfRecordWriterTrait()),
+      WritePrevVersion(WritePrevVersion) {}
 
 InstrProfWriter::~InstrProfWriter() { delete InfoObj; }
 
@@ -433,6 +435,8 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   IndexedInstrProf::Header Header;
   Header.Magic = IndexedInstrProf::Magic;
   Header.Version = IndexedInstrProf::ProfVersion::CurrentVersion;
+  if (WritePrevVersion)
+    Header.Version--;
   if (static_cast<bool>(ProfileKind & InstrProfKind::IRInstrumentation))
     Header.Version |= VARIANT_MASK_IR_PROF;
   if (static_cast<bool>(ProfileKind & InstrProfKind::ContextSensitive))
@@ -484,7 +488,8 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   OS.write(0);
 
   uint64_t VTableNamesOffset = OS.tell();
-  OS.write(0);
+  if (!WritePrevVersion)
+    OS.write(0);
 
   // Reserve space to write profile summary data.
   uint32_t NumEntries = ProfileSummaryBuilder::DefaultCutoffs.size();
@@ -608,27 +613,29 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
 
   uint64_t VTableNamesSectionStart = OS.tell();
 
-  // Use a dummy (and uncompressed) string as compressed vtable names and get
-  // the necessary profile format change in place for version 12.
-  // TODO: Store the list of vtable names in InstrProfWriter and use the
-  // real compressed name.
-  std::string CompressedVTableNames = "VTableNames";
+  if (!WritePrevVersion) {
+    // Use a dummy (and uncompressed) string as compressed vtable names and get
+    // the necessary profile format change in place for version 12.
+    // TODO: Store the list of vtable names in InstrProfWriter and use the
+    // real compressed name.
+    std::string CompressedVTableNames = "VTableNames";
 
-  uint64_t CompressedStringLen = CompressedVTableNames.length();
+    uint64_t CompressedStringLen = CompressedVTableNames.length();
 
-  // Record the length of compressed string.
-  OS.write(CompressedStringLen);
+    // Record the length of compressed string.
+    OS.write(CompressedStringLen);
 
-  // Write the chars in compressed strings.
-  for (auto &c : CompressedVTableNames)
-    OS.writeByte(static_cast<uint8_t>(c));
+    // Write the chars in compressed strings.
+    for (auto &c : CompressedVTableNames)
+      OS.writeByte(static_cast<uint8_t>(c));
 
-  // Pad up to a multiple of 8.
-  // InstrProfReader would read bytes according to 'CompressedStringLen'.
-  uint64_t PaddedLength = alignTo(CompressedStringLen, 8);
+    // Pad up to a multiple of 8.
+    // InstrProfReader would read bytes according to 'CompressedStringLen'.
+    uint64_t PaddedLength = alignTo(CompressedStringLen, 8);
 
-  for (uint64_t K = CompressedStringLen; K < PaddedLength; K++) {
-    OS.writeByte(0);
+    for (uint64_t K = CompressedStringLen; K < PaddedLength; K++) {
+      OS.writeByte(0);
+    }
   }
 
   uint64_t TemporalProfTracesSectionStart = 0;
@@ -662,26 +669,48 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
   }
   InfoObj->CSSummaryBuilder = nullptr;
 
-  // Now do the final patch:
-  PatchItem PatchItems[] = {
-      // Patch the Header.HashOffset field.
-      {HashTableStartFieldOffset, &HashTableStart, 1},
-      // Patch the Header.MemProfOffset (=0 for profiles without MemProf
-      // data).
-      {MemProfSectionOffset, &MemProfSectionStart, 1},
-      // Patch the Header.BinaryIdSectionOffset.
-      {BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
-      // Patch the Header.TemporalProfTracesOffset (=0 for profiles without
-      // traces).
-      {TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
-      {VTableNamesOffset, &VTableNamesSectionStart, 1},
-      // Patch the summary data.
-      {SummaryOffset, reinterpret_cast<uint64_t *>(TheSummary.get()),
-       (int)(SummarySize / sizeof(uint64_t))},
-      {CSSummaryOffset, reinterpret_cast<uint64_t *>(TheCSSummary.get()),
-       (int)CSSummarySize}};
-
-  OS.patch(PatchItems, std::size(PatchItems));
+  if (!WritePrevVersion) {
+    // Now do the final patch:
+    PatchItem PatchItems[] = {
+        // Patch the Header.HashOffset field.
+        {HashTableStartFieldOffset, &HashTableStart, 1},
+        // Patch the Header.MemProfOffset (=0 for profiles without MemProf
+        // data).
+        {MemProfSectionOffset, &MemProfSectionStart, 1},
+        // Patch the Header.BinaryIdSectionOffset.
+        {BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
+        // Patch the Header.TemporalProfTracesOffset (=0 for profiles without
+        // traces).
+        {TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
+        {VTableNamesOffset, &VTableNamesSectionStart, 1},
+        // Patch the summary data.
+        {SummaryOffset, reinterpret_cast<uint64_t *>(TheSummary.get()),
+         (int)(SummarySize / sizeof(uint64_t))},
+        {CSSummaryOffset, reinterpret_cast<uint64_t *>(TheCSSummary.get()),
+         (int)CSSummarySize}};
+
+    OS.patch(PatchItems, std::size(PatchItems));
+  } else {
+    // Now do the final patch:
+    PatchItem PatchItems[] = {
+        // Patch the Header.HashOffset field.
+        {HashTableStartFieldOffset, &HashTableStart, 1},
+        // Patch the Header.MemProfOffset (=0 for profiles without MemProf
+        // data).
+        {MemProfSectionOffset, &MemProfSectionStart, 1},
+        // Patch the Header.BinaryIdSectionOffset.
+        {BinaryIdSectionOffset, &BinaryIdSectionStart, 1},
+        // Patch the Header.TemporalProfTracesOffset (=0 for profiles without
+        // traces).
+        {TemporalProfTracesOffset, &TemporalProfTracesSectionStart, 1},
+        // Patch the summary data.
+        {SummaryOffset, reinterpret_cast<uint64_t *>(TheSummary.get()),
+         (int)(SummarySize / sizeof(uint64_t))},
+        {CSSummaryOffset, reinterpret_cast<uint64_t *>(TheCSSummary.get()),
+         (int)CSSummarySize}};
+
+    OS.patch(PatchItems, std::size(PatchItems));
+  }
 
   for (const auto &I : FunctionData)
     for (const auto &F : I.getValue())
diff --git a/llvm/test/tools/llvm-profdata/profile-version.test b/llvm/test/tools/llvm-profdata/profile-version.test
new file mode 100644
index 00000000000000..cb68a648d5e5aa
--- /dev/null
+++ b/llvm/test/tools/llvm-profdata/profile-version.test
@@ -0,0 +1,9 @@
+Test the profile version.
+
+RUN: llvm-profdata merge -o %t.profdata %p/Inputs/basic.proftext
+RUN: llvm-profdata show --profile-version %t.profdata | FileCheck %s
+CHECK: Profile version: 12
+
+RUN: llvm-profdata merge -o %t.prev.profdata %p/Inputs/basic.proftext --write-prev-version
+RUN: llvm-profdata show --profile-version %t.prev.profdata | FileCheck %s --check-prefix=PREV
+PREV: Profile version: 11
diff --git a/llvm/tools/llvm-profdata/llvm-profdata.cpp b/llvm/tools/llvm-profdata/llvm-profdata.cpp
index 8400b0769944cf..327c6cf66541db 100644
--- a/llvm/tools/llvm-profdata/llvm-profdata.cpp
+++ b/llvm/tools/llvm-profdata/llvm-profdata.cpp
@@ -291,6 +291,15 @@ cl::opt<bool> DropProfileSymbolList(
     cl::desc("Drop the profile symbol list when merging AutoFDO profiles "
              "(only meaningful for -sample)"));
 
+// Temporary support for writing the previous version of the format, to enable
+// some forward compaitibility.
+// TODO: Consider enabling this with future version changes as well, to ease
+// deployment of newer versions of llvm-profdata.
+cl::opt<bool> DoWritePrevVersion(
+    "write-prev-version", cl::init(false), cl::Hidden,
+    cl::desc("Write the previous version of indexed format, to enable "
+             "some forward compatibility."));
+
 // Options specific to overlap subcommand.
 cl::opt<std::string> BaseFilename(cl::Positional, cl::Required,
                                   cl::desc("<base profile file>"),
@@ -579,8 +588,8 @@ struct WriterContext {
   WriterContext(bool IsSparse, std::mutex &ErrLock,
                 SmallSet<instrprof_error, 4> &WriterErrorCodes,
                 uint64_t ReservoirSize = 0, uint64_t MaxTraceLength = 0)
-      : Writer(IsSparse, ReservoirSize, MaxTraceLength), ErrLock(ErrLock),
-        WriterErrorCodes(WriterErrorCodes) {}
+      : Writer(IsSparse, ReservoirSize, MaxTraceLength, DoWritePrevVersion),
+        ErrLock(ErrLock), WriterErrorCodes(WriterErrorCodes) {}
 };
 
 /// Computer the overlap b/w profile BaseFilename and TestFileName,

[snehasish]

lgtm

[snehasish]

typo: compatibility

[teresajohnson]

done

[snehasish]

Was your intention to have this default to true or false? I'm ok with true since vtable names isn't fully implemented yet - in that case just make it true on L76 and drop this default in the constructor.

[teresajohnson]

It was meant to be false - I had it set to true for testing and nothing failed...switched it to false.

[snehasish]

Since the changes down below are VTableName change specific would it make more sense to explicitly set this to the version we want?

[teresajohnson]

done. added an assert on the current version so we make sure this handling is updated if needed.

[snehasish]

typo: compatibility

[teresajohnson]

done