[PATCH] D32783: [PartialInlining] Add frequency based cost analysis

[David Li via Phabricator via llvm-commits]

davidxl marked 3 inline comments as done.
davidxl added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:396
+          continue;
+        OutlinedRegionSize += InlineConstants::InstrCost;
+      }
----------------
eraman wrote:
> This should ideally be in InlineCost. Not all instructions have the same cost (eg. switch). At the least it should be documented here.
Added comment.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:458
+  // Weighted saving is smaller than weighted cost, return false
+  if (EntryFreq * NormWeightedSavings < OutlinedCallFreq * NormWeightedRcost) {
+    ORE.emit(OptimizationRemarkAnalysis(DEBUG_TYPE, "OutliningCallcostTooHigh",
----------------
eraman wrote:
> NonWeightedSavings above only includes the savings due to elimination of call and arg setup. Since inlining also can eliminate instructions in a given context, we are likely undercounting the savings here.
> 
possibly. Since the inlined region is just a set of guarding blocks, the addition benefit is probably not that high.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:701
+  // Branch cost:
+  RCost += NumExitBlocks;
+
----------------
eraman wrote:
> This should be multiplied by InstrCost.
removed the function. Use the actual extracted function to compute instead.


https://reviews.llvm.org/D32783