<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Jun 27, 2017 at 10:17 AM Xinliang David Li <<a href="mailto:davidxl@google.com">davidxl@google.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Jun 27, 2017 at 9:35 AM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 24, 2015 at 10:26 PM Xinliang David Li via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: davidxl<br>
Date: Wed Nov 25 00:23:38 2015<br>
New Revision: 254057<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=254057&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=254057&view=rev</a><br>
Log:<br>
[PGO] Convert InstrProfRecord based serialization methods to use common C methods<br>
<br>
1. Convert serialization methods using InstrProfRecord as source into C (impl)<br>
interfaces using Closure.<br>
2. Reimplement InstrProfRecord serialization method to use new C interface<br>
as dummy wrapper.<br>
<br>
Now it is ready to implement wrapper for runtime value profile data.<br>
<br>
(The new code need better source location -- but not changed in this patch to<br>
minimize diffs. )<br>
<br>
<br>
Modified:<br>
llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=254057&r1=254056&r2=254057&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=254057&r1=254056&r2=254057&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)<br>
+++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Nov 25 00:23:38 2015<br>
@@ -474,9 +474,6 @@ typedef struct ValueProfRecord {<br>
/// Read data from this record and save it to Record.<br>
void deserializeTo(InstrProfRecord &Record,<br>
InstrProfRecord::ValueMapType *VMap);<br>
- /// Extract data from \c Record and serialize into this instance.<br>
- void serializeFrom(const InstrProfRecord &Record, uint32_t ValueKind,<br>
- uint32_t NumValueSites);<br>
/// In-place byte swap:<br>
/// Do byte swap for this instance. \c Old is the original order before<br>
/// the swap, and \c New is the New byte order.<br>
@@ -524,8 +521,6 @@ typedef struct ValueProfData {<br>
/// Read data from this data and save it to \c Record.<br>
void deserializeTo(InstrProfRecord &Record,<br>
InstrProfRecord::ValueMapType *VMap);<br>
- /// Return the first \c ValueProfRecord instance.<br>
- ValueProfRecord *getFirstValueProfRecord();<br>
} ValueProfData;<br>
<br>
/* The closure is designed to abstact away two types of value profile data:<br>
@@ -542,21 +537,20 @@ typedef struct ValueProfData {<br>
* in class InstrProfRecord.<br>
*/<br>
typedef struct ValueProfRecordClosure {<br>
- void *Record;<br>
- uint32_t (*GetNumValueKinds)(void *Record);<br>
- uint32_t (*GetNumValueSites)(void *Record, uint32_t VKind);<br>
- uint32_t (*GetNumValueData)(void *Record, uint32_t VKind);<br>
- uint32_t (*GetNumValueDataForSite)(void *R, uint32_t VK, uint32_t S);<br>
+ const void *Record;<br>
+ uint32_t (*GetNumValueKinds)(const void *Record);<br>
+ uint32_t (*GetNumValueSites)(const void *Record, uint32_t VKind);<br>
+ uint32_t (*GetNumValueData)(const void *Record, uint32_t VKind);<br>
+ uint32_t (*GetNumValueDataForSite)(const void *R, uint32_t VK, uint32_t S);<br>
<br>
/* After extracting the value profile data from the value profile record,<br>
* this method is used to map the in-memory value to on-disk value. If<br>
* the method is null, value will be written out untranslated.<br>
*/<br>
uint64_t (*RemapValueData)(uint32_t, uint64_t Value);<br>
- void (*GetValueForSite)(InstrProfValueData *Dst, void *R, uint32_t K,<br>
+ void (*GetValueForSite)(const void *R, InstrProfValueData *Dst, uint32_t K,<br>
uint32_t S, uint64_t (*Mapper)(uint32_t, uint64_t));<br>
-<br>
- ValueProfData *(*AllocateValueProfData)(size_t TotalSizeInBytes);<br>
+ ValueProfData *(*AllocValueProfData)(size_t TotalSizeInBytes);<br>
} ValueProfRecordClosure;<br>
<br>
/// Return the \c ValueProfRecord header size including the padding bytes.<br>
@@ -599,6 +593,11 @@ inline ValueProfRecord *getValueProfReco<br>
NumValueData));<br>
}<br>
<br>
+/// Return the first \c ValueProfRecord instance.<br>
+inline ValueProfRecord *getFirstValueProfRecord(ValueProfData *This) {<br>
+ return (ValueProfRecord *)((char *)This + sizeof(ValueProfData));<br>
+}<br>
+<br>
namespace IndexedInstrProf {<br>
<br>
enum class HashT : uint32_t {<br>
<br>
Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=254057&r1=254056&r2=254057&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/ProfileData/InstrProf.cpp?rev=254057&r1=254056&r2=254057&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)<br>
+++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Nov 25 00:23:38 2015<br>
@@ -155,16 +155,21 @@ void ValueProfRecord::deserializeTo(Inst<br>
}<br>
}<br>
<br>
-void ValueProfRecord::serializeFrom(const InstrProfRecord &Record,<br>
- uint32_t ValueKind,<br>
- uint32_t NumValueSites) {<br>
- Kind = ValueKind;<br>
- this->NumValueSites = NumValueSites;<br>
- InstrProfValueData *DstVD = getValueProfRecordValueData(this);<br>
- for (uint32_t S = 0; S < NumValueSites; S++) {<br>
- uint32_t ND = Record.getNumValueDataForSite(ValueKind, S);<br>
- SiteCountArray[S] = ND;<br>
- Record.getValueForSite(DstVD, ValueKind, S, stringToHash);<br>
+// Extract data from \c Closure and serialize into \c This instance.<br>
+void serializeValueProfRecordFrom(ValueProfRecord *This,<br>
+ ValueProfRecordClosure *Closure,<br>
+ uint32_t ValueKind, uint32_t NumValueSites) {<br>
+ uint32_t S;<br>
+ const void *Record = Closure->Record;<br>
+ This->Kind = ValueKind;<br>
+ This->NumValueSites = NumValueSites;<br>
+ InstrProfValueData *DstVD = getValueProfRecordValueData(This);<br>
+<br>
+ for (S = 0; S < NumValueSites; S++) {<br>
+ uint32_t ND = Closure->GetNumValueDataForSite(Record, ValueKind, S);<br>
+ This->SiteCountArray[S] = ND;<br>
+ Closure->GetValueForSite(Record, DstVD, ValueKind, S,<br>
+ Closure->RemapValueData);<br>
DstVD += ND;<br>
}<br>
}<br>
@@ -205,18 +210,22 @@ void ValueProfRecord::swapBytes(support:<br>
}<br>
}<br>
<br>
-uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {<br>
+/// Return the total size in bytes of the on-disk value profile data<br>
+/// given the data stored in Record.<br>
+uint32_t getValueProfDataSize(ValueProfRecordClosure *Closure) {<br>
+ uint32_t Kind;<br>
uint32_t TotalSize = sizeof(ValueProfData);<br>
- uint32_t NumValueKinds = Record.getNumValueKinds();<br>
+ const void *Record = Closure->Record;<br>
+ uint32_t NumValueKinds = Closure->GetNumValueKinds(Record);<br>
if (NumValueKinds == 0)<br>
return TotalSize;<br>
<br>
- for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {<br>
- uint32_t NumValueSites = Record.getNumValueSites(Kind);<br>
+ for (Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {<br>
+ uint32_t NumValueSites = Closure->GetNumValueSites(Record, Kind);<br>
if (!NumValueSites)<br>
continue;<br>
- TotalSize +=<br>
- getValueProfRecordSize(NumValueSites, Record.getNumValueData(Kind));<br>
+ TotalSize += getValueProfRecordSize(NumValueSites,<br>
+ Closure->GetNumValueData(Record, Kind));<br>
}<br>
return TotalSize;<br>
}<br>
@@ -226,37 +235,95 @@ void ValueProfData::deserializeTo(InstrP<br>
if (NumValueKinds == 0)<br>
return;<br>
<br>
- ValueProfRecord *VR = getFirstValueProfRecord();<br>
+ ValueProfRecord *VR = getFirstValueProfRecord(this);<br>
for (uint32_t K = 0; K < NumValueKinds; K++) {<br>
VR->deserializeTo(Record, VMap);<br>
VR = getValueProfRecordNext(VR);<br>
}<br>
}<br>
<br>
-static std::unique_ptr<ValueProfData> AllocValueProfData(uint32_t TotalSize) {<br>
+static std::unique_ptr<ValueProfData> allocValueProfData(uint32_t TotalSize) {<br>
return std::unique_ptr<ValueProfData>(new (::operator new(TotalSize))<br>
ValueProfData());<br>
}<br>
<br>
-std::unique_ptr<ValueProfData><br>
-ValueProfData::serializeFrom(const InstrProfRecord &Record) {<br>
- uint32_t TotalSize = getSize(Record);<br>
+ValueProfData *serializeValueProfDataFrom(ValueProfRecordClosure *Closure) {<br>
+ uint32_t TotalSize = getValueProfDataSize(Closure);<br>
<br>
- std::unique_ptr<ValueProfData> VPD = AllocValueProfData(TotalSize);<br>
+ ValueProfData *VPD = Closure->AllocValueProfData(TotalSize);<br>
<br>
VPD->TotalSize = TotalSize;<br>
- VPD->NumValueKinds = Record.getNumValueKinds();<br>
- ValueProfRecord *VR = VPD->getFirstValueProfRecord();<br>
+ VPD->NumValueKinds = Closure->GetNumValueKinds(Closure->Record);<br>
+ ValueProfRecord *VR = getFirstValueProfRecord(VPD);<br>
for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {<br>
- uint32_t NumValueSites = Record.getNumValueSites(Kind);<br>
+ uint32_t NumValueSites = Closure->GetNumValueSites(Closure->Record, Kind);<br>
if (!NumValueSites)<br>
continue;<br>
- VR->serializeFrom(Record, Kind, NumValueSites);<br>
+ serializeValueProfRecordFrom(VR, Closure, Kind, NumValueSites);<br>
VR = getValueProfRecordNext(VR);<br>
}<br>
return VPD;<br>
}<br>
<br>
+// C wrappers of InstrProfRecord member functions used in Closure.<br>
+// These C wrappers are used as adaptors so that C++ code can be<br>
+// invoked as callbacks.<br>
+uint32_t getNumValueKindsInstrProf(const void *Record) {<br>
+ return reinterpret_cast<const InstrProfRecord *>(Record)->getNumValueKinds();<br>
+}<br>
+<br>
+uint32_t getNumValueSitesInstrProf(const void *Record, uint32_t VKind) {<br>
+ return reinterpret_cast<const InstrProfRecord *>(Record)<br>
+ ->getNumValueSites(VKind);<br>
+}<br>
+<br>
+uint32_t getNumValueDataInstrProf(const void *Record, uint32_t VKind) {<br>
+ return reinterpret_cast<const InstrProfRecord *>(Record)<br>
+ ->getNumValueData(VKind);<br>
+}<br>
+<br>
+uint32_t getNumValueDataForSiteInstrProf(const void *R, uint32_t VK,<br>
+ uint32_t S) {<br>
+ return reinterpret_cast<const InstrProfRecord *>(R)<br>
+ ->getNumValueDataForSite(VK, S);<br>
+}<br>
+<br>
+void getValueForSiteInstrProf(const void *R, InstrProfValueData *Dst,<br>
+ uint32_t K, uint32_t S,<br>
+ uint64_t (*Mapper)(uint32_t, uint64_t)) {<br>
+ return reinterpret_cast<const InstrProfRecord *>(R)<br>
+ ->getValueForSite(Dst, K, S, Mapper);<br>
+}<br>
+<br>
+ValueProfData *allocValueProfDataInstrProf(size_t TotalSizeInBytes) {<br>
+ return (ValueProfData *)(new (::operator new(TotalSizeInBytes))<br>
+ ValueProfData());<br>
+}<br>
+<br>
+static ValueProfRecordClosure InstrProfRecordClosure = {<br>
+ 0,<br>
+ getNumValueKindsInstrProf,<br>
+ getNumValueSitesInstrProf,<br>
+ getNumValueDataInstrProf,<br>
+ getNumValueDataForSiteInstrProf,<br>
+ stringToHash,<br>
+ getValueForSiteInstrProf,<br>
+ allocValueProfDataInstrProf};<br>
+<br>
+uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {<br>
+ InstrProfRecordClosure.Record = &Record;<br>
+ return getValueProfDataSize(&InstrProfRecordClosure);<br></blockquote><div><br>Am I reading this right that this function is not thread safe (using mutable global state without locking)? For a function called 'getSize(Foo)' that seems quite surprising/dangerous.<br><br>Perhaps this should be:<br><br>static /const/ ValueProfRecordClosure InstrProfRecordClosure<br>...<br><br>uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {<br> auto Closure = InstrProfRecordClosure;<br> Closure.Record = &Record;<br> return getValueProfDataSize(&InstrProfRecordClosure);<br>}<br><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This suggestion sounds fine.</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>Or maybe even better to split the parameters into the compile-time constant part, and the state parameter (& wondering about the name too):<br><br>static const ValueProfRecordVTable IstrProfRecordVTable = ...<br><br>uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {<br> return getValueProfDataSize(&Record, &InstrProfRecordClosure);</div><div>}<br><br></div></div></div></blockquote><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>This is more involved, unless we see clear win with this, I prefer the first suggestion.</div></div></div></div></blockquote><div><br></div><div>Bit more involved - but would it be an improvement? Seems like it might be, in any case (continued at the next comment... )</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div>But my motivation for looking here is actually a bit more involved:<br><br>Wondering if it'd be reasonable to be able to get the record size without ever having an InstrProfRecord (since InstrProfRecord has a bunch of fields that aren't relevant to this operation, I think - so could save memory/processing by not having to construct/provide them).<br><br>If I'm reading this right - this only uses the InstrProfRecord::{IndirectCallSites, MemOPSizes} fields? Given the patch I sent out for reduced memory usage that pulls these two fields into their own struct - perhaps that struct could be expanded a bit - sink the operations that only act on these two fields into the struct, and use that struct as the interface here? (so ValueProfData::getSize takes what's currently called InstrProfRecord::ValueProfData (which, having found this API, I now realize is a bit of a naming collision anyway, so probably best if a new name is picked for this thing, especially if it'll be referred to more broadly)) That way callers don't need the other stuff in an InstrProfRecord (Counts, Name, Hash, Error) to call these APIs.<br> </div></div></div></blockquote><div><br></div><div><br></div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>I made some comments in that review thread.</div></div></div></div></blockquote><div><br>Ah, you were suggesting a nested struct in the InstrProfRecord that holds only the counts/indirectCallSites/memOPSizes, without the name/hash/error (I'd also like to see if the error could be removed - an error member is a bit of an oddity, though not outright unreasonable in all cases) - and that struct (InstrProfRecord::Values? Open to naming ideas) is the one that this API (ValueProfData::getSize and similar) should be defined in terms of, yes? (since they don't need any of the naming fields)<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div><br></div><div>thanks,</div><div><br></div><div>David</div></div></div></div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+}<br>
+<br>
+std::unique_ptr<ValueProfData><br>
+ValueProfData::serializeFrom(const InstrProfRecord &Record) {<br>
+ InstrProfRecordClosure.Record = &Record;<br>
+<br>
+ std::unique_ptr<ValueProfData> VPD(<br>
+ serializeValueProfDataFrom(&InstrProfRecordClosure));<br>
+ return VPD;<br>
+}<br>
+<br>
ErrorOr<std::unique_ptr<ValueProfData>><br>
ValueProfData::getValueProfData(const unsigned char *D,<br>
const unsigned char *const BufferEnd,<br>
@@ -277,14 +344,14 @@ ValueProfData::getValueProfData(const un<br>
if (TotalSize % sizeof(uint64_t))<br>
return instrprof_error::malformed;<br>
<br>
- std::unique_ptr<ValueProfData> VPD = AllocValueProfData(TotalSize);<br>
+ std::unique_ptr<ValueProfData> VPD = allocValueProfData(TotalSize);<br>
<br>
memcpy(VPD.get(), D, TotalSize);<br>
// Byte swap.<br>
VPD->swapBytesToHost(Endianness);<br>
<br>
// Data integrity check:<br>
- ValueProfRecord *VR = VPD->getFirstValueProfRecord();<br>
+ ValueProfRecord *VR = getFirstValueProfRecord(VPD.get());<br>
for (uint32_t K = 0; K < VPD->NumValueKinds; K++) {<br>
if (VR->Kind > IPVK_Last)<br>
return instrprof_error::malformed;<br>
@@ -304,7 +371,7 @@ void ValueProfData::swapBytesToHost(supp<br>
sys::swapByteOrder<uint32_t>(TotalSize);<br>
sys::swapByteOrder<uint32_t>(NumValueKinds);<br>
<br>
- ValueProfRecord *VR = getFirstValueProfRecord();<br>
+ ValueProfRecord *VR = getFirstValueProfRecord(this);<br>
for (uint32_t K = 0; K < NumValueKinds; K++) {<br>
VR->swapBytes(Endianness, getHostEndianness());<br>
VR = getValueProfRecordNext(VR);<br>
@@ -316,7 +383,7 @@ void ValueProfData::swapBytesFromHost(su<br>
if (Endianness == getHostEndianness())<br>
return;<br>
<br>
- ValueProfRecord *VR = getFirstValueProfRecord();<br>
+ ValueProfRecord *VR = getFirstValueProfRecord(this);<br>
for (uint32_t K = 0; K < NumValueKinds; K++) {<br>
ValueProfRecord *NVR = getValueProfRecordNext(VR);<br>
VR->swapBytes(getHostEndianness(), Endianness);<br>
@@ -325,9 +392,4 @@ void ValueProfData::swapBytesFromHost(su<br>
sys::swapByteOrder<uint32_t>(TotalSize);<br>
sys::swapByteOrder<uint32_t>(NumValueKinds);<br>
}<br>
-<br>
-ValueProfRecord *ValueProfData::getFirstValueProfRecord() {<br>
- return reinterpret_cast<ValueProfRecord *>((char *)this +<br>
- sizeof(ValueProfData));<br>
-}<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</blockquote></div></div></div></blockquote></div></div>