[PATCH] D10674: Value profiling - patchset 3
Betul Buyukkurt
betulb at codeaurora.org
Thu Jul 23 11:45:36 PDT 2015
Hi All,
I've been working on a separate project thus am starting to look at this as of today. I'll have the first implementation ready and respond to the comments early next week.
Thanks,
-Betul
-----Original Message-----
From: David [mailto:davidxl at google.com]
Sent: Sunday, July 19, 2015 8:47 PM
To: betulb at codeaurora.org; mail at justinbogner.com; davidxl at google.com; dnovillo at google.com; dsule at codeaurora.org; ibaev at codeaurora.org
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: [PATCH] D10674: Value profiling - patchset 3
davidxl added inline comments.
================
Comment at: include/llvm/ProfileData/InstrProf.h:51
@@ +50,3 @@
+ indirect_call_target = 0,
+ reserved_1 = 1,
+ reserved_2 = 2,
----------------
Remove the reserved kinds.
================
Comment at: include/llvm/ProfileData/InstrProf.h:59
@@ +58,3 @@
+// Profiling information for a single target value struct
+InstrProfValueRecord {
+ InstrProfValueRecord() {};
----------------
It is better to make this structure similar to InstrProfRecord, which is itself a container class instead of just representing one single target value. It will make the value profile merging more efficient -- see comments at the value profile merging code.
Suggested layout of the structure is similar to how value profile is represented on disk basically a value site map pointing to the collected values profile data per site. Each site's value profile data is represented as small linked list (similar to compile_rt rep):
struct InstrValue {
uint64_t Value; // Value or Target Name String pointer
uint64_t NumTaken;
InstrValue *Next;
};
struct InstrProfValueRecord {
vector<InstrValue*> SiteValues; // Size of the vector is the number of sites in func };
================
Comment at: include/llvm/ProfileData/InstrProf.h:64
@@ +63,3 @@
+ CounterIndex(CounterIndex) {}
+ SmallString<32> Name;
+ uint64_t Value;
----------------
No need for both Name and Value.
================
Comment at: include/llvm/ProfileData/InstrProf.h:67
@@ +66,3 @@
+ uint64_t NumTaken;
+ uint32_t CounterIndex;
+};
----------------
No need for CounterIndex.
================
Comment at: include/llvm/ProfileData/InstrProf.h:78
@@ -54,2 +77,3 @@
std::vector<uint64_t> Counts;
+ std::vector<InstrProfValueRecord> Values[instrprof_value_kind::size];
};
----------------
This will become:
InstrProfValueRecord *Values[instrprof_value_kind::size];
================
Comment at: include/llvm/ProfileData/InstrProfWriter.h:29
@@ +28,3 @@
+/// IndexedInstrProfWriter
+struct IndexedInstrProfRecord {
+ IndexedInstrProfRecord() {}
----------------
Is there a need for this data type ? InstrProfRecord can be used for the writer too.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:376
@@ +375,3 @@
+ // Read value profiling data
+ for (uint32_t Kind = instrprof_value_kind::first;
+ Kind < instrprof_value_kind::size; ++Kind) {
----------------
Remove the loop for now.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:381
@@ +380,3 @@
+ return data_type();
+ uint32_t NumValues = endian::readNext<uint32_t, little, unaligned>(D);
+ ValueBuffer.clear();
----------------
NumValues field should be followed by NumSites field.
================
Comment at: lib/ProfileData/InstrProfReader.cpp:383
@@ +382,3 @@
+ ValueBuffer.clear();
+ for (unsigned I = 0; I < NumValues; ++I) {
+ // Read size of the name string
----------------
Should do loop with value site index: for (index = 0; index < NumSites; index++) {
================
Comment at: lib/ProfileData/InstrProfReader.cpp:387
@@ +386,3 @@
+ return data_type();
+ uint32_t NameSize = endian::readNext<uint32_t, little, unaligned>(D);
+ // Read name string
----------------
See comment in EmitData about format change suggestions regarding name strings
================
Comment at: lib/ProfileData/InstrProfReader.cpp:396
@@ +395,3 @@
+ return data_type();
+ uint64_t Value = endian::readNext<uint64_t, little, unaligned>(D);
+ uint64_t NumTaken = endian::readNext<uint64_t, little,
+ unaligned>(D);
----------------
Each site starts with a field: num_of_targets
================
Comment at: lib/ProfileData/InstrProfReader.cpp:398
@@ +397,3 @@
+ uint64_t NumTaken = endian::readNext<uint64_t, little, unaligned>(D);
+ uint32_t Index = endian::readNext<uint32_t, little, unaligned>(D);
+ ValueBuffer.push_back(
----------------
skip the Index field (should not be recorded)
================
Comment at: lib/ProfileData/InstrProfReader.cpp:400
@@ +399,3 @@
+ ValueBuffer.push_back(
+ InstrProfValueRecord(Name, Value, NumTaken, Index));
+ }
----------------
See suggestions above about making InstrProfValueRecord a container class
================
Comment at: lib/ProfileData/InstrProfWriter.cpp:85
@@ -68,1 +84,3 @@
LE.write<uint64_t>(I);
+ for (uint32_t Kind = instrprof_value_kind::first;
+ Kind < instrprof_value_kind::size; ++Kind) {
----------------
Commented in another thread, the suggested new format is:
[value profile start]
value_kind
number_of_sites
[sites start]
num_of_targets (site 0)
target_value
target_count
target_value
target_count
....
num_of_targets (site k)
target_value
target_count
Note that the value for indirect_target_kind should be the file offset of the target name string in the string section. There is a string section per function to record the indirect target function strings. Note that the writer should also perform string commoning to reduce disk space. The disk format with string should look like:
[string section start]
string_section_size
string_1 // null terminated
string_2 // ...
..
[value profile start]
value_kind
number_of_sites
[sites start]
num_of_targets (site 0)
target_value
target_count
target_value
target_count
....
num_of_targets (site k)
target_value
target_count
================
Comment at: lib/ProfileData/InstrProfWriter.cpp:118
@@ +117,3 @@
+ Kind < instrprof_value_kind::size; ++Kind) {
+ std::vector<InstrProfValueRecord>::const_iterator I =
+ Source.Values[Kind].begin();
----------------
This has quadratic complexity. Please change the data structure of InstrProfValueRecord into an array of short linked list so that the search/match can be much faster (assuming average number of targets is a small const, the new complexity will be linear).
http://reviews.llvm.org/D10674
More information about the llvm-commits
mailing list