[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 09:35:24 PDT 2017
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);
}
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);
}
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.
> +}
> +
> +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/95934c48/attachment.html>
More information about the llvm-commits
mailing list