[PATCH] D79485: [BPI] Improve static heuristics for "cold" paths.

[John Brawn via Phabricator via llvm-commits]

john.brawn added a comment.

It look like there have been recent changes to BranchProbabilityInfo that mean this patch needs to be rebased.



================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:146
 
+  enum SccBlockType {
+    Inner = 0x0,
----------------
Could do with a comment here explaining what these types mean - in particular the value type of SccBlockTypeMap is a bitmask of SccBlockType, so explaining that a block can be of more than one type would be good.


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:197
+  const LoopInfo *LI = nullptr;
+  /// TODO:
+  SccInfo SccI;
----------------
What's TODO here (and similar for the TODOs below)?


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:206
 
-  void computePostDominatedByUnreachable(const Function &F,
-                                         PostDominatorTree *PDT);
-  void computePostDominatedByColdCall(const Function &F,
-                                      PostDominatorTree *PDT);
-  bool calcUnreachableHeuristics(const BasicBlock *BB);
+  int getSCCNum(const BasicBlock *BB) const;
+  uint32_t getSccBlockInfo(const BasicBlock *BB, int SccNum) const;
----------------
Would it be possible to move all of these is/getSCC functions into SccInfo, to avoid cluttering up BranchProbabilityInfo?


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:207
+  int getSCCNum(const BasicBlock *BB) const;
+  uint32_t getSccBlockInfo(const BasicBlock *BB, int SccNum) const;
+  void setSccBlockInfo(const BasicBlock *BB, int SccNum);
----------------
Should this be getSccBlockType?


================
Comment at: llvm/include/llvm/Analysis/BranchProbabilityInfo.h:208
+  uint32_t getSccBlockInfo(const BasicBlock *BB, int SccNum) const;
+  void setSccBlockInfo(const BasicBlock *BB, int SccNum);
+  bool isSCCHeader(const BasicBlock *BB, int SccNum) const {
----------------
Probably calculateSccBlockInfo would be better here.


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:806
+  // Changed to 'true' if at least one successor has estimated weight.
+  bool FoundEstimatedWeigth = false;
+  SmallVector<uint32_t, 4> SuccWeights;
----------------
Weight, not Weigth


================
Comment at: llvm/lib/Analysis/BranchProbabilityInfo.cpp:820
+      Weight = Weight.getValueOr(DEFAULT_WEIGHT) / TC;
+      if (Weight.getValue() == ZERO_WEIGHT)
+        Weight = VERY_LOW_WEIGHT;
----------------
This is making sure that the divide isn't rounding down to zero, yes? In which case I don't think it'll correctly handle the case where getEstimatedEdgeWeight returns ZERO_WEIGHT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79485