[llvm] r254057 - [PGO] Convert InstrProfRecord based serialization methods to use common C methods
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 27 11:33:58 PDT 2017
On Tue, Jun 27, 2017 at 10:17 AM Xinliang David Li <davidxl at google.com>
wrote:
> 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.
>
Bit more involved - but would it be an improvement? Seems like it might be,
in any case (continued at the next comment... )
>
>
>
>> 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.
>
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)
- Dave
>
>
> 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/0c4faf81/attachment.html>
More information about the llvm-commits
mailing list