[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 a comment.

Thanks Rong.
Let me know what you think.



================
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:
> 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.


================
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:
> 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.


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

https://reviews.llvm.org/D67920