[llvm] r254057 - [PGO] Convert InstrProfRecord based serialization methods to use common C methods
Xinliang David Li via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 27 10:17:12 PDT 2017
On Tue, Jun 27, 2017 at 9:35 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Nov 24, 2015 at 10:26 PM Xinliang David Li via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: davidxl
>> Date: Wed Nov 25 00:23:38 2015
>> New Revision: 254057
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=254057&view=rev
>> Log:
>> [PGO] Convert InstrProfRecord based serialization methods to use common C
>> methods
>>
>> 1. Convert serialization methods using InstrProfRecord as source into C
>> (impl)
>> interfaces using Closure.
>> 2. Reimplement InstrProfRecord serialization method to use new C interface
>> as dummy wrapper.
>>
>> Now it is ready to implement wrapper for runtime value profile data.
>>
>> (The new code need better source location -- but not changed in this
>> patch to
>> minimize diffs. )
>>
>>
>> Modified:
>> llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> llvm/trunk/lib/ProfileData/InstrProf.cpp
>>
>> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/
>> llvm/ProfileData/InstrProf.h?rev=254057&r1=254056&r2=254057&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
>> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Wed Nov 25 00:23:38
>> 2015
>> @@ -474,9 +474,6 @@ typedef struct ValueProfRecord {
>> /// Read data from this record and save it to Record.
>> void deserializeTo(InstrProfRecord &Record,
>> InstrProfRecord::ValueMapType *VMap);
>> - /// Extract data from \c Record and serialize into this instance.
>> - void serializeFrom(const InstrProfRecord &Record, uint32_t ValueKind,
>> - uint32_t NumValueSites);
>> /// In-place byte swap:
>> /// Do byte swap for this instance. \c Old is the original order before
>> /// the swap, and \c New is the New byte order.
>> @@ -524,8 +521,6 @@ typedef struct ValueProfData {
>> /// Read data from this data and save it to \c Record.
>> void deserializeTo(InstrProfRecord &Record,
>> InstrProfRecord::ValueMapType *VMap);
>> - /// Return the first \c ValueProfRecord instance.
>> - ValueProfRecord *getFirstValueProfRecord();
>> } ValueProfData;
>>
>> /* The closure is designed to abstact away two types of value profile
>> data:
>> @@ -542,21 +537,20 @@ typedef struct ValueProfData {
>> * in class InstrProfRecord.
>> */
>> typedef struct ValueProfRecordClosure {
>> - void *Record;
>> - uint32_t (*GetNumValueKinds)(void *Record);
>> - uint32_t (*GetNumValueSites)(void *Record, uint32_t VKind);
>> - uint32_t (*GetNumValueData)(void *Record, uint32_t VKind);
>> - uint32_t (*GetNumValueDataForSite)(void *R, uint32_t VK, uint32_t S);
>> + const void *Record;
>> + uint32_t (*GetNumValueKinds)(const void *Record);
>> + uint32_t (*GetNumValueSites)(const void *Record, uint32_t VKind);
>> + uint32_t (*GetNumValueData)(const void *Record, uint32_t VKind);
>> + uint32_t (*GetNumValueDataForSite)(const void *R, uint32_t VK,
>> uint32_t S);
>>
>> /* After extracting the value profile data from the value profile
>> record,
>> * this method is used to map the in-memory value to on-disk value. If
>> * the method is null, value will be written out untranslated.
>> */
>> uint64_t (*RemapValueData)(uint32_t, uint64_t Value);
>> - void (*GetValueForSite)(InstrProfValueData *Dst, void *R, uint32_t K,
>> + void (*GetValueForSite)(const void *R, InstrProfValueData *Dst,
>> uint32_t K,
>> uint32_t S, uint64_t (*Mapper)(uint32_t,
>> uint64_t));
>> -
>> - ValueProfData *(*AllocateValueProfData)(size_t TotalSizeInBytes);
>> + ValueProfData *(*AllocValueProfData)(size_t TotalSizeInBytes);
>> } ValueProfRecordClosure;
>>
>> /// Return the \c ValueProfRecord header size including the padding
>> bytes.
>> @@ -599,6 +593,11 @@ inline ValueProfRecord *getValueProfReco
>> NumValueData));
>> }
>>
>> +/// Return the first \c ValueProfRecord instance.
>> +inline ValueProfRecord *getFirstValueProfRecord(ValueProfData *This) {
>> + return (ValueProfRecord *)((char *)This + sizeof(ValueProfData));
>> +}
>> +
>> namespace IndexedInstrProf {
>>
>> enum class HashT : uint32_t {
>>
>> Modified: llvm/trunk/lib/ProfileData/InstrProf.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
>> ProfileData/InstrProf.cpp?rev=254057&r1=254056&r2=254057&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/ProfileData/InstrProf.cpp (original)
>> +++ llvm/trunk/lib/ProfileData/InstrProf.cpp Wed Nov 25 00:23:38 2015
>> @@ -155,16 +155,21 @@ void ValueProfRecord::deserializeTo(Inst
>> }
>> }
>>
>> -void ValueProfRecord::serializeFrom(const InstrProfRecord &Record,
>> - uint32_t ValueKind,
>> - uint32_t NumValueSites) {
>> - Kind = ValueKind;
>> - this->NumValueSites = NumValueSites;
>> - InstrProfValueData *DstVD = getValueProfRecordValueData(this);
>> - for (uint32_t S = 0; S < NumValueSites; S++) {
>> - uint32_t ND = Record.getNumValueDataForSite(ValueKind, S);
>> - SiteCountArray[S] = ND;
>> - Record.getValueForSite(DstVD, ValueKind, S, stringToHash);
>> +// Extract data from \c Closure and serialize into \c This instance.
>> +void serializeValueProfRecordFrom(ValueProfRecord *This,
>> + ValueProfRecordClosure *Closure,
>> + uint32_t ValueKind, uint32_t
>> NumValueSites) {
>> + uint32_t S;
>> + const void *Record = Closure->Record;
>> + This->Kind = ValueKind;
>> + This->NumValueSites = NumValueSites;
>> + InstrProfValueData *DstVD = getValueProfRecordValueData(This);
>> +
>> + for (S = 0; S < NumValueSites; S++) {
>> + uint32_t ND = Closure->GetNumValueDataForSite(Record, ValueKind, S);
>> + This->SiteCountArray[S] = ND;
>> + Closure->GetValueForSite(Record, DstVD, ValueKind, S,
>> + Closure->RemapValueData);
>> DstVD += ND;
>> }
>> }
>> @@ -205,18 +210,22 @@ void ValueProfRecord::swapBytes(support:
>> }
>> }
>>
>> -uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {
>> +/// Return the total size in bytes of the on-disk value profile data
>> +/// given the data stored in Record.
>> +uint32_t getValueProfDataSize(ValueProfRecordClosure *Closure) {
>> + uint32_t Kind;
>> uint32_t TotalSize = sizeof(ValueProfData);
>> - uint32_t NumValueKinds = Record.getNumValueKinds();
>> + const void *Record = Closure->Record;
>> + uint32_t NumValueKinds = Closure->GetNumValueKinds(Record);
>> if (NumValueKinds == 0)
>> return TotalSize;
>>
>> - for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {
>> - uint32_t NumValueSites = Record.getNumValueSites(Kind);
>> + for (Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {
>> + uint32_t NumValueSites = Closure->GetNumValueSites(Record, Kind);
>> if (!NumValueSites)
>> continue;
>> - TotalSize +=
>> - getValueProfRecordSize(NumValueSites,
>> Record.getNumValueData(Kind));
>> + TotalSize += getValueProfRecordSize(NumValueSites,
>> + Closure->GetNumValueData(Record,
>> Kind));
>> }
>> return TotalSize;
>> }
>> @@ -226,37 +235,95 @@ void ValueProfData::deserializeTo(InstrP
>> if (NumValueKinds == 0)
>> return;
>>
>> - ValueProfRecord *VR = getFirstValueProfRecord();
>> + ValueProfRecord *VR = getFirstValueProfRecord(this);
>> for (uint32_t K = 0; K < NumValueKinds; K++) {
>> VR->deserializeTo(Record, VMap);
>> VR = getValueProfRecordNext(VR);
>> }
>> }
>>
>> -static std::unique_ptr<ValueProfData> AllocValueProfData(uint32_t
>> TotalSize) {
>> +static std::unique_ptr<ValueProfData> allocValueProfData(uint32_t
>> TotalSize) {
>> return std::unique_ptr<ValueProfData>(new (::operator new(TotalSize))
>> ValueProfData());
>> }
>>
>> -std::unique_ptr<ValueProfData>
>> -ValueProfData::serializeFrom(const InstrProfRecord &Record) {
>> - uint32_t TotalSize = getSize(Record);
>> +ValueProfData *serializeValueProfDataFrom(ValueProfRecordClosure
>> *Closure) {
>> + uint32_t TotalSize = getValueProfDataSize(Closure);
>>
>> - std::unique_ptr<ValueProfData> VPD = AllocValueProfData(TotalSize);
>> + ValueProfData *VPD = Closure->AllocValueProfData(TotalSize);
>>
>> VPD->TotalSize = TotalSize;
>> - VPD->NumValueKinds = Record.getNumValueKinds();
>> - ValueProfRecord *VR = VPD->getFirstValueProfRecord();
>> + VPD->NumValueKinds = Closure->GetNumValueKinds(Closure->Record);
>> + ValueProfRecord *VR = getFirstValueProfRecord(VPD);
>> for (uint32_t Kind = IPVK_First; Kind <= IPVK_Last; Kind++) {
>> - uint32_t NumValueSites = Record.getNumValueSites(Kind);
>> + uint32_t NumValueSites = Closure->GetNumValueSites(Closure->Record,
>> Kind);
>> if (!NumValueSites)
>> continue;
>> - VR->serializeFrom(Record, Kind, NumValueSites);
>> + serializeValueProfRecordFrom(VR, Closure, Kind, NumValueSites);
>> VR = getValueProfRecordNext(VR);
>> }
>> return VPD;
>> }
>>
>> +// C wrappers of InstrProfRecord member functions used in Closure.
>> +// These C wrappers are used as adaptors so that C++ code can be
>> +// invoked as callbacks.
>> +uint32_t getNumValueKindsInstrProf(const void *Record) {
>> + return reinterpret_cast<const InstrProfRecord
>> *>(Record)->getNumValueKinds();
>> +}
>> +
>> +uint32_t getNumValueSitesInstrProf(const void *Record, uint32_t VKind) {
>> + return reinterpret_cast<const InstrProfRecord *>(Record)
>> + ->getNumValueSites(VKind);
>> +}
>> +
>> +uint32_t getNumValueDataInstrProf(const void *Record, uint32_t VKind) {
>> + return reinterpret_cast<const InstrProfRecord *>(Record)
>> + ->getNumValueData(VKind);
>> +}
>> +
>> +uint32_t getNumValueDataForSiteInstrProf(const void *R, uint32_t VK,
>> + uint32_t S) {
>> + return reinterpret_cast<const InstrProfRecord *>(R)
>> + ->getNumValueDataForSite(VK, S);
>> +}
>> +
>> +void getValueForSiteInstrProf(const void *R, InstrProfValueData *Dst,
>> + uint32_t K, uint32_t S,
>> + uint64_t (*Mapper)(uint32_t, uint64_t)) {
>> + return reinterpret_cast<const InstrProfRecord *>(R)
>> + ->getValueForSite(Dst, K, S, Mapper);
>> +}
>> +
>> +ValueProfData *allocValueProfDataInstrProf(size_t TotalSizeInBytes) {
>> + return (ValueProfData *)(new (::operator new(TotalSizeInBytes))
>> + ValueProfData());
>> +}
>> +
>> +static ValueProfRecordClosure InstrProfRecordClosure = {
>> + 0,
>> + getNumValueKindsInstrProf,
>> + getNumValueSitesInstrProf,
>> + getNumValueDataInstrProf,
>> + getNumValueDataForSiteInstrProf,
>> + stringToHash,
>> + getValueForSiteInstrProf,
>> + allocValueProfDataInstrProf};
>> +
>> +uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {
>> + InstrProfRecordClosure.Record = &Record;
>> + return getValueProfDataSize(&InstrProfRecordClosure);
>>
>
> 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.
>
> Perhaps this should be:
>
> static /const/ ValueProfRecordClosure InstrProfRecordClosure
> ...
>
> uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {
> auto Closure = InstrProfRecordClosure;
> Closure.Record = &Record;
> return getValueProfDataSize(&InstrProfRecordClosure);
> }
>
>
This suggestion sounds fine.
> Or maybe even better to split the parameters into the compile-time
> constant part, and the state parameter (& wondering about the name too):
>
> static const ValueProfRecordVTable IstrProfRecordVTable = ...
>
> uint32_t ValueProfData::getSize(const InstrProfRecord &Record) {
> return getValueProfDataSize(&Record, &InstrProfRecordClosure);
> }
>
>
This is more involved, unless we see clear win with this, I prefer the
first suggestion.
> But my motivation for looking here is actually a bit more involved:
>
> 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).
>
> 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.
>
>
I made some comments in that review thread.
thanks,
David
> +}
>> +
>> +std::unique_ptr<ValueProfData>
>> +ValueProfData::serializeFrom(const InstrProfRecord &Record) {
>> + InstrProfRecordClosure.Record = &Record;
>> +
>> + std::unique_ptr<ValueProfData> VPD(
>> + serializeValueProfDataFrom(&InstrProfRecordClosure));
>> + return VPD;
>> +}
>> +
>> ErrorOr<std::unique_ptr<ValueProfData>>
>> ValueProfData::getValueProfData(const unsigned char *D,
>> const unsigned char *const BufferEnd,
>> @@ -277,14 +344,14 @@ ValueProfData::getValueProfData(const un
>> if (TotalSize % sizeof(uint64_t))
>> return instrprof_error::malformed;
>>
>> - std::unique_ptr<ValueProfData> VPD = AllocValueProfData(TotalSize);
>> + std::unique_ptr<ValueProfData> VPD = allocValueProfData(TotalSize);
>>
>> memcpy(VPD.get(), D, TotalSize);
>> // Byte swap.
>> VPD->swapBytesToHost(Endianness);
>>
>> // Data integrity check:
>> - ValueProfRecord *VR = VPD->getFirstValueProfRecord();
>> + ValueProfRecord *VR = getFirstValueProfRecord(VPD.get());
>> for (uint32_t K = 0; K < VPD->NumValueKinds; K++) {
>> if (VR->Kind > IPVK_Last)
>> return instrprof_error::malformed;
>> @@ -304,7 +371,7 @@ void ValueProfData::swapBytesToHost(supp
>> sys::swapByteOrder<uint32_t>(TotalSize);
>> sys::swapByteOrder<uint32_t>(NumValueKinds);
>>
>> - ValueProfRecord *VR = getFirstValueProfRecord();
>> + ValueProfRecord *VR = getFirstValueProfRecord(this);
>> for (uint32_t K = 0; K < NumValueKinds; K++) {
>> VR->swapBytes(Endianness, getHostEndianness());
>> VR = getValueProfRecordNext(VR);
>> @@ -316,7 +383,7 @@ void ValueProfData::swapBytesFromHost(su
>> if (Endianness == getHostEndianness())
>> return;
>>
>> - ValueProfRecord *VR = getFirstValueProfRecord();
>> + ValueProfRecord *VR = getFirstValueProfRecord(this);
>> for (uint32_t K = 0; K < NumValueKinds; K++) {
>> ValueProfRecord *NVR = getValueProfRecordNext(VR);
>> VR->swapBytes(getHostEndianness(), Endianness);
>> @@ -325,9 +392,4 @@ void ValueProfData::swapBytesFromHost(su
>> sys::swapByteOrder<uint32_t>(TotalSize);
>> sys::swapByteOrder<uint32_t>(NumValueKinds);
>> }
>> -
>> -ValueProfRecord *ValueProfData::getFirstValueProfRecord() {
>> - return reinterpret_cast<ValueProfRecord *>((char *)this +
>> - sizeof(ValueProfData));
>> -}
>> }
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170627/2dfe6286/attachment.html>
More information about the llvm-commits
mailing list