[PATCH] D32666: [PartialInlining] Add support in partial inliner to use cost analysis in inliner

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

LGTM with a few minor comments.



================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:80
+      std::function<TargetTransformInfo &(Function &)> *GTTI,
+      Optional<function_ref<BlockFrequencyInfo &(Function &)>> GBFI,
+      ProfileSummaryInfo *ProfSI)
----------------
I know this is how it is in InlineCost.cpp, but still: why not use function_ref everywhere?


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:302
+
+  if (IC.isAlways()) {
+    ORE.emit(OptimizationRemarkAnalysis(DEBUG_TYPE, "AlwaysInline", Call)
----------------
This is fine, but perhaps you can filter out functions with always_inline attribute earlier and not do the analysis at all. That can be done in a separate patch. Similarly for noinline as well.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:475
 
   ++NumPartialInlined;
 
----------------
This should also be moved into the loop above (and the description of the statistic changed to indicate that it is the number of callsites)


https://reviews.llvm.org/D32666