[PATCH] LLVM changes for indirect call target profiling support

David davidxl at google.com
Thu May 14 10:41:15 PDT 2015


================
Comment at: docs/LangRef.rst:8089
@@ +8088,3 @@
+
+The first argument is a pointer to a global variable containing the
+name of the entity being instrumented. This should generally be the
----------------
betulb wrote:
> davidxl wrote:
> > Can entity anything else other than function? If not, make it clear 'name of the function ...'.
> The description for instrprof_increment intrinsic uses the same language, so I did not make the proposed change to preserve the genericity.
ok.

================
Comment at: docs/LangRef.rst:8093
@@ +8092,3 @@
+
+The second argument is a hash value that can be used by the consumer
+of the profile data to detect changes to the instrumented source. It
----------------
betulb wrote:
> davidxl wrote:
> > Why do we need to do this at instruction level? Function level mismatch checking by the consumer should be good enough.
> It's function level. Hash values are used to check changes in the function's source.
ok.

================
Comment at: include/llvm/IR/IntrinsicInst.h:386
@@ +385,3 @@
+
+    GlobalVariable *getName() const {
+      return cast<GlobalVariable>(
----------------
betulb wrote:
> davidxl wrote:
> > Maybe GetFunctionName?
> I tried to stay consistent with instrprof_increment intrinsic, thus this change is not implemented. I'll revisit, if insistent.
ok.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:149
@@ +148,3 @@
+    struct ProfileDataV1 V1Data;
+    const IntPtrT FunctionPointer;
+    const uint32_t NumIndirectCallSites;
----------------
betulb wrote:
> davidxl wrote:
> > Changing ProfileData structure like this will eventually lead to reader code to be come really messy and unmanageable. 
> > 
> > This is a simple way to avoid this change -- simply 'peel' the value profile related per-function data into a different data section. It will make adding new value profilers in the future much simpler.
> I agree. I tried to derive another class using this reader class as a base. Thus, made sure that the code for the RawInstrProfReader is not changed. However, the peeling and moving value data into its own section had one issue i.e. reliance on the linker to make sure the ith elements of ProfileData and ValueData elements correspond to the same instrumented function in the same order.
> 
> We read and process the raw profile data in iteration of ProfileData elements. I was not sure if the elements of ProfileData and ValueData sections would always correlate to one another.
The linking order is indeed a problem. Your new patch makes this moot.

================
Comment at: include/llvm/ProfileData/InstrProfReader.h:162
@@ -161,2 +161,3 @@
   };
-
+  struct RawHeaderV2 {
+    struct RawHeaderV1 V1Header;
----------------
betulb wrote:
> davidxl wrote:
> > Similarly, instead of changing the common header, simply split out the value profiler related control info into subheaders, so that the file layout looks like this:
> > 
> >     RawHeader
> >     ValueProfilerHeader1
> >     ValueProfilerHeader2
> > 
> > The version number in the main raw header indicates the presence or absense of the value profiler header in the file, and can decide to read it or skip it.
> > 
> > By so doing, the header reading code can be shared between different versions.
> > 
> > I strongly suggest you change like this ..
> > 
> In the current runtime, the value header gives the combined value data size for all value kinds. The order w/in the raw profile data file looks as follows:
> 
> The value data entries for value kinds [0-ValueKindLast] for Function 1
> The value data entries for value kinds [0-ValueKindLast] for Function 2  ...
> The value data entries for value kinds [0-ValueKindLast] for Function N
> 
> Header gives the combined value data size for all value kinds for all functions. The profile data file and all readers/writers are future proofed as they're already set up to output/read multiple value kinds.
ok.

http://reviews.llvm.org/D8908

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list