-
Notifications
You must be signed in to change notification settings - Fork 11.3k
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]Add a test fixture to parameterize the read-write test of value profiles #73038
Conversation
… test of value profiles
@llvm/pr-subscribers-pgo Author: Mingming Liu (minglotus-6) ChangesThis patch factor out the common code among three similar test cases. The input data and test logic are pretty similar. Parameterize the differences (prof-weight and endianness) as advised in #72611.
Github doesn't support re-open of a merged PR so far so created new PRs rather than re-land a test-only change. Full diff: https://github.com/llvm/llvm-project/pull/73038.diff 1 Files Affected:
diff --git a/llvm/unittests/ProfileData/InstrProfTest.cpp b/llvm/unittests/ProfileData/InstrProfTest.cpp
index f26f244afc5378e..2aca6fe3d88b075 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"
@@ -68,6 +69,25 @@ struct SparseInstrProfTest : public InstrProfTest {
void SetUp() override { Writer.setOutputSparse(true); }
};
+struct InstrProfReaderWriterTest
+ : public InstrProfTest,
+ public ::testing::WithParamInterface<
+ std::tuple<bool, uint64_t, llvm::endianness>> {
+ void SetUp() override { Writer.setOutputSparse(std::get<0>(GetParam())); }
+ void TearDown() override {
+ // Reset writer value profile data endianness after each test case. Note
+ // it's not necessary to reset reader value profile endianness for each test
+ // case. Each test case creates a new reader; at reader initialization time,
+ // it uses the endianness from hash table object (which is little by
+ // default).
+ Writer.setValueProfDataEndianness(llvm::endianness::little);
+ }
+
+ uint64_t getProfWeight() const { return std::get<1>(GetParam()); }
+
+ llvm::endianness getEndianness() const { return std::get<2>(GetParam()); }
+};
+
struct MaybeSparseInstrProfTest : public InstrProfTest,
public ::testing::WithParamInterface<bool> {
void SetUp() override { Writer.setOutputSparse(GetParam()); }
@@ -640,7 +660,7 @@ static const char callee4[] = "callee4";
static const char callee5[] = "callee5";
static const char callee6[] = "callee6";
-TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
+TEST_P(InstrProfReaderWriterTest, icall_data_read_write) {
NamedInstrProfRecord Record1("caller", 0x1234, {1, 2});
// 4 value sites.
@@ -655,13 +675,20 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
InstrProfValueData VD3[] = {{(uint64_t)callee1, 1}};
Record1.addValueData(IPVK_IndirectCallTarget, 3, VD3, 1, nullptr);
- Writer.addRecord(std::move(Record1), Err);
+ Writer.addRecord(std::move(Record1), getProfWeight(), Err);
Writer.addRecord({"callee1", 0x1235, {3, 4}}, Err);
Writer.addRecord({"callee2", 0x1235, {3, 4}}, Err);
Writer.addRecord({"callee3", 0x1235, {3, 4}}, Err);
+
+ // Set writer value prof data endianness.
+ Writer.setValueProfDataEndianness(getEndianness());
+
auto Profile = Writer.writeBuffer();
readProfile(std::move(Profile));
+ // Set reader value prof data endianness.
+ Reader->setValueProfDataEndianness(getEndianness());
+
Expected<InstrProfRecord> R = Reader->getInstrProfRecord("caller", 0x1234);
EXPECT_THAT_ERROR(R.takeError(), Succeeded());
ASSERT_EQ(4U, R->getNumValueSites(IPVK_IndirectCallTarget));
@@ -674,16 +701,25 @@ TEST_P(MaybeSparseInstrProfTest, get_icall_data_read_write) {
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(3U * getProfWeight(), VD[0].Count);
+ ASSERT_EQ(2U * getProfWeight(), VD[1].Count);
+ ASSERT_EQ(1U * getProfWeight(), VD[2].Count);
+ ASSERT_EQ(6U * getProfWeight(), 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"));
}
+INSTANTIATE_TEST_SUITE_P(
+ WeightAndEndiannessTest, InstrProfReaderWriterTest,
+ ::testing::Combine(
+ ::testing::Bool(), /* Sparse */
+ ::testing::Values(1U, 10U), /* Weight */
+ ::testing::Values(llvm::endianness::big,
+ llvm::endianness::little) /* Endianness */
+ ));
+
TEST_P(MaybeSparseInstrProfTest, annotate_vp_data) {
NamedInstrProfRecord Record("caller", 0x1234, {1, 2});
Record.reserveSites(IPVK_IndirectCallTarget, 1);
@@ -780,96 +816,6 @@ 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, 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, get_icall_data_merge1) {
static const char caller[] = "caller";
NamedInstrProfRecord Record11(caller, 0x1234, {1, 2});
|
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.
lgtm
@@ -6,6 +6,7 @@ | |||
// | |||
//===----------------------------------------------------------------------===// | |||
|
|||
#include "llvm/ADT/bit.h" |
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.
I don't think this is necessary?
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. removed it.
I tried to use a struct to represent test values rather than using std::tuple
in the middle, and the header is needed in the middle (if I have need llvm::endianness
). A struct is a little bit more expressive but chose to use std::tuple
in order to use testing::Combine
and testing::Values
to generate test parameters.
WeightAndEndiannessTest, InstrProfReaderWriterTest, | ||
::testing::Combine( | ||
::testing::Bool(), /* Sparse */ | ||
::testing::Values(1U, 10U), /* Weight */ |
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.
nit: ProfWeight
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.
done.
This patch factor out the common code among three similar test cases. The input data and test logic are pretty similar. Parameterize the differences (prof-weight and endianness) as advised in #72611.
Github doesn't support re-open of a merged PR so far so created new PRs rather than re-land a test-only change.