[PATCH] D71733: [NFC][InlineCost] Factor cost modeling out of CallAnalyzer traversal.

[Easwaran Raman via Phabricator via llvm-commits]

eraman added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:128
 
-  /// Tunable parameters that control the analysis.
-  const InlineParams &Params;
+  bool SingleBB = true;
 
----------------
This should go to InlineCostCallAnalyzer as it is not needed for legality check or symbolic evaluation.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:133
+  virtual void onBlockAnalyzed(const BasicBlock *BB) {}
+  /// Called at the end of the analysis of the callsite. Return the outcome of
+  /// the analysis, i.e. 'InlineResult(true)' if the inlining may happen, or
----------------
Please leave an empty line before the doc comment to make this more readable (here and other places below)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:473
+        InlineConstants::IndirectCallThreshold;
+    InlineCostCallAnalyzer CA(TTI, GetAssumptionCache, GetBFI, PSI, ORE, F,
+                              Call, IndirectCallParams, false);
----------------
Let's say you are using a different flavor of InlineCostCallAnalyzer (one which  has different implemnetation of the virtual onXYZ methods), you should be instantiating an instance of that class here and call analyze here.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1457
       // This is normally lowered to 4 LLVM instructions.
-      addCost(3 * InlineConstants::InstrCost);
+      onLoadRelativeIntrinsic();
       return false;
----------------
Comment should go to callee (please check all places where you call onXYZ methods if the comment belong in the caller or callee)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1483
 
   if (TTI.isLoweredToCall(F)) {
     // We account for the average 1 instruction per call argument setup here.
----------------
Why not move the entire block into a single function?


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1641
   //   n + n / 2 - 1 = n * 3 / 2 - 1
-  if (NumCaseCluster <= 3) {
-    // Suppose a comparison includes one compare and one conditional branch.
-    addCost(NumCaseCluster * 2 * InlineConstants::InstrCost);
+  if (!onCaseCluster(NumCaseCluster))
     return false;
----------------
Comments should go into the callee.  Also, it's better to merge onCaseCluster and onFinalizeSwitch into one function and just do

onFinalizeSwitch();
return false;

(Revisiting this - I think even onJumpTable should be folded into onFinalizeSwitch. From simplification/legality perspective,  there is notthing left to do here after checking if the condition folds into a constant)


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1737
     else
-      addCost(InlineConstants::InstrCost);
+      onCommonInstruction();
 
----------------
onCommonInstructionSimplification is more meaningful.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1938
   for (unsigned Idx = 0; Idx != BBWorklist.size(); ++Idx) {
     // Bail out the moment we cross the threshold. This means we'll under-count
     // the cost, but only when undercounting doesn't matter.
----------------
The comments are not in-sync with the code. shouldStop could potentially use other ways to check if the analysis should stop - not just cost exceeds the threshold


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2016
   // functions (and hence DT and LI will hopefully be cheap).
   if (Caller->hasMinSize()) {
     DominatorTree DT(F);
----------------
Shouldn't this entire block be moved to InlineCostCallAnalyzer as this isn't performing symbolic evaluation or legality check?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71733