[nfc][InstrProfTest]Parameterize the edge cases of value profile merge by value kind

[mingmingl-llvm]

There are three test cases to test the merge of value profiles. 'get_icall_data_merge1' tests the basic case; {get_icall_data_merge1_saturation, get_icall_data_merge_site_trunc} tests the edge case.

This patch parameterizes the edge case test coverage by value kind and adds the coverage of 'IPVK_MemOPSize'. Keep the basic test structure as it is. The main reason is test data construction and test assertions is clearer for each kind in the basic test.

• Using a loop for different value kinds in one test case doesn't work very well. The instr-prof-writer is stateful (e.g., keeps track of per-function profile data in a container)

[llvmbot]

@llvm/pr-subscribers-pgo

Author: Mingming Liu (minglotus-6)

Changes

There are three test cases to test the merge of value profiles. 'get_icall_data_merge1' tests the basic case; {get_icall_data_merge1_saturation, get_icall_data_merge_site_trunc} tests the edge case.

This patch parameterizes the edge case test coverage by value kind and adds the coverage of 'IPVK_MemOPSize'. Keep the basic test structure as it is. The main reason is test data construction and test assertions is clearer for each kind in the basic test.

---

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

1 Files Affected:

• (modified) llvm/unittests/ProfileData/InstrProfTest.cpp (+47-31)

diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index 875e2d06d839367..e6613a90dc7c53e 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -815,7 +815,7 @@ TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
   ASSERT_EQ(1U, ValueData[3].Count);
 }
 
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) {
+TEST_P(MaybeSparseInstrProfTest, icall_data_merge) {
   static const char caller[] = "caller";
   NamedInstrProfRecord Record11(caller, 0x1234, {1, 2});
   NamedInstrProfRecord Record12(caller, 0x1234, {1, 2});
@@ -920,8 +920,18 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) {
   ASSERT_EQ(2U, VD_4[2].Count);
 }
 
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
+struct ValueProfileMergeEdgeCaseTest
+    : public InstrProfTest,
+      public ::testing::WithParamInterface<std::tuple<bool, uint32_t>> {
+  void SetUp() override { Writer.setOutputSparse(std::get<0>(GetParam())); }
+
+  uint32_t getValueProfileKind() const { return std::get<1>(GetParam()); }
+};
+
+TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_saturation) {
+  const uint32_t ValueKind = getValueProfileKind();
   static const char bar[] = "bar";
+  const uint64_t ProfiledValue = 0x5678;
 
   const uint64_t MaxValCount = std::numeric_limits<uint64_t>::max();
   const uint64_t MaxEdgeCount = getInstrMaxCountValue();
@@ -944,18 +954,18 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
   ASSERT_EQ(Result, instrprof_error::success);
 
   NamedInstrProfRecord Record4("baz", 0x5678, {3, 4});
-  Record4.reserveSites(IPVK_IndirectCallTarget, 1);
-  InstrProfValueData VD4[] = {{uint64_t(bar), 1}};
-  Record4.addValueData(IPVK_IndirectCallTarget, 0, VD4, 1, nullptr);
+  Record4.reserveSites(ValueKind, 1);
+  InstrProfValueData VD4[] = {{ProfiledValue, 1}};
+  Record4.addValueData(ValueKind, 0, VD4, 1, nullptr);
   Result = instrprof_error::success;
   Writer.addRecord(std::move(Record4), Err);
   ASSERT_EQ(Result, instrprof_error::success);
 
   // Verify value data counter overflow.
   NamedInstrProfRecord Record5("baz", 0x5678, {5, 6});
-  Record5.reserveSites(IPVK_IndirectCallTarget, 1);
-  InstrProfValueData VD5[] = {{uint64_t(bar), MaxValCount}};
-  Record5.addValueData(IPVK_IndirectCallTarget, 0, VD5, 1, nullptr);
+  Record5.reserveSites(ValueKind, 1);
+  InstrProfValueData VD5[] = {{ProfiledValue, MaxValCount}};
+  Record5.addValueData(ValueKind, 0, VD5, 1, nullptr);
   Result = instrprof_error::success;
   Writer.addRecord(std::move(Record5), Err);
   ASSERT_EQ(Result, instrprof_error::counter_overflow);
@@ -966,48 +976,48 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
   // Verify saturation of counts.
   Expected<InstrProfRecord> ReadRecord1 =
       Reader->getInstrProfRecord("foo", 0x1234);
-  EXPECT_THAT_ERROR(ReadRecord1.takeError(), Succeeded());
-  ASSERT_EQ(MaxEdgeCount, ReadRecord1->Counts[0]);
+  ASSERT_THAT_ERROR(ReadRecord1.takeError(), Succeeded());
+  EXPECT_EQ(MaxEdgeCount, ReadRecord1->Counts[0]);
 
   Expected<InstrProfRecord> ReadRecord2 =
       Reader->getInstrProfRecord("baz", 0x5678);
   ASSERT_TRUE(bool(ReadRecord2));
-  ASSERT_EQ(1U, ReadRecord2->getNumValueSites(IPVK_IndirectCallTarget));
+  ASSERT_EQ(1U, ReadRecord2->getNumValueSites(ValueKind));
   std::unique_ptr<InstrProfValueData[]> VD =
-      ReadRecord2->getValueForSite(IPVK_IndirectCallTarget, 0);
-  ASSERT_EQ(StringRef("bar"), StringRef((const char *)VD[0].Value, 3));
-  ASSERT_EQ(MaxValCount, VD[0].Count);
+      ReadRecord2->getValueForSite(ValueKind, 0);
+  EXPECT_EQ(ProfiledValue, VD[0].Value);
+  EXPECT_EQ(MaxValCount, VD[0].Count);
 }
 
