[PATCH] D153154: [NFC][Inliner] Introduce per-target savings multiplier and profitable multiplier to use a hybrid of cost-benefit and cost-threshold analysis.

[Kazu Hirata via Phabricator via llvm-commits]

kazu added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:72
   unsigned getInliningThresholdMultiplier() const { return 1; }
+  unsigned getInliningCostBenefitAnalysisSavingsMultiplier() const { return 8; }
+  unsigned getInliningCostBenefitAnalysisProfitableMultiplier() const  { return 8; }
----------------
IIUC, with this patch, we stop honoring the command line option `-inline-savings-multiplier`.  Is there any way you could retain the ability to specify the multiplier as a command line option so that we can easily experiment with different values without rebuilding the compiler?  You could do something like:

```
if (InlineSavingsMultiplier.getNumOccurrences()) {
  // Honor the option.
  ...
} else {
  // Do whatever architecture-specific things we need to do.
  ...
}
```



================
Comment at: llvm/lib/Analysis/InlineCost.cpp:908-912
+    APInt ProfitableThreshold = CycleSavings;
+    ProfitableThreshold *= TTI.getInliningCostBenefitAnalysisProfitableMultiplier();
+
+    if (RHS.ule(ProfitableThreshold))
+      return false;
----------------
IIUC, we are returning `false` if the cost-benefit ratio is below some threshold.  Could you somehow mention that we are comparing the cost-benefit ratio against some threshold as a comment?

```
    // Do not inline if the savings does not justify the cost of inlining.                                                  
    // Specifically, we evaluate the following inequality:                                                                  
    //                                                                                                                      
    //  CycleSavings                PSI->getOrCompHotCountThreshold()                                                       
    // -------------- <= --------------------------------------------------------                                           
    //       Size        TTI.getInliningCostBenefitAnalysisProfitableMultiplier()
```

If you come with some concise language instead of nearly repeating the entire comment block, that's even better.


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

https://reviews.llvm.org/D153154