Skip to content

Conversation

@kazutakahirata
Copy link
Contributor

This patch upgrades a unit test to MemProf Version 3 while removing
those bits that cannot be upgraded to Version 3.

The bits being removed expect instrprof_error::hash_mismatch from a
broken MemProf profile that references a frame that doesn't actually
exist. Now, Version 3 no longer issues
instrprof_error::hash_mismatch. Even if it still issued
instrprof_error::hash_mismatch, we would have a couple of hurdles:

  • InstrProfWriter::addMemProfData will soon require all (or none) of
    the fields (frames, call stacks, and records) be populated. That
    is, it won't accept an instance of IndexedMemProfData with frames
    missing.

  • writeMemProfV3 asserts that every frame occurs at least once:

    assert(MemProfData.Frames.size() == FrameHistogram.size());

This patch gives up on instrprof_error::hash_mismatch and tries to
trigger instrprof_error::unknown_function with the empty profile.

This patch upgrades a unit test to MemProf Version 3 while removing
those bits that cannot be upgraded to Version 3.

The bits being removed expect instrprof_error::hash_mismatch from a
broken MemProf profile that references a frame that doesn't actually
exist.  Now, Version 3 no longer issues
instrprof_error::hash_mismatch.  Even if it still issued
instrprof_error::hash_mismatch, we would have a couple of hurdles:

- InstrProfWriter::addMemProfData will soon require all (or none) of
  the fields (frames, call stacks, and records) be populated.  That
  is, it won't accept an instance of IndexedMemProfData with frames
  missing.

- writeMemProfV3 asserts that every frame occurs at least once:

  assert(MemProfData.Frames.size() == FrameHistogram.size());

This patch gives up on instrprof_error::hash_mismatch and tries to
trigger instrprof_error::unknown_function with the empty profile.
@llvmbot llvmbot added the PGO Profile Guided Optimizations label Nov 20, 2024
@llvmbot
Copy link
Member

llvmbot commented Nov 20, 2024

@llvm/pr-subscribers-pgo

Author: Kazu Hirata (kazutakahirata)

Changes

This patch upgrades a unit test to MemProf Version 3 while removing
those bits that cannot be upgraded to Version 3.

The bits being removed expect instrprof_error::hash_mismatch from a
broken MemProf profile that references a frame that doesn't actually
exist. Now, Version 3 no longer issues
instrprof_error::hash_mismatch. Even if it still issued
instrprof_error::hash_mismatch, we would have a couple of hurdles:

  • InstrProfWriter::addMemProfData will soon require all (or none) of
    the fields (frames, call stacks, and records) be populated. That
    is, it won't accept an instance of IndexedMemProfData with frames
    missing.

  • writeMemProfV3 asserts that every frame occurs at least once:

    assert(MemProfData.Frames.size() == FrameHistogram.size());

This patch gives up on instrprof_error::hash_mismatch and tries to
trigger instrprof_error::unknown_function with the empty profile.


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

1 Files Affected:

  • (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+3-19)
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 3beab8976304ef..8bd39fd71266af 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -640,29 +640,13 @@ TEST_F(InstrProfTest, test_memprof_getrecord_error) {
   ASSERT_THAT_ERROR(Writer.mergeProfileKind(InstrProfKind::MemProf),
                     Succeeded());
 
-  const IndexedMemProfRecord IndexedMR = makeRecord(
-      /*AllocFrames=*/
-      {
-          {0, 1},
-          {2, 3},
-      },
-      /*CallSiteFrames=*/{
-          {4, 5},
-      });
-  // We skip adding the frame mappings here unlike the test_memprof unit test
-  // above to exercise the failure path when getMemProfRecord is invoked.
-  Writer.addMemProfRecord(/*Id=*/0x9999, IndexedMR);
-
+  Writer.setMemProfVersionRequested(memprof::Version3);
+  // Generate an empty profile.
   auto Profile = Writer.writeBuffer();
   readProfile(std::move(Profile));
 
-  // Missing frames give a hash_mismatch error.
-  auto RecordOr = Reader->getMemProfRecord(0x9999);
-  ASSERT_TRUE(
-      ErrorEquals(instrprof_error::hash_mismatch, RecordOr.takeError()));
-
   // Missing functions give a unknown_function error.
-  RecordOr = Reader->getMemProfRecord(0x1111);
+  auto RecordOr = Reader->getMemProfRecord(0x1111);
   ASSERT_TRUE(
       ErrorEquals(instrprof_error::unknown_function, RecordOr.takeError()));
 }

@kazutakahirata kazutakahirata merged commit ec5b729 into llvm:main Nov 20, 2024
7 of 9 checks passed
@kazutakahirata kazutakahirata deleted the memprof_cleanup_makeRecord branch November 20, 2024 22:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PGO Profile Guided Optimizations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants