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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:473
+        InlineConstants::IndirectCallThreshold;
+    InlineCostCallAnalyzer CA(TTI, GetAssumptionCache, GetBFI, PSI, ORE, F,
+                              Call, IndirectCallParams, false);
----------------
eraman wrote:
> 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.
Maybe, but I'd prefer addressing that problem when the scenario arises, and avoid the added complexity meanwhile, wdyt?


================
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.
----------------
eraman wrote:
> 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
True, and in this case, the code is kind of self-evident. I moved the comment to the shouldStop implementation.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:2016
   // functions (and hence DT and LI will hopefully be cheap).
   if (Caller->hasMinSize()) {
     DominatorTree DT(F);
----------------
eraman wrote:
> Shouldn't this entire block be moved to InlineCostCallAnalyzer as this isn't performing symbolic evaluation or legality check?
Possibly, but all that prep work - getting a DominatorTree, LoopInfo, going over loops, making sure we ignore those that aren't executed - would be necessary if we want to account the number of loops somehow. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71733