[PATCH] D67920: [PGO] Refactor Value Profiling into a plugin based oracle and create a well defined API for the plugins.

[Rong Xu via Phabricator via llvm-commits]

xur added inline comments.


================
Comment at: llvm/lib/Transforms/Instrumentation/ValueProfileCollector.h:49
+///     total=15; (value,freq) set = {(8,10), (4,5)}
+///
+/// For this reason, a plugin is responsible for computing the insertion point
----------------
w2yehia wrote:
> xur wrote:
> > Sorry, I don't follow this paragraph. What is 'total" here, the BB count for "if ...", or  the total count for memcpy"?
> > 
> > We do need a match for the time of instrumentation and use.
> Here I'm trying to demonstrate to the reader that at different points in a user program the value profile info that can be potentially collected for a particular expression are not the same. 
> I'm using the term 'total' in the same sense as the 3rd operand (also called "total" I think) of a value profile metadata node.
> So, `total=100` is the total number of data points (i.e. values) that `nn` took when measured at the beginning of the function foo;
> and `total=15` is the total number of data pointer that `nn` took when measured right before executing the memcpy (so inside the if branch).
> 
> I understand that the IR has to match at the time of instrumentation and the time of use.
OK. The profiled value should reflect the value being profiled (the InsertPt instruction).
The user should be responsible for that. For that case, if one instruments memcpy(...) , they should get total=15 etc.
If they instrument "if (b)", they should get total=100.

The use part should know do different things based on what is instrumented.  I think you really meant something like
"Value profiling an expression means to track the values that this expression
  takes at runtime and the frequency of each value at the instrumented instructions"




================
Comment at: llvm/lib/Transforms/Instrumentation/ValueProfileCollector.h:58
+    Instruction *InsertPt;      // Insert the VP lib call before this instr.
+    Instruction *AnnotatedInst; // Where metadata is attached.
+  };
----------------
w2yehia wrote:
> xur wrote:
> > For value profile, we usually find the target instruction and instrument right before it. The annotation will be on that instruction. 
> > So here InsertPT and AnnotatedInstr should be always the same.
> > I don't think of a need to differentiating this two.
> It is true that for the current two plugins (memop and ICP) the Insertion point and Annotated Instruction are the same.
> But for something like loop trip count profiling (something we're experimenting with), we found it necessary to have separate instruction pointers. 
> To be more exact, the loop trip count value profile MD gets stored on the branch instruction of the loop latch block, but the insertion point is somewhere in the loop pre-header.
OK.  But in theory, the collect does not need to know the annotaedInstr as it will not be used in instrumentation.  Right?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67920/new/

https://reviews.llvm.org/D67920