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

[wael yehia via Phabricator via llvm-commits]

w2yehia marked 2 inline comments as done.
w2yehia 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
----------------
xur wrote:
> 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"
> 
> 
Yes. the "at the instrumented instructions" addition is necessary to make the statement more precise. 
Actually, I wrote this whole paragraph to point out a subtle point that I missed myself when working on value profiling, and that is the program location where the value of an expression is recorded matters.


================
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.
+  };
----------------
xur wrote:
> 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?
I assume you are talking about the ValueProfileCollector when you say "the collect". 
The ValueProfileCollector is used in both the PGOInstrumentationGen and PGOInstrumentationUse phases.
However, it is true that only the `Value *V` and `Instruction *InsertPt` fields are used in the PGOInstrumentationGen phase, while only the `Instruction *AnnotatedInstr` field is used in the PGOInstrumentationUse phase.

The reason all three members are in one struct rather than in two structs (one for each phase) is that it was natural to generate all pieces of information about a particular candidate in one place in the code, and then have each phase use what it needs from it.


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

https://reviews.llvm.org/D67920