[PATCH] D35804: [BPI] Detect branches in loops that make themselves not taken

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:428
 
+  // Sometimes in a loop we have a branch whose condition is made false by
+  // taking it. This is typically something like
----------------
Can you extract the new logic into a separate function : computeUnlikelySuccs Or some other name?


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:445
+  std::set<const BasicBlock*> UnlikelyBlocks;
+  if (BI && BI->isConditional()) {
+    // Check if the branch is based on an instruction compared with a constant
----------------
If the logic is extracted to a helper function, do a early return here to make it clearer.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:448
+    CmpInst *CI = dyn_cast<CmpInst>(BI->getCondition());
+    if (CI && isa<Instruction>(CI->getOperand(0)) &&
+        isa<Constant>(CI->getOperand(1))) {
----------------
Early return if condition is not met.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:458
+      std::list<BinaryOperator*> InstChain;
+      while (!CmpPHI && CmpLHS && isa<BinaryOperator>(CmpLHS) &&
+             isa<Constant>(CmpLHS->getOperand(1))) {
----------------
limit the walk within the enclosing loop?


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:465
+      }
+      // Trace the phi node to find all values that come from successors of BB
+      SmallPtrSet<PHINode*, 8> VisitedInsts;
----------------
early return if there is no phi found?


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:483
+          Value *V = P->getIncomingValueForBlock(B);
+          if (isa<Constant>(V) &&
+              std::find(succ_begin(BB), succ_end(BB), B) != succ_end(BB)) {
----------------
continue if not const


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:488
+            for (Instruction *I : InstChain) {
+              if (!CmpLHSConst)
+                break;
----------------
move this down. The first one should always be non-null.


================
Comment at: lib/Analysis/BranchProbabilityInfo.cpp:502
+              if (Result &&
+                  ((Result->isZeroValue() && B == BI->getSuccessor(0)) ||
+                   (Result->isOneValue() && B == BI->getSuccessor(1))))
----------------
It is probably not too interesting to handle the opposite scenario -- the successor sets some value which makes the branch to it more likely .


================
Comment at: test/Analysis/BranchProbabilityInfo/loop.ll:400
+  br label %for.inc
+; CHECK: edge if.then -> for.inc probability is 0x80000000 / 0x80000000 = 100.00% [HOT edge]
+
----------------
no need to check  unconditional branch.


Repository:
  rL LLVM

https://reviews.llvm.org/D35804