[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Looks great, thanks! Just some minor comments.



================
Comment at: llvm/include/llvm/InitializePasses.h:198
 void initializeInstrProfilingLegacyPassPass(PassRegistry&);
+void initializeCGProfileLegacyPassPass(PassRegistry&);
 void initializeInstrOrderFileLegacyPassPass(PassRegistry&);
----------------
I think these declarations should be in alphabetic order.


================
Comment at: llvm/include/llvm/Transforms/IPO.h:285
 
+ModulePass *createCGProfileLegacyPass();
 } // End llvm namespace
----------------
ultra nit: I would probably keep the empty line before the } here


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:838
+  // Add Module flag "CG Profile" based on Branch Frequency Information.
+  MPM.add(createCGProfileLegacyPass());
+
----------------
In the new pm this pass seems to be added conditionally based on some flag: if (PTO.CallGraphProfile)
Is it possible to do something similar here?


================
Comment at: llvm/lib/Transforms/Instrumentation/CGProfile.cpp:101
 
-void CGProfilePass::addModuleFlags(
-    Module &M,
-    MapVector<std::pair<Function *, Function *>, uint64_t> &Counts) const {
-  if (Counts.empty())
-    return;
+struct CGProfileLegacyPass : public ModulePass {
+  static char ID;
----------------
MaskRay wrote:
> wrap a struct/class in an unnamed namespace to prevent name collision
Might as well declare it 'final' while you're here.


================
Comment at: llvm/lib/Transforms/Instrumentation/CGProfile.cpp:129
+
+INITIALIZE_PASS(CGProfileLegacyPass, "cgprofile", "Call Graph Profile", false,
+                false)
----------------
Should it be "cg-profile" to match the npm pass name? Arthur has been working on making such pass names equal recently.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83013