-// This test tests that when there are too many values
-// for a given site, the merged results are properly
-// truncated.
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge_site_trunc) {
+// This test tests that when there are too many values for a given site, the
+// merged results are properly truncated.
+TEST_P(ValueProfileMergeEdgeCaseTest, value_profile_data_merge_site_trunc) {
+  const uint32_t ValueKind = getValueProfileKind();
   static const char caller[] = "caller";
 
   NamedInstrProfRecord Record11(caller, 0x1234, {1, 2});
   NamedInstrProfRecord Record12(caller, 0x1234, {1, 2});
 
   // 2 value sites.
-  Record11.reserveSites(IPVK_IndirectCallTarget, 2);
+  Record11.reserveSites(ValueKind, 2);
   InstrProfValueData VD0[255];
   for (int I = 0; I < 255; I++) {
     VD0[I].Value = 2 * I;
     VD0[I].Count = 2 * I + 1000;
   }
 
-  Record11.addValueData(IPVK_IndirectCallTarget, 0, VD0, 255, nullptr);
-  Record11.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
+  Record11.addValueData(ValueKind, 0, VD0, 255, nullptr);
+  Record11.addValueData(ValueKind, 1, nullptr, 0, nullptr);
 
-  Record12.reserveSites(IPVK_IndirectCallTarget, 2);
+  Record12.reserveSites(ValueKind, 2);
   InstrProfValueData VD1[255];
   for (int I = 0; I < 255; I++) {
     VD1[I].Value = 2 * I + 1;
     VD1[I].Count = 2 * I + 1001;
   }
 
-  Record12.addValueData(IPVK_IndirectCallTarget, 0, VD1, 255, nullptr);
-  Record12.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
+  Record12.addValueData(ValueKind, 0, VD1, 255, nullptr);
+  Record12.addValueData(ValueKind, 1, nullptr, 0, nullptr);
 
   Writer.addRecord(std::move(Record11), Err);
   // Merge profile data.
@@ -1017,17 +1027,23 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge_site_trunc) {
   readProfile(std::move(Profile));
 
   Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
-  EXPECT_THAT_ERROR(R.takeError(), Succeeded());
-  std::unique_ptr<InstrProfValueData[]> VD(
-      R->getValueForSite(IPVK_IndirectCallTarget, 0));
-  ASSERT_EQ(2U, R->getNumValueSites(IPVK_IndirectCallTarget));
-  ASSERT_EQ(255U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
+  ASSERT_THAT_ERROR(R.takeError(), Succeeded());
+  std::unique_ptr<InstrProfValueData[]> VD(R->getValueForSite(ValueKind, 0));
+  ASSERT_EQ(2U, R->getNumValueSites(ValueKind));
+  EXPECT_EQ(255U, R->getNumValueDataForSite(ValueKind, 0));
   for (unsigned I = 0; I < 255; I++) {
-    ASSERT_EQ(VD[I].Value, 509 - I);
-    ASSERT_EQ(VD[I].Count, 1509 - I);
+    EXPECT_EQ(VD[I].Value, 509 - I);
+    EXPECT_EQ(VD[I].Count, 1509 - I);
   }
 }
 
+INSTANTIATE_TEST_SUITE_P(
+    EdgeCaseTest, ValueProfileMergeEdgeCaseTest,
+    ::testing::Combine(::testing::Bool(), /* Sparse */
+                       ::testing::Values(IPVK_IndirectCallTarget,
+                                         IPVK_MemOPSize) /* ValueKind */
+                       ));
+
 static void addValueProfData(InstrProfRecord &Record) {
   Record.reserveSites(IPVK_IndirectCallTarget, 5);
   InstrProfValueData VD0[] = {{uint64_t(callee1), 400},

[snehasish]

lgtm