[nfc][InstrFDO]Encapsulate header writes in a class member function #90142

mingmingl-llvm · 2024-04-25T22:50:38Z

The smaller class member are more focused and easier to maintain. This also paves the way for partial header forward compatibility in #88212

snehasish · 2024-04-29T21:56:01Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+  InfoObj->CSSummaryBuilder = &CSISB;
+
+  // Populate the hash table generator.
+  SmallVector<std::pair<StringRef, const ProfilingData *>, 0> OrderedData;


Do you know why the size hint = 0?

I guess this is old code just being moved in this patch but we can drop the hint since now SmallVector can infer the size hint automatically.

snehasish · 2024-04-29T21:57:16Z

llvm/lib/ProfileData/InstrProfWriter.cpp

  if (!WritePrevVersion)
    OS.write(0);

+  return OS.tell() - StartOffset;


If this returned value isn't being used then maybe update the definition and call site? Also you can just return a HeaderFieldOffsets struct instead of passing it in by reference.

Thanks for the review @snehasish !

Before making changes for the other comments, just a quick question on the return value: the subsequent patch for partial forward compatibility (#88212) needs header byte size (for reader to locate the end of the header fields), and InstrProfWriter::writeHeader is the function that has the header byte size information.

To make the diff smaller for the subsequent patch, I wonder if returning std::pair<uint64_t /* ByteSize*/, HeaderFieldOffsets> sounds good.

Ah makes sense. I would suggest just leave a FIXME comment that the returned value will be used in a future patch at the call site.

done by using a FIXME at the callsite.

Also chose (void) rather than std::ignore after I searched the differences and came across transmission/transmission#3667 (comment)

snehasish · 2024-04-29T21:59:54Z

llvm/include/llvm/ProfileData/InstrProfWriter.h

@@ -197,13 +197,26 @@ class InstrProfWriter {
                     const OverlapFuncFilters &FuncFilter);

 private:
+  struct HeaderFieldOffsets {


Add a comment describing what this struct holds and how the contents are used.

mingmingl-llvm

ptal, thanks!

mingmingl-llvm · 2024-04-30T00:29:09Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+  InfoObj->CSSummaryBuilder = &CSISB;
+
+  // Populate the hash table generator.
+  SmallVector<std::pair<StringRef, const ProfilingData *>, 0> OrderedData;


mingmingl-llvm · 2024-04-30T00:29:36Z

llvm/include/llvm/ProfileData/InstrProfWriter.h

@@ -197,13 +197,26 @@ class InstrProfWriter {
                     const OverlapFuncFilters &FuncFilter);

 private:
+  struct HeaderFieldOffsets {


mingmingl-llvm · 2024-04-30T01:03:47Z

llvm/lib/ProfileData/InstrProfWriter.cpp

  if (!WritePrevVersion)
    OS.write(0);

+  return OS.tell() - StartOffset;


done by using a FIXME at the callsite.

Also chose (void) rather than std::ignore after I searched the differences and came across transmission/transmission#3667 (comment)

kazutakahirata · 2024-05-15T21:41:32Z

Instead of grouping the offsets of header fields into a struct, could we add a couple of member functions to llvm::IndexedInstrProf::Header instead?

struct Header {
  // ... existing stuff ...

  uint64_t HeaderPos;

  // Write out the header, including placeholders for various offsets.
  void write(ProfOStream &OS, bool WritePrevVersion) {
    // Only write out the first four fields.
    for (int I = 0; I < 4; I++)
      OS.write(reinterpret_cast<uint64_t *>(&Header)[I]);

    UpdatePos = OS.tell();
    OS.write(0);  // HashTableStart
    OS.write(0);  // MemProfSectionStart
    OS.write(0);  // BinaryIdSectionStart
    ...
    // Do whatever is appropriate with WritePrevVersion.
  }

  // Update the header.
  void patch(ProfOStream &OS, bool WritePrevVersion) {
    uint64_t Updates[] = {HashTableStart, MemProfSectionStart, BinaryIdSectionStart, ...};
    OS.patch({{UpdatePos, Updates, std::size(Updates)}});
    // Do whatever is appropriate with WritePrevVersion.
  }
};

This way, we can remove variables like HashTableStartFieldOffset, MemProfSectionOffset, and BinaryIdSectionOffset, which carry carry redundant information as a group. We know that MemProfSectionOffset == HashTableStartFieldOffset + 8, BinaryIdSectionOffset == HashTableStartFieldOffset + 8, etc. Also, the details of the header format are encapsulated in the new member functions, away from InstrProfWriter.

mingmingl-llvm · 2024-05-16T21:18:16Z

Instead of grouping the offsets of header fields into a struct, could we add a couple of member functions to llvm::IndexedInstrProf::Header instead?
struct Header {
  // ... existing stuff ...

  uint64_t HeaderPos;

  // Write out the header, including placeholders for various offsets.
  void write(ProfOStream &OS, bool WritePrevVersion) {
    // Only write out the first four fields.
    for (int I = 0; I < 4; I++)
      OS.write(reinterpret_cast<uint64_t *>(&Header)[I]);

    UpdatePos = OS.tell();
    OS.write(0);  // HashTableStart
    OS.write(0);  // MemProfSectionStart
    OS.write(0);  // BinaryIdSectionStart
    ...
    // Do whatever is appropriate with WritePrevVersion.
  }

  // Update the header.
  void patch(ProfOStream &OS, bool WritePrevVersion) {
    uint64_t Updates[] = {HashTableStart, MemProfSectionStart, BinaryIdSectionStart, ...};
    OS.patch({{UpdatePos, Updates, std::size(Updates)}});
    // Do whatever is appropriate with WritePrevVersion.
  }
};
This way, we can remove variables like HashTableStartFieldOffset, MemProfSectionOffset, and BinaryIdSectionOffset, which carry carry redundant information as a group. We know that MemProfSectionOffset == HashTableStartFieldOffset + 8, BinaryIdSectionOffset == HashTableStartFieldOffset + 8, etc. Also, the details of the header format are encapsulated in the new member functions, away from InstrProfWriter.

It makes a lot of sense to use compact information in the *Offset fields. In the same spirit as demonstrated in the comment, writeHeader now records BackPatchStartOffset as an output parameter.

ProfOStream (the output stream class) is defined in InstrProfWriter.h/cpp, and IndexedInstrProf::Header is defined in InstrProf.h. To make writeHeader a method of Header class, ProfOStream needs to be moved to InstrProf.h (or a common dependency of InstrProfWriter or InstrProf). So keep writeHeader a method of InstrProfWriter as discussed offline. PTAL, thanks!

kazutakahirata

Thank you for updating the patch! Please see comments.

kazutakahirata · 2024-05-17T01:17:32Z

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -552,6 +552,41 @@ static Error writeMemProf(
              memprof::MaximumSupportedVersion));
 }

+size_t InstrProfWriter::writeHeader(IndexedInstrProf::Header &Header,


May I suggest const IndexedInstrProf::Header &?

kazutakahirata · 2024-05-17T01:20:10Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+    OS.write(0);
+
+  // Returns the number of bytes written in the header section.
+  return OS.tell() - StartOffset;


May I suggest return BackPatchStartOffset; instead?

The caller can easily compute OS.tell() - StartOffset on its own. Also, the expression is simple enough and self explanatory.

kazutakahirata · 2024-05-17T01:40:26Z

llvm/lib/ProfileData/InstrProfWriter.cpp

  if (!WritePrevVersion) {
    // Now do the final patch:
    PatchItem PatchItems[] = {
        // Patch the Header.HashOffset field.
-        {HashTableStartFieldOffset, &HashTableStart, 1},
+        {BackPatchStartOffset, &HashTableStart, 1},


May I suggest grouping these three variables?

uint64_t HeaderUpdates[] = { HashTableStart, MemProfSectionStart, BinaryIdSectionStart }; PatchItem PatchItems[] = { {BackPatchStartOffset, HeaderUpdate, std::size(HeaderUpdates)}, ...

I am suggesting this because:

The essence of the header format is that these fields are written out one after another. We don't really care about the offsets of these individual fields.

Grouping them reduces the maintenance burden. Adding a new section just involves adding another element to the HeaderUpdates array above.

The HeaderUpdates array concisely describes the header format, at least the portion that needs patching.

Now, if you choose not to group them, I would write:

const size_t MemProfOffset = BackPatchStartOffset + sizeof(uint64_t);

because the size of each header field on disk comes from:

void write(uint64_t V) { LE.write<uint64_t>(V); }

not the type of IndexedInstrProf::Header::MemProfOffset, etc.

I updated the code to use sizeof(uint64_t)

kazutakahirata · 2024-05-17T01:46:43Z

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -788,12 +805,12 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
    // Now do the final patch:
    PatchItem PatchItems[] = {
        // Patch the Header.HashOffset field.
-        {HashTableStartFieldOffset, &HashTableStart, 1},
+        {BackPatchStartOffset, &HashTableStart, 1},


Likewise, I suggest grouping these fields.

I'll leave them as they currently are, to make this patch focused on header writer encapsulation (which paves the way for partial forward compatibility in #88212)

kazutakahirata · 2024-05-17T01:52:54Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+
+  BackPatchStartOffset = OS.tell();
+
+  // Reserve the space for HashOffset field.


nit: Could we shorten comments?

// Reserve space for offsets to be patched later. OS.write(0); // MemProf OS.write(0); // BinaryIdOffset OS.write(0); // Temporal profile if (!WritePrevVersion) OS.write(0); // VTable names

This way, the code looks like a table and avoids repetitive comments.

mingmingl-llvm

thanks! PTAL.

mingmingl-llvm · 2024-05-17T18:42:06Z

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -552,6 +552,41 @@ static Error writeMemProf(
              memprof::MaximumSupportedVersion));
 }

+size_t InstrProfWriter::writeHeader(IndexedInstrProf::Header &Header,


mingmingl-llvm · 2024-05-17T18:46:27Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+
+  BackPatchStartOffset = OS.tell();
+
+  // Reserve the space for HashOffset field.


mingmingl-llvm · 2024-05-17T18:46:36Z

llvm/lib/ProfileData/InstrProfWriter.cpp

+    OS.write(0);
+
+  // Returns the number of bytes written in the header section.
+  return OS.tell() - StartOffset;


mingmingl-llvm · 2024-05-17T18:48:25Z

llvm/lib/ProfileData/InstrProfWriter.cpp

  if (!WritePrevVersion) {
    // Now do the final patch:
    PatchItem PatchItems[] = {
        // Patch the Header.HashOffset field.
-        {HashTableStartFieldOffset, &HashTableStart, 1},
+        {BackPatchStartOffset, &HashTableStart, 1},


I updated the code to use sizeof(uint64_t)

mingmingl-llvm · 2024-05-17T18:49:39Z

llvm/lib/ProfileData/InstrProfWriter.cpp

@@ -788,12 +805,12 @@ Error InstrProfWriter::writeImpl(ProfOStream &OS) {
    // Now do the final patch:
    PatchItem PatchItems[] = {
        // Patch the Header.HashOffset field.
-        {HashTableStartFieldOffset, &HashTableStart, 1},
+        {BackPatchStartOffset, &HashTableStart, 1},


I'll leave them as they currently are, to make this patch focused on header writer encapsulation (which paves the way for partial forward compatibility in #88212)

kazutakahirata

LGTM modulo minor comments about comments.

llvm/include/llvm/ProfileData/InstrProfWriter.h

llvm/lib/ProfileData/InstrProfWriter.cpp

Co-authored-by: Kazu Hirata <[email protected]>

mingmingl-llvm · 2024-05-17T23:26:08Z

thanks for the reviews and suggestions!

I will use 'add suggestions to batch' next time (I forgot this feature is available) :/

…bb9b5575a Local branch amd-gfx 663bb9b Merged main:d34be649af1aa849c21a5a0570617c3a89d5f0b8 into amd-gfx:95f9b6ae8b37 Remote branch main 7b977e0 [nfc][InstrFDO]Encapsulate header writes in a class member function (llvm#90142)

mingmingl-llvm added 2 commits April 25, 2024 15:45

[nfc][InstrFDO]Encapsulate header writes in a class member function

90109db

'void' unused return value. The value will be used in a follow-up patch

3c108ad

mingmingl-llvm requested a review from snehasish April 25, 2024 22:50

mingmingl-llvm marked this pull request as ready for review April 25, 2024 22:50

mingmingl-llvm mentioned this pull request Apr 25, 2024

[InstrFDO]Allow indexed profile reader to parse compatible future versions and returns errors for incompatible ones. #88212

Open

snehasish reviewed Apr 29, 2024

View reviewed changes

resolve review feedback

db86e88

mingmingl-llvm commented Apr 30, 2024

View reviewed changes

mingmingl-llvm requested a review from snehasish April 30, 2024 01:08

mingmingl-llvm requested a review from kazutakahirata May 14, 2024 22:05

mingmingl-llvm marked this pull request as draft May 16, 2024 20:54

mingmingl-llvm added 2 commits May 16, 2024 14:11

resolve review feedback

8b6256d

clean up

22d066a

mingmingl-llvm marked this pull request as ready for review May 16, 2024 21:18

kazutakahirata reviewed May 17, 2024

View reviewed changes

resolve review feedback

379a032

mingmingl-llvm commented May 17, 2024

View reviewed changes

mingmingl-llvm requested a review from kazutakahirata May 17, 2024 18:52

kazutakahirata approved these changes May 17, 2024

View reviewed changes

llvm/include/llvm/ProfileData/InstrProfWriter.h Outdated Show resolved Hide resolved

llvm/lib/ProfileData/InstrProfWriter.cpp Outdated Show resolved Hide resolved

llvm/lib/ProfileData/InstrProfWriter.cpp Outdated Show resolved Hide resolved

mingmingl-llvm and others added 3 commits May 17, 2024 16:24

Update llvm/include/llvm/ProfileData/InstrProfWriter.h

464a717

Co-authored-by: Kazu Hirata <[email protected]>

Update llvm/lib/ProfileData/InstrProfWriter.cpp

b15bcc2

Co-authored-by: Kazu Hirata <[email protected]>

Update llvm/lib/ProfileData/InstrProfWriter.cpp

1faf269

Co-authored-by: Kazu Hirata <[email protected]>

mingmingl-llvm added 2 commits May 17, 2024 16:32

update comment

a855e0a

remove unused field

7b82574

mingmingl-llvm merged commit 7b977e0 into llvm:main May 19, 2024
3 of 4 checks passed

mingmingl-llvm deleted the backpatching branch May 19, 2024 04:51

PeddleSpam mentioned this pull request May 20, 2024

[AMDGPU] Fix error in #88512. #92770

Merged


		BackPatchStartOffset = OS.tell();

		// Reserve the space for HashOffset field.

[nfc][InstrFDO]Encapsulate header writes in a class member function #90142

[nfc][InstrFDO]Encapsulate header writes in a class member function #90142

Uh oh!

Conversation

mingmingl-llvm commented Apr 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazutakahirata commented May 15, 2024

Uh oh!

mingmingl-llvm commented May 16, 2024

Uh oh!

kazutakahirata left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazutakahirata left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mingmingl-llvm Apr 30, 2024 •

edited

Loading

mingmingl-llvm Apr 30, 2024 •

edited

Loading