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

Sorry it takes long to review this. 
I have a pending patch that value profiling the expensive MOD/REM instructions.  It is more tightly coupled to edge profile (than current mem-size-op and icall value profile).
The difference is  that it does not have <valuesite, value> pair.  The counts are stored like the edge counts.
It seems to be fine with patch as the instrumentation says in PGOInstrumenation.cpp and only the candidate collecting is extracted out.

Overall, I'm fine with this refactor/improvement.



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


================
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.
+  };
----------------
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.


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

https://reviews.llvm.org/D67920