[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 11:46:52 PDT 2017
On Tue, Jun 27, 2017 at 11:33 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> 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... )
>
>
It might improve a little in terms of writer time, but needs to be
measurable -- otherwise we are doing busy work for nothing :)
>
>>
>>
>>> 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)
>
Perhaps InstrProfRecord::Counters ?
> 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)
>
>
Right.
David
> - 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/761e0506/attachment.html>
More information about the llvm-commits
mailing list