[nfc][InstrProf]Remove 'offsetOf' when parsing indexed profiles #93346

mingmingl-llvm · 2024-05-24T20:50:10Z

In Header::readFromBuffer, read the buffer in the forward direction by using readNext.
When compute the header size, spell out the constant.

With the changes above, we can remove offsetOf in InstrProf.cpp

Changes include: 1. In InstrProfReader.cpp, compute ProfileVersion once, rather than repeatedly use macro `GET_VERSION(H.Version)`. 2. In InstrProfWriter.cpp, store back-patched header fields in a vector and patch once. 3. In InstrProf.cpp, remove offsetOf and use a helper function read_and_decrement. llvm/Support/Endian.h has `readNext` method which increments the buffer. For `Header::readFromBuffer`, switch statements allow code sharing by fall-through, and it's not very straightforward to use 'readNext'.

…-project/github-pull-requests/builds/67098#018fac5e-811d-46e4-8ba4-62c8b57f28e3

mingmingl-llvm · 2024-05-24T20:55:30Z

I'm planning to resume #88212 after this clean-up.

#88212 will add at least 2 new header fields. This simplification can reduce code complexity there.

kazutakahirata · 2024-05-24T21:28:51Z

Thank you for cleaning up these things! I like where things are going overall. Now, would you mind creating focused patches that do one thing.

I think the GET_VERSION clean up should call a method in Header. If Header->getVersion() is confusing, you might say Header->getIndexedProfileVersion() or something. I wouldn't save the version in ProfileVersion. Instead, I would keep calling Header->getIndexedProfileVersion().
read_and_decrement should be named readAndDecrment or readAndDecrement according to the LLVM Coding Standards. Now, I feel a bit uneasy about going backward. Let's discuss this in a separate patch.

mingmingl-llvm · 2024-05-28T22:38:07Z

Thank you for cleaning up these things! I like where things are going overall. Now, would you mind creating focused patches that do one thing.

Sure. I created #93613 and #93594.

I think the GET_VERSION clean up should call a method in Header. If Header->getVersion() is confusing, you might say Header->getIndexedProfileVersion() or something. I wouldn't save the version in ProfileVersion. Instead, I would keep calling Header->getIndexedProfileVersion().

#93613 does this.

read_and_decrement should be named readAndDecrment or readAndDecrement according to the LLVM Coding Standards. Now, I feel a bit uneasy about going backward. Let's discuss this in a separate patch.

