[PATCH] D69331: [LegacyPM] Port CGProfile pass

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D69331#1718885 <https://reviews.llvm.org/D69331#1718885>, @evgeny777 wrote:

> Thanks for looking at it, Teresa!
>
> Here is a llvm-dev post, suggesting to use new PM by default:
>  http://lists.llvm.org/pipermail/llvm-dev/2017-October/118280.html
>
> As you may have noticed it was two years ago and new PM is still "brand new" :)
>  Also, AFAIK, codegen passes still use old PM, so I guess it will be around for quite a while.
>  Until new PM is adopted as default by LLVM community, switching to it is not an option for us, unfortunately.
>  And I need just this pass (it does pretty good job optimizing PGO instrumented images) and don't have plans to port anything else.


Ok, still suggest trying to move to new PM proactively to make sure it works for you, but it's fine with me to back port this. A few comments below.



================
Comment at: include/llvm/LinkAllPasses.h:87
       (void) llvm::createCFLSteensAAWrapperPass();
+      (void)llvm::createCGProfilePass();
       (void) llvm::createStructurizeCFGPass();
----------------
Formatting off


================
Comment at: lib/Transforms/IPO/PassManagerBuilder.cpp:781
+  // Create CG profile metadata if appropriate
+  MPM.add(createCGProfilePass());
+
----------------
I notice that in the new PM this is added after the OptimizerLast EPs are added.  Not sure if that is intentional - should the new and old PMs be consistent here?


================
Comment at: lib/Transforms/Instrumentation/CGProfile.cpp:30
   InstrProfSymtab Symtab;
+  MapVector<std::pair<Function *, Function *>, uint64_t> Counts;
   auto UpdateCounts = [&](TargetTransformInfo &TTI, Function *F,
----------------
Nit: Suggest moving this declaration to before Symtab which puts it in the same relative order as before and should avoid a spurious diff here.


================
Comment at: test/Other/opt-O2-pipeline.ll:290
+; CHECK-NEXT:     CG profile metadata generator
+; CHECK-NEXT:       Unnamed pass: implement Pass::getPassName()
+; CHECK-NEXT:     FunctionPass Manager
----------------
Will this unnamed pass issue be fixed by D69315?


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

https://reviews.llvm.org/D69331