[PATCH] D18126: Port InstrProfiling pass to new pass manager

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

LGTM with a minor tweak below. Mostly answer questions below.


================
Comment at: include/llvm/Transforms/InstrProfiling.h:27
@@ +26,3 @@
+/// instrumentation pass.
+struct InstrProfiling : PassInfoMixin<InstrProfiling> {
+public:
----------------
Probably should be 'class' instead of 'struct' at this point.

================
Comment at: include/llvm/Transforms/InstrProfiling.h:38
@@ +37,3 @@
+  Module *M;
+  typedef struct PerFunctionProfileData {
+    uint32_t NumValueSites[IPVK_Last + 1];
----------------
Not relevant for this patch, but the use of a typedef struct is really odd in C++ and should be unnecessary. It'd be good to clean this up.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:34-36
@@ -33,3 +33,5 @@
 
-class InstrProfiling : public ModulePass {
+class InstrProfilingLegacyPass : public ModulePass {
+  InstrProfiling InstrProf;
+
 public:
----------------
No, its just come up in code reviews and examples.

I can try to write some documentation around this.

================
Comment at: lib/Transforms/Instrumentation/InstrProfiling.cpp:34-49
@@ -33,19 +33,18 @@
 
-class InstrProfiling : public ModulePass {
+class InstrProfilingLegacyPass : public ModulePass {
+  InstrProfiling InstrProf;
+
 public:
   static char ID;
-
-  InstrProfiling() : ModulePass(ID) {}
-
-  InstrProfiling(const InstrProfOptions &Options)
-      : ModulePass(ID), Options(Options) {}
-
+  InstrProfilingLegacyPass() : ModulePass(ID), InstrProf() {}
+  InstrProfilingLegacyPass(const InstrProfOptions &Options)
+      : ModulePass(ID), InstrProf(Options) {}
   const char *getPassName() const override {
     return "Frontend instrumentation-based coverage lowering";
   }
 
-  bool runOnModule(Module &M) override;
+  bool runOnModule(Module &M) override { return InstrProf.run(M); }
 
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
   }
----------------
chandlerc wrote:
> No, its just come up in code reviews and examples.
> 
> I can try to write some documentation around this.
This does force the pass's basic implementation to be in a header, but this usually shouldn't be that big of a deal. If there are really serious issues, the standard techniques for separating it back into a source file can be used. But generally, C++ classes should live in headers and have proper interfaces.

Consider how useful it can be to have these have proper interfaces in header files if you ever want to unit test a pass to exercise behavior not easily reproduced or observed in a full integration test. We've had several places already where the new pass manager interfaces allowed us to test code that would otherwise have been extremely challenging.


http://reviews.llvm.org/D18126