[llvm] r254057 - [PGO] Convert InstrProfRecord based serialization methods to use common C methods
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 29 15:05:51 PDT 2017
On Tue, Jun 27, 2017 at 11:46 AM Xinliang David Li <davidxl at google.com>
wrote:
> 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 :)
>
I think of it more as "refactoring"/cleanup while I'm here anyway. I've
done some of that in a prototype patch that refactors the counters into a
sub-struct.
>
>
>
>>
>>>
>>>
>>>> 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 ?
>
Sure - currently I've gone with "CounterHolder" as the type and "Counters"
as the member. But can easily change that - can discuss it on the review
thread for that I'll start shortly.
https://reviews.llvm.org/D34838
>
>
>> 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.
>
Cool cool - done that then.
>
> 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/20170629/e36da508/attachment.html>
More information about the llvm-commits
mailing list