[llvm] r254008 - [PGO] Introduce value profile data closure type.

[Sean Silva via llvm-commits]

On Tue, Nov 24, 2015 at 11:21 AM, Xinliang David Li via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: davidxl
> Date: Tue Nov 24 13:21:15 2015
> New Revision: 254008
>
> URL: http://llvm.org/viewvc/llvm-project?rev=254008&view=rev
> Log:
> [PGO] Introduce value profile data closure type.
>
> The closure is designed to abstact away two types of value profile
> data:
>   - InstrProfRecord which is the primary data structure used to
>     represent profile data in host tools (reader, writer, and profile-use)
>   - value profile runtime data structure suitable to be used by C
>     runtime library.
> Both sources of data need to serialize to disk/memory-buffer in common
> format: ValueProfData.
>
> The abstraction allows compiler-rt's raw profiler writer to share
> the same code with indexed profile writer.
>
>
> Modified:
>     llvm/trunk/include/llvm/ProfileData/InstrProf.h
>
> Modified: llvm/trunk/include/llvm/ProfileData/InstrProf.h
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ProfileData/InstrProf.h?rev=254008&r1=254007&r2=254008&view=diff
>
> ==============================================================================
> --- llvm/trunk/include/llvm/ProfileData/InstrProf.h (original)
> +++ llvm/trunk/include/llvm/ProfileData/InstrProf.h Tue Nov 24 13:21:15
> 2015
> @@ -538,6 +538,18 @@ typedef struct ValueProfData {
>    ValueProfRecord *getFirstValueProfRecord();
>  } ValueProfData;
>
> +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);
>

Breaking a line here is preferable to using inconsistent names.


> +  uint64_t (*RemapValueData)(uint64_t Value);
> +  void (*GetValueForSite)(InstrProfValueData Dst[], void *R, uint32_t K,
> +                          uint32_t S);
>

The actual meaning of `[]` here is unintuitive (bad aspect of C language
design). Just use `*` to avoid confusion.


> +  ValueProfData *(*AllocateValueProfData)(size_t TotalSizeInBytes);
> +} ValueProfRecordClosure;
> +
>

Also I would suggest adding documentation to each of these function
pointers. You are essentially declaring an abstract base class. It should
document it as such (or reference other documentation from which it is
"obvious" just based on the names, but I don't know if we have such other
documentation).

-- Sean Silva


>  inline uint32_t getValueProfRecordHeaderSize(uint32_t NumValueSites) {
>    uint32_t Size = offsetof(ValueProfRecord, SiteCountArray) +
>                    sizeof(uint8_t) * NumValueSites;
>
>
> _______________________________________________
> 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/20151124/aadf3ef6/attachment.html>