-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[nfc][InstrProfTest]Factor out common code for value profile test #72611
Changes from all commits
d55ea10
5e2ce69
f49811c
1064235
889ed43
d35eb74
1076564
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,13 +64,130 @@ 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; | ||
|
||
// This helper function adds the value profile data to Record. The type of | ||
// value profiles is specified by 'ValueKind'. 'ValueDataArray' is a non-const | ||
// reference and the vector element is a mutable array reference. This is mainly | ||
// because method `InstrProfRecord::addValueData` takes a pointer and might | ||
// modify the pointed-to content. | ||
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. | ||
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); } | ||
}; | ||
|
||
struct MaybeSparseInstrProfTest : public InstrProfTest, | ||
public ::testing::WithParamInterface<bool> { | ||
void SetUp() override { Writer.setOutputSparse(GetParam()); } | ||
|
||
public: | ||
// 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) { | ||
ASSERT_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 purpose. | ||
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); | ||
} | ||
} | ||
} | ||
|
||
// A helper function to test the writes and reads of indirect call value | ||
// 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( | ||
uint64_t ProfWeight = 1, | ||
llvm::endianness Endianness = llvm::endianness::little) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
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).
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I am also a little wary of what we might find if we do a cartesian product :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep! This Github doesn't support re-open of a merged PR so I'll create new PRs for the test. |
||
} 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be an assertion since we shouldn't access the contents of R if this fails. |
||
|
||
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 +196,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 +745,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(); | ||
} | ||
|
||
TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) { | ||
|
@@ -780,94 +845,15 @@ 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: | ||
TEST_P(MaybeSparseInstrProfTest, icall_data_read_write_big_endian) { | ||
testICallDataReadWrite(1 /* ProfWeight */, llvm::endianness::big); | ||
// Restore little endianness after this test case. | ||
Writer.setValueProfDataEndianness(llvm::endianness::little); | ||
Reader->setValueProfDataEndianness(llvm::endianness::little); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed now? It makes me think that there is some dependency across the tests.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) { | ||
|
@@ -893,9 +879,8 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) { | |
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}}; | ||
InstrProfValueData VD4[] = { | ||
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}}; | ||
Record11.addValueData(IPVK_IndirectCallTarget, 4, VD4, 3, nullptr); | ||
|
||
// A different record for the same caller. | ||
|
@@ -912,9 +897,8 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_merge1) { | |
|
||
Record12.addValueData(IPVK_IndirectCallTarget, 3, nullptr, 0, nullptr); | ||
|
||
InstrProfValueData VD42[] = {{uint64_t(callee1), 1}, | ||
{uint64_t(callee2), 2}, | ||
{uint64_t(callee3), 3}}; | ||
InstrProfValueData VD42[] = { | ||
{uint64_t(callee1), 1}, {uint64_t(callee2), 2}, {uint64_t(callee3), 3}}; | ||
Record12.addValueData(IPVK_IndirectCallTarget, 4, VD42, 3, nullptr); | ||
|
||
Writer.addRecord(std::move(Record11), Err); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this setup method be shared with other value profile kind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.