I did hesitate between reading forward and backward when write the partial forward compatibility patch (#88212), and chose to retain backward parsing mainly because existing code prefers switch with fallthroughs.

https://gcc.godbolt.org/z/qdM4naeM1 roughly demonstrated the IR for both switch-with-fallthrough and a couple of if statements. Since this is not performance critical code and the thoughts I hear so far (here, and in a chat room which discusses code patterns/styles) prefer to read forward, I updated the code to use readNext.

…cide profile version. (#93613) This is a split of #93346 as discussed.

…once (#93594) This is a split of #93346 as discussed.

github-actions · 2024-05-29T18:07:28Z

✅ With the latest revision this PR passed the C/C++ code formatter.

snehasish · 2024-05-29T19:52:39Z

llvm/lib/ProfileData/InstrProf.cpp

-// native endianness if necessary.
-static inline uint64_t read(const unsigned char *Buffer, size_t Offset) {
+// native endianness if necessary. Increment the buffer past the read value.
+static inline uint64_t readNext(const unsigned char *&Buffer) {


Do we need this wrapper now? Instead we can just use the readNext call directly. You can use a using directive to reduce the namespace boilerplate if you want.

Removed this wrapper.

snehasish · 2024-05-29T19:54:56Z

llvm/lib/ProfileData/InstrProf.cpp

@@ -1704,19 +1685,17 @@ size_t Header::size() const {
                  "been added to the header, if not add a case statement to "
                  "fall through to the latest version.");


Update the comment and assert message?

done. I also updated the assert message in static_assert(std::is_standard_layout_v<Header>.

llvmbot · 2024-05-29T20:31:49Z

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

In Header::readFromBuffer, read the buffer in the forward direction by using readNext.
When compute the header size, spell out the constant.

With the changes above, we can remove offsetOf in InstrProf.cpp

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

1 Files Affected:

(modified) llvm/lib/ProfileData/InstrProf.cpp (+38-59)

diff --git a/llvm/lib/ProfileData/InstrProf.cpp b/llvm/lib/ProfileData/InstrProf.cpp
index dcf6aac8b5996..8fc5d8e2a0bba 100644
--- a/llvm/lib/ProfileData/InstrProf.cpp
+++ b/llvm/lib/ProfileData/InstrProf.cpp
@@ -1627,66 +1627,46 @@ void OverlapStats::dump(raw_fd_ostream &OS) const {
 }
 
 namespace IndexedInstrProf {
-// A C++14 compatible version of the offsetof macro.
-template <typename T1, typename T2>
-inline size_t constexpr offsetOf(T1 T2::*Member) {
-  constexpr T2 Object{};
-  return size_t(&(Object.*Member)) - size_t(&Object);
-}
-
-// Read a uint64_t from the specified buffer offset, and swap the bytes in
-// native endianness if necessary.
-static inline uint64_t read(const unsigned char *Buffer, size_t Offset) {
-  using namespace ::support;
-  return endian::read<uint64_t, llvm::endianness::little, unaligned>(Buffer +
-                                                                     Offset);
-}
-
 Expected<Header> Header::readFromBuffer(const unsigned char *Buffer) {
   using namespace support;
   static_assert(std::is_standard_layout_v<Header>,
-                "The header should be standard layout type since we use offset "
-                "of fields to read.");
+                "Use standard layout for Header for simplicity");
   Header H;
 
-  H.Magic = read(Buffer, offsetOf(&Header::Magic));
+  H.Magic =
+      endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
   // Check the magic number.
   if (H.Magic != IndexedInstrProf::Magic)
     return make_error<InstrProfError>(instrprof_error::bad_magic);
 
   // Read the version.
-  H.Version = read(Buffer, offsetOf(&Header::Version));
+  H.Version =
+      endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
   if (H.getIndexedProfileVersion() >
       IndexedInstrProf::ProfVersion::CurrentVersion)
     return make_error<InstrProfError>(instrprof_error::unsupported_version);
 
-  switch (H.getIndexedProfileVersion()) {
-    // When a new field is added in the header add a case statement here to
-    // populate it.
-    static_assert(
-        IndexedInstrProf::ProfVersion::CurrentVersion == Version12,
-        "Please update the reading code below if a new field has been added, "
-        "if not add a case statement to fall through to the latest version.");
-  case 12ull:
-    H.VTableNamesOffset = read(Buffer, offsetOf(&Header::VTableNamesOffset));
-    [[fallthrough]];
-  case 11ull:
-    [[fallthrough]];
-  case 10ull:
+  static_assert(IndexedInstrProf::ProfVersion::CurrentVersion == Version12,
+                "Please update the reading as needed when a new field is added "
+                "or when indexed profile version gets bumped.");
+
+  Buffer += sizeof(uint64_t); // Skip Header.Unused field.
+  H.HashType =
+      endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  H.HashOffset =
+      endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  if (H.getIndexedProfileVersion() >= 8)
+    H.MemProfOffset =
+        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  if (H.getIndexedProfileVersion() >= 9)
+    H.BinaryIdOffset =
+        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  if (H.getIndexedProfileVersion() >= 10)
     H.TemporalProfTracesOffset =
-        read(Buffer, offsetOf(&Header::TemporalProfTracesOffset));
-    [[fallthrough]];
-  case 9ull:
-    H.BinaryIdOffset = read(Buffer, offsetOf(&Header::BinaryIdOffset));
-    [[fallthrough]];
-  case 8ull:
-    H.MemProfOffset = read(Buffer, offsetOf(&Header::MemProfOffset));
-    [[fallthrough]];
-  default: // Version7 (when the backwards compatible header was introduced).
-    H.HashType = read(Buffer, offsetOf(&Header::HashType));
-    H.HashOffset = read(Buffer, offsetOf(&Header::HashOffset));
-  }
-
+        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  if (H.getIndexedProfileVersion() >= 12)
+    H.VTableNamesOffset =
+        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
   return H;
 }
 
@@ -1696,27 +1676,26 @@ uint64_t Header::getIndexedProfileVersion() const {
 
 size_t Header::size() const {
   switch (getIndexedProfileVersion()) {
-    // When a new field is added to the header add a case statement here to
-    // compute the size as offset of the new field + size of the new field. This
-    // relies on the field being added to the end of the list.
-    static_assert(IndexedInstrProf::ProfVersion::CurrentVersion == Version12,
-                  "Please update the size computation below if a new field has "
-                  "been added to the header, if not add a case statement to "
-                  "fall through to the latest version.");
+    // To retain backward compatibility, new fields must be appended to the end
+    // of the header, and byte offset of existing fields shouldn't change when
+    // indexed profile version gets incremented.
+    static_assert(
+        IndexedInstrProf::ProfVersion::CurrentVersion == Version12,
+        "Please update the size computation below if a new field has "
+        "been added to the header; for a version bump without new "
+        "fields, add a case statement to fall through to the latest version.");
   case 12ull:
-    return offsetOf(&Header::VTableNamesOffset) +
-           sizeof(Header::VTableNamesOffset);
+    return 72;
   case 11ull:
     [[fallthrough]];
   case 10ull:
-    return offsetOf(&Header::TemporalProfTracesOffset) +
-           sizeof(Header::TemporalProfTracesOffset);
+    return 64;
   case 9ull:
-    return offsetOf(&Header::BinaryIdOffset) + sizeof(Header::BinaryIdOffset);
+    return 56;
   case 8ull:
-    return offsetOf(&Header::MemProfOffset) + sizeof(Header::MemProfOffset);
+    return 48;
   default: // Version7 (when the backwards compatible header was introduced).
-    return offsetOf(&Header::HashOffset) + sizeof(Header::HashOffset);
+    return 40;
   }
 }

snehasish

lgtm, thanks for improving the code here.

snehasish · 2024-05-29T20:38:46Z

llvm/lib/ProfileData/InstrProf.cpp

+  if (H.getIndexedProfileVersion() >= 9)
+    H.BinaryIdOffset =
+        endian::readNext<uint64_t, llvm::endianness::little, unaligned>(Buffer);
+  if (H.getIndexedProfileVersion() >= 10)


So version 11 is also handled in this condition right? Maybe add a comment to document.

kazutakahirata

LGTM modulo minor comments. Thank you for the clean up!

llvm/lib/ProfileData/InstrProf.cpp

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

mingmingl-llvm · 2024-05-30T18:54:49Z

I did a sanity check by invoking llvm-profdata to show and merge profiles, using profiles without this PR. Going to merge.

mingmingl-llvm requested review from kazutakahirata and snehasish May 24, 2024 20:51

mingmingl-llvm marked this pull request as ready for review May 24, 2024 20:51

cast size_t to int to fix compile error in https://buildkite.com/llvm…

bb60ded

…-project/github-pull-requests/builds/67098#018fac5e-811d-46e4-8ba4-62c8b57f28e3

This was referenced May 28, 2024

[nfc][InstrProfWriter]Store header fields in a vector and back patch once #93594

Merged

[nfc][InstrFDO] Add Header::getIndexedProfileVersion and use it to decide profile version. #93613

Merged

Read the buffer in the forward direction and remove 'offset_of'

3852e32

mingmingl-llvm changed the title ~~[nfc][InstrProf]Simplify instrumentation profile reader and writer code~~ [nfc][InstrProf]Remove 'offsetOf' when parsing indexed profiles May 28, 2024

Correct the push in previous commit

987c105

mingmingl-llvm added a commit that referenced this pull request May 29, 2024

[nfc][InstrFDO] Add Header::getIndexedProfileVersion and use it to de…

737a301

…cide profile version. (#93613) This is a split of #93346 as discussed.

mingmingl-llvm added a commit that referenced this pull request May 29, 2024

[nfc][InstrProfWriter]Store header fields in a vector and back patch …

c546578

…once (#93594) This is a split of #93346 as discussed.

mingmingl-llvm added 2 commits May 29, 2024 10:54

run 'git merge main'

9e22aaf

call Header.getIndexedProfileVersion()

737a7a9

clang-format

f40dc7b

snehasish reviewed May 29, 2024

View reviewed changes

resolve review comments

1ada31d

llvmbot added the PGO Profile Guided Optimizations label May 29, 2024

snehasish approved these changes May 29, 2024

View reviewed changes

kazutakahirata approved these changes May 29, 2024

View reviewed changes

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

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

mingmingl-llvm and others added 4 commits May 29, 2024 15:47

apply suggestions

33ee4eb

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

apply suggestion

1f440a1

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

resolve feedback

344bd96

clang-format

ab022e9

mingmingl-llvm merged commit c803c29 into llvm:main May 30, 2024
7 checks passed

mingmingl-llvm deleted the prepare branch May 30, 2024 19:46

mingmingl-llvm mentioned this pull request Jun 5, 2024

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

Open

		@@ -1704,19 +1685,17 @@ size_t Header::size() const {
		"been added to the header, if not add a case statement to "
		"fall through to the latest version.");

[nfc][InstrProf]Remove 'offsetOf' when parsing indexed profiles #93346

[nfc][InstrProf]Remove 'offsetOf' when parsing indexed profiles #93346

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mingmingl-llvm commented May 24, 2024

Uh oh!

kazutakahirata commented May 24, 2024

Uh oh!

mingmingl-llvm commented May 28, 2024

Uh oh!

github-actions bot commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snehasish May 29, 2024

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm May 29, 2024

Choose a reason for hiding this comment

Uh oh!

snehasish May 29, 2024

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm May 29, 2024

Choose a reason for hiding this comment

Uh oh!

llvmbot commented May 29, 2024

Uh oh!

snehasish left a comment

Choose a reason for hiding this comment

Uh oh!

snehasish May 29, 2024

Choose a reason for hiding this comment

Uh oh!

mingmingl-llvm May 30, 2024

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 commented May 30, 2024

Uh oh!

Uh oh!

Uh oh!

mingmingl-llvm commented May 24, 2024 •

edited

Loading

github-actions bot commented May 29, 2024 •

edited

Loading