[PATCH] D78987: [BPI][NFC] Reuse post dominantor tree from analysis manager when available

[Evgeniy via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:139
   void calculate(const Function &F, const LoopInfo &LI,
-                 const TargetLibraryInfo *TLI = nullptr);
+                 const TargetLibraryInfo *TLI, PostDominatorTree *PDT);
 
----------------
skatkov wrote:
> why not nullptr by default?
In general, default initialization is considered bad programming practice and should avoided in most cases. That doesn't mean it should never be used but this case doesn't qualify for this. In this case "nullptr" is bad default because analysis results will have lower accuracy than could. We should not "encouraging" callers to omit this argument.


================
Comment at: llvm/lib/Analysis/OptimizationRemarkEmitter.cpp:40
   BranchProbabilityInfo BPI;
-  BPI.calculate(*F, LI);
+  BPI.calculate(*F, LI, nullptr, nullptr);
 
----------------
skatkov wrote:
> If nullptr by default above, no need in this change...
Let me do a trick here. Instead of calling calculate explicitly we can pass required arguments to the constructor directly. Note that I haven't removed defaults from the constructor (at least for now). In fact the only reason stopping me from doing that is number of affected places (which is way more than for 'calculate').


================
Comment at: llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp:1774
   BranchProbabilityInfo BPI;
-  BPI.calculate(F, LI);
+  BPI.calculate(F, LI, &TLI, nullptr);
   InductiveRangeCheckElimination IRCE(SE, &BPI, DT, LI);
----------------
skatkov wrote:
> why nullptr instead of request Pass manager for analysis if it is created anyway?
Even though current implementation of BPI.calculate uses post dominator tree unconditionally we can potentially switch to conditional use. In particular in presence of profile information on branches we give it a priority and don't actually need post dominator tree to compute probabilities. Yes, currently there is one exception from this rule but this is discussable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78987