[nfc][InstrProfTest]Factor out common code for value profile test #72611
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ay VDArray could be vector<MutabeArrayRef<T>> rather than vector<vector<T>>
|
@llvm/pr-subscribers-pgo Author: Mingming Liu (minglotus-6) Changes
Patch is 30.43 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/72611.diff 1 Files Affected:
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index f26f244afc5378e..3e67e55bea73f2b 100644
--- a/llvm/unittests/ProfileData/InstrProfTest.cpp
+++ b/llvm/unittests/ProfileData/InstrProfTest.cpp
@@ -6,6 +6,7 @@
//
//===----------------------------------------------------------------------===//
+#include "llvm/ADT/bit.h"
#include "llvm/IR/Function.h"
#include "llvm/IR/IRBuilder.h"
#include "llvm/IR/LLVMContext.h"
@@ -64,6 +65,37 @@ struct InstrProfTest : ::testing::Test {
}
};
+static const char callee1[] = "callee1";
+static const char callee2[] = "callee2";
+static const char callee3[] = "callee3";
+static const char callee4[] = "callee4";
+static const char callee5[] = "callee5";
+static const char callee6[] = "callee6";
+
+static const auto Err = [](Error E) {
+ consumeError(std::move(E));
+ FAIL();
+};
+
+typedef std::vector<MutableArrayRef<InstrProfValueData>> VDArray;
+
+// 'ValueDataArray' should be a non-const reference, and the array element is
+// mutable array reference of InstrProfRecord. This is mainly because method
+// `InstrProfRecord::addValueData` might modify the underlying C array.
+static void addValueProfData(InstrProfRecord &Record, uint32_t ValueKind,
+ VDArray &ValueDataArray) {
+ Record.reserveSites(ValueKind, ValueDataArray.size());
+ for (long unsigned int i = 0; i < ValueDataArray.size(); i++) {
+ // The state of vector::data() is not specified when the vector is empty,
+ // and MutableArrayRef takes vector::data() when initialized with a vector.
+ // This should probably be fixed in MutableArrayRef library.
+ Record.addValueData(ValueKind, i,
+ ValueDataArray[i].empty() ? nullptr
+ : ValueDataArray[i].data(),
+ ValueDataArray[i].size(), nullptr);
+ }
+}
+
struct SparseInstrProfTest : public InstrProfTest {
void SetUp() override { Writer.setOutputSparse(true); }
};
@@ -71,6 +103,101 @@ struct SparseInstrProfTest : public InstrProfTest {
struct MaybeSparseInstrProfTest : public InstrProfTest,
public ::testing::WithParamInterface<bool> {
void SetUp() override { Writer.setOutputSparse(GetParam()); }
+
+ void TearDown() override {
+ // Restore little endianness after each test case.
+ Writer.setValueProfDataEndianness(llvm::endianness::little);
+ if (Reader)
+ Reader->setValueProfDataEndianness(llvm::endianness::little);
+ }
+
+public:
+ // A helper function to test the counter-overflow when merging value profiles.
+ void testValueProfileMergeSaturation(uint32_t ValueKind);
+ // A helper function to test that when there are too many values for a given
+ // site, the merged results are properly truncated.
+ void testValueProfileMergeTrunc(uint32_t ValueKind);
+
+ // Tests that value profiles in Record has the same content as (possibly
+ // weighted and sorted) InputVDs for each value kind. ValueProfSorted is true
+ // iff the value profiles of Record are already sorted by count.
+ // InputVDs is a non-const reference since it might need a in-place sort.
+ void testValueDataArray(
+ std::vector<std::pair<uint32_t /* ValueKind */, VDArray &>> &InputVDs,
+ InstrProfRecord &Record, bool ValueProfSorted = false,
+ uint64_t ProfWeight = 1) {
+ for (auto &[ValueKind, InputVD] : InputVDs) {
+ // Tests that the numbers of instrumented value sites are the same.
+ EXPECT_EQ(InputVD.size(), Record.getNumValueSites(ValueKind));
+ for (unsigned i = 0; i < InputVD.size(); i++) {
+ ASSERT_EQ(InputVD[i].size(),
+ Record.getNumValueDataForSite(ValueKind, i));
+
+ uint64_t WantTotalC = 0, GetTotalC = 0;
+ std::unique_ptr<InstrProfValueData[]> OutputVD =
+ Record.getValueForSite(ValueKind, i, &GetTotalC);
+
+ // If value profile elements of the same instrumented site are sorted by
+ // count in Record, sort the input value data array the same way for
+ // comparison.
+ if (ValueProfSorted) {
+ llvm::stable_sort(InputVD[i], [](const InstrProfValueData &lhs,
+ const InstrProfValueData &rhs) {
+ return lhs.Count > rhs.Count;
+ });
+ }
+
+ // The previous ASSERT_EQ already tests the number of value data is
+ // InputVD[i].size(), so there won't be out-of-bound access.
+ for (unsigned j = 0; j < InputVD[i].size(); j++) {
+ EXPECT_EQ(InputVD[i][j].Count * ProfWeight, OutputVD[j].Count);
+ EXPECT_EQ(InputVD[i][j].Value, OutputVD[j].Value);
+ WantTotalC += InputVD[i][j].Count;
+ }
+ EXPECT_EQ(WantTotalC * ProfWeight, GetTotalC);
+ }
+ }
+ }
+
+ void testICallDataReadWrite(
+ uint64_t ProfWeight,
+ llvm::endianness Endianness = llvm::endianness::little) {
+ NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
+
+ // 4 function value sites.
+ std::vector<InstrProfValueData> FuncVD0 = {
+ {uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
+ std::vector<InstrProfValueData> FuncVD2 = {{uint64_t(callee1), 1},
+ {uint64_t(callee2), 2}};
+ std::vector<InstrProfValueData> FuncVD3 = {{uint64_t(callee1), 1}};
+ VDArray FuncVD = {FuncVD0, {}, FuncVD2, FuncVD3};
+ addValueProfData(Record1, IPVK_IndirectCallTarget, FuncVD);
+
+ if (ProfWeight == 1U) {
+ Writer.addRecord(std::move(Record1), Err);
+ } else {
+ Writer.addRecord(std::move(Record1), ProfWeight, Err);
+ }
+ Writer.addRecord({"callee1", 0x1235, {3, 4}}, Err);
+ Writer.addRecord({"callee2", 0x1235, {3, 4}}, Err);
+ Writer.addRecord({"callee3", 0x1235, {3, 4}}, Err);
+
+ // Set writer endianness.
+ Writer.setValueProfDataEndianness(Endianness);
+
+ auto Profile = Writer.writeBuffer();
+ readProfile(std::move(Profile));
+
+ // Set reader endianness.
+ Reader->setValueProfDataEndianness(Endianness);
+
+ Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
+ EXPECT_THAT_ERROR(R.takeError(), Succeeded());
+
+ std::vector<std::pair<uint32_t, VDArray &>> InputVDs = {
+ std::pair<uint32_t, VDArray &>{IPVK_IndirectCallTarget, FuncVD}};
+ testValueDataArray(InputVDs, R.get(), true, ProfWeight);
+ }
};
TEST_P(MaybeSparseInstrProfTest, write_and_read_empty_profile) {
@@ -79,11 +206,6 @@ TEST_P(MaybeSparseInstrProfTest, write_and_read_empty_profile) {
ASSERT_TRUE(Reader->begin() == Reader->end());
}
-static const auto Err = [](Error E) {
- consumeError(std::move(E));
- FAIL();
-};
-
TEST_P(MaybeSparseInstrProfTest, write_and_read_one_function) {
Writer.addRecord({"foo", 0x1234, {1, 2, 3, 4}}, Err);
auto Profile = Writer.writeBuffer();
@@ -633,55 +755,8 @@ TEST_F(InstrProfTest, test_irpgo_read_deprecated_names) {
Succeeded());
}
-static const char callee1[] = "callee1";
-static const char callee2[] = "callee2";
-static const char callee3[] = "callee3";
-static const char callee4[] = "callee4";
-static const char callee5[] = "callee5";
-static const char callee6[] = "callee6";
-
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
- NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
-
- // 4 value sites.
- Record1.reserveSites(IPVK_IndirectCallTarget, 4);
- InstrProfValueData VD0[] = {
- {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
- Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
- // No value profile data at the second site.
- Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
- InstrProfValueData VD2[] = {{(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}};
- Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, 2, nullptr);
- InstrProfValueData VD3[] = {{(uint64_t)callee1, 1}};
- Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
-
- Writer.addRecord(std::move(Record1), Err);
- Writer.addRecord({"callee1", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee2", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee3", 0x1235, {3, 4}}, Err);
- auto Profile = Writer.writeBuffer();
- readProfile(std::move(Profile));
-
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
- EXPECT_THAT_ERROR(R.takeError(), Succeeded());
- ASSERT_EQ(4U, R->getNumValueSites(IPVK_IndirectCallTarget));
- ASSERT_EQ(3U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
- ASSERT_EQ(0U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 1));
- ASSERT_EQ(2U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 2));
- ASSERT_EQ(1U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 3));
-
- uint64_t TotalC;
- std::unique_ptr<InstrProfValueData[]> VD =
- R->getValueForSite(IPVK_IndirectCallTarget, 0, &TotalC);
-
- ASSERT_EQ(3U, VD[0].Count);
- ASSERT_EQ(2U, VD[1].Count);
- ASSERT_EQ(1U, VD[2].Count);
- ASSERT_EQ(6U, TotalC);
-
- ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
- ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
- ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
+TEST_P(MaybeSparseInstrProfTest, icall_data_read_write) {
+ testICallDataReadWrite(1 /* ProfWeight */);
}
TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
@@ -780,142 +855,41 @@ TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
ASSERT_EQ(1U, ValueData[3].Count);
}
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_with_weight) {
- NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
-
- // 4 value sites.
- Record1.reserveSites(IPVK_IndirectCallTarget, 4);
- InstrProfValueData VD0[] = {
- {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
- Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
- // No value profile data at the second site.
- Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
- InstrProfValueData VD2[] = {{(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}};
- Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, 2, nullptr);
- InstrProfValueData VD3[] = {{(uint64_t)callee1, 1}};
- Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
-
- Writer.addRecord(std::move(Record1), 10, Err);
- Writer.addRecord({"callee1", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee2", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee3", 0x1235, {3, 4}}, Err);
- auto Profile = Writer.writeBuffer();
- readProfile(std::move(Profile));
-
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
- EXPECT_THAT_ERROR(R.takeError(), Succeeded());
- ASSERT_EQ(4U, R->getNumValueSites(IPVK_IndirectCallTarget));
- ASSERT_EQ(3U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
- ASSERT_EQ(0U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 1));
- ASSERT_EQ(2U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 2));
- ASSERT_EQ(1U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 3));
-
- uint64_t TotalC;
- std::unique_ptr<InstrProfValueData[]> VD =
- R->getValueForSite(IPVK_IndirectCallTarget, 0, &TotalC);
- ASSERT_EQ(30U, VD[0].Count);
- ASSERT_EQ(20U, VD[1].Count);
- ASSERT_EQ(10U, VD[2].Count);
- ASSERT_EQ(60U, TotalC);
-
- ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
- ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
- ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
+TEST_P(MaybeSparseInstrProfTest, icall_data_read_write_with_weight) {
+ testICallDataReadWrite(10 /* ProfWeight */);
}
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write_big_endian) {
- NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
-
- // 4 value sites.
- Record1.reserveSites(IPVK_IndirectCallTarget, 4);
- InstrProfValueData VD0[] = {
- {(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}, {(uint64_t)callee3, 3}};
- Record1.addValueData(IPVK_IndirectCallTarget, 0, VD0, 3, nullptr);
- // No value profile data at the second site.
- Record1.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
- InstrProfValueData VD2[] = {{(uint64_t)callee1, 1}, {(uint64_t)callee2, 2}};
- Record1.addValueData(IPVK_IndirectCallTarget, 2, VD2, 2, nullptr);
- InstrProfValueData VD3[] = {{(uint64_t)callee1, 1}};
- Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
-
- Writer.addRecord(std::move(Record1), Err);
- Writer.addRecord({"callee1", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee2", 0x1235, {3, 4}}, Err);
- Writer.addRecord({"callee3", 0x1235, {3, 4}}, Err);
-
- // Set big endian output.
- Writer.setValueProfDataEndianness(llvm::endianness::big);
-
- auto Profile = Writer.writeBuffer();
- readProfile(std::move(Profile));
-
- // Set big endian input.
- Reader->setValueProfDataEndianness(llvm::endianness::big);
-
- Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
- EXPECT_THAT_ERROR(R.takeError(), Succeeded());
- ASSERT_EQ(4U, R->getNumValueSites(IPVK_IndirectCallTarget));
- ASSERT_EQ(3U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 0));
- ASSERT_EQ(0U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 1));
- ASSERT_EQ(2U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 2));
- ASSERT_EQ(1U, R->getNumValueDataForSite(IPVK_IndirectCallTarget, 3));
-
- std::unique_ptr<InstrProfValueData[]> VD =
- R->getValueForSite(IPVK_IndirectCallTarget, 0);
- ASSERT_EQ(StringRef((const char *)VD[0].Value, 7), StringRef("callee3"));
- ASSERT_EQ(StringRef((const char *)VD[1].Value, 7), StringRef("callee2"));
- ASSERT_EQ(StringRef((const char *)VD[2].Value, 7), StringRef("callee1"));
-
- // Restore little endian default:
- Writer.setValueProfDataEndianness(llvm::endianness::little);
+TEST_P(MaybeSparseInstrProfTest, icall_data_read_write_big_endian) {
+ testICallDataReadWrite(1 /* ProfWeight */, llvm::endianness::big);
}
-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});
// 5 value sites.
- Record11.reserveSites(IPVK_IndirectCallTarget, 5);
- InstrProfValueData VD0[] = {{uint64_t(callee1), 1},
- {uint64_t(callee2), 2},
- {uint64_t(callee3), 3},
- {uint64_t(callee4), 4}};
- Record11.addValueData(IPVK_IndirectCallTarget, 0, VD0, 4, nullptr);
-
- // No value profile data at the second site.
- Record11.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
-
- InstrProfValueData VD2[] = {
+ std::vector<InstrProfValueData> VD0 = {{uint64_t(callee1), 1},
+ {uint64_t(callee2), 2},
+ {uint64_t(callee3), 3},
+ {uint64_t(callee4), 4}};
+ std::vector<InstrProfValueData> VD2 = {
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
- Record11.addValueData(IPVK_IndirectCallTarget, 2, VD2, 3, nullptr);
-
- InstrProfValueData VD3[] = {{uint64_t(callee1), 1}};
- Record11.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
-
- InstrProfValueData VD4[] = {{uint64_t(callee1), 1},
- {uint64_t(callee2), 2},
- {uint64_t(callee3), 3}};
- Record11.addValueData(IPVK_IndirectCallTarget, 4, VD4, 3, nullptr);
+ std::vector<InstrProfValueData> VD3 = {{uint64_t(callee1), 1}};
+ std::vector<InstrProfValueData> VD4 = {
+ {uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
+ VDArray FuncVD0 = {VD0, {}, VD2, VD3, VD4};
+ addValueProfData(Record11, IPVK_IndirectCallTarget, FuncVD0);
// A different record for the same caller.
- Record12.reserveSites(IPVK_IndirectCallTarget, 5);
- InstrProfValueData VD02[] = {{uint64_t(callee2), 5}, {uint64_t(callee3), 3}};
- Record12.addValueData(IPVK_IndirectCallTarget, 0, VD02, 2, nullptr);
-
- // No value profile data at the second site.
- Record12.addValueData(IPVK_IndirectCallTarget, 1, nullptr, 0, nullptr);
-
- InstrProfValueData VD22[] = {
+ std::vector<InstrProfValueData> VD02 = {{uint64_t(callee2), 5},
+ {uint64_t(callee3), 3}};
+ std::vector<InstrProfValueData> VD22 = {
{uint64_t(callee2), 1}, {uint64_t(callee3), 3}, {uint64_t(callee4), 4}};
- Record12.addValueData(IPVK_IndirectCallTarget, 2, VD22, 3, nullptr);
-
- Record12.addValueData(IPVK_IndirectCallTarget, 3, nullptr, 0, nullptr);
-
- InstrProfValueData VD42[] = {{uint64_t(callee1), 1},
- {uint64_t(callee2), 2},
- {uint64_t(callee3), 3}};
- Record12.addValueData(IPVK_IndirectCallTarget, 4, VD42, 3, nullptr);
+ std::vector<InstrProfValueData> VD42 = {
+ {uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}};
+ VDArray FuncVD1 = {VD02, {}, VD22, {}, VD42};
+ addValueProfData(Record12, IPVK_IndirectCallTarget, FuncVD1);
Writer.addRecord(std::move(Record11), Err);
// Merge profile data.
@@ -975,94 +949,121 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) {
ASSERT_EQ(2U, VD_4[2].Count);
}
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1_saturation) {
- static const char bar[] = "bar";
-
- const uint64_t MaxValCount = std::numeric_limits<uint64_t>::max();
+// Tests the block counter overflow scenario for merge of profile records.
+TEST_P(MaybeSparseInstrProfTest, test_block_counter_merge_saturation) {
+ // The max edge count is smaller than std::numeric_limits<uint64_t>::max().
+ // See the method for details.
const uint64_t MaxEdgeCount = getInstrMaxCountValue();
- instrprof_error Result;
+ instrprof_error Result = instrprof_error::success;
auto Err = [&](Error E) {
Result = std::get<0>(InstrProfError::take(std::move(E)));
};
- Result = instrprof_error::success;
Writer.addRecord({"foo", 0x1234, {1}}, Err);
+ // The first record should be fine.
ASSERT_EQ(Result, instrprof_error::success);
// Verify counter overflow.
Result = instrprof_error::success;
Writer.addRecord({"foo", 0x1234, {MaxEdgeCount}}, Err);
- ASSERT_EQ(Result, instrprof_error::counter_overflow);
+ EXPECT_EQ(Result, instrprof_error::counter_overflow);
+
+ auto Profile = Writer.writeBuffer();
+ readProfile(std::move(Profile));
+
+ Expected<InstrProfRecord> ReadRecord1 =
+ Reader->getInstrProfRecord("foo", 0x1234);
+ EXPECT_THAT_ERROR(ReadRecord1.takeError(), Succeeded());
+ // Verify saturation of counts.
+ EXPECT_EQ(MaxEdgeCount, ReadRecord1->Counts[0]);
+}
+
+void MaybeSparseInstrProfTest::testValueProfileMergeSaturation(
+ uint32_t ValueKind) {
+ assert(ValueKind == IPVK_IndirectCallTarget);
+ static const char bar[] = "bar";
+
+ const uint64_t ProfiledValue = uint64_t(bar);
+
+ InstrProfValueData VDWithCounterOne[] = {{ProfiledValue, 1}};
+
+ const uint64_t MaxValCount = std::numeric_limits<uint64_t>::max();
+
+ InstrProfValueData VDWithMaxValCount[] = {{ProfiledValue, MaxValCount}};
+
+ instrprof_error Result;
+ auto Err = [&](Error E) {
+ Result = std::get<0>(InstrProfError::take(std::move(E)));
+ };
Result = instrprof_error::success;
Writer.addRecord({bar, 0x9012, {8}}, Err);
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);
+
+ Record4.addValueData(ValueKind, 0, VDWithCounterOne, 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(ValueK...
[truncated]
|
david-xl
reviewed
Nov 18, 2023
This could be further split. Keep the refactor of three related tests in this PR, and going to use separate PRs for the rest. |
david-xl
reviewed
Nov 20, 2023
| // adding a record. `Endianness` specifies the endianness used by profile | ||
| // writer and reader when handling value profile records. | ||
| void testICallDataReadWrite( | ||
| uint64_t ProfWeight, |
There was a problem hiding this comment.
Can set default weight to 1.
| // profiles. The profile writer will scale counters by `ProfWeight` when | ||
| // adding a record. `Endianness` specifies the endianness used by profile | ||
| // writer and reader when handling value profile records. | ||
| void testICallDataReadWrite( |
There was a problem hiding this comment.
Can this setup method be shared with other value profile kind?
There was a problem hiding this comment.
yes. The plan is to extend this method (rename to 'testICallAndVTableDataReadWrite` and add vtable value profiles) in the patch that introduces vtable as a kind of value profiles.
david-xl
approved these changes
Nov 20, 2023
snehasish
reviewed
Nov 20, 2023
| Reader->setValueProfDataEndianness(Endianness); | ||
|
|
||
| Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234); | ||
| EXPECT_THAT_ERROR(R.takeError(), Succeeded()); |
There was a problem hiding this comment.
I think this should be an assertion since we shouldn't access the contents of R if this fails.
| Writer.setValueProfDataEndianness(llvm::endianness::little); | ||
| Reader->setValueProfDataEndianness(llvm::endianness::little); |
There was a problem hiding this comment.
Why is this needed now? It makes me think that there is some dependency across the tests..
There was a problem hiding this comment.
The previous test case doesn't have it and the whole unit test passes. I'm adding this line mainly to reset the state but tbh I didn't dig into how this passed previously.
| // writer and reader when handling value profile records. | ||
| void testICallDataReadWrite( | ||
| uint64_t ProfWeight = 1, | ||
| llvm::endianness Endianness = llvm::endianness::little) { |
There was a problem hiding this comment.
I would like to keep the test assertions/expectations in the test themselves, using this helper makes the tests harder to debug on failure. Here is a slightly different refactoring you can consider: These are already parameterized tests so lets extend the params to include endianness and weights. We can combine the params in INSTANTIATE_TEST_SUITE_P using
test::Combine(testing::Bool(),
Values(TestParam{/*endian=*/little, /*weight=*/1},
TestParam{/*endian=*/little, /*weight=*/10},
TestParam{/*endian=*/big, /*weight=*/1}
)
)
Here TestParam is a new struct which is introduced. This will keep the existing coverage. You can also increase coverage by combining all cases (as cartesian product).
test::Combine(testing::Bool(), // sparse?
Values(little, big), // endianness
Values(1, 10), // weights
)
)
What do you think? Feel free to avoid if this is requires more time than you want to invest.
There was a problem hiding this comment.
At a high level, I'm in favor of keeping test assertions/expectations so that the line numbers are obvious when a test fails.
Future parameterizing the test fixture is a good step, but a cartesian product over all existing test cases of MaybeSparseInstrProfTest would create more test cases for all combinations that become vague to reason about (i.e., value_prof_data_read_write tests serialization/deserialization of InstrProfRecord but invoke profile reader or writer) or become to adds maintain overhead (i.e., when new aspects are added but not applicable, a test case will do something like (if std::get<N>(getParam()) == irrelevant) return early.)
Since it would probably require more work to refactor the unittest, could I parallel track the test coverage of new feature #66825 and the improvement of unit tests? This way it's less work to keep up with upstream main.
There was a problem hiding this comment.
Yes, I am also a little wary of what we might find if we do a cartesian product :)
I'm ok to defer the refactoring so that we can account for the new vtable prof test too. Thanks!
There was a problem hiding this comment.
As chatted offline, I realized test coverage for a new kind of value-type might double the amount of code without factoring out common code once I attempted to add new tests. Thanks for going over the test cases together and discussing about how to properly clean it up and prepare for the new value type. I'm sending out smaller patches.
| addValueProfData(Record1, IPVK_IndirectCallTarget, FuncVD); | ||
|
|
||
| if (ProfWeight == 1U) { | ||
| Writer.addRecord(std::move(Record1), Err); |
There was a problem hiding this comment.
addRecord without a weight is a thin convenience wrapper, I think it's ok to drop this if condition in favour of directly passing ProfWeight.
There was a problem hiding this comment.
yep! This if-else is for the test coverage of the thin-wrapper. I could do this in the further parameterized version later.
Github doesn't support re-open of a merged PR so I'll create new PRs for the test.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Three existing test cases {get_icall_data_read_write, get_icall_data_read_write_with_weight, get_icall_data_read_write_big_endian} have common test data and testing logic. Extract common code into
testICallDataReadWrite.addValueProfDataandtestValueDataArray. This two helper functions are used bytestICallDataReadWrite, and possibly other test cases.