[PATCH] D10825: Improvement on computing edge probabilities when choosing the best successor in machine block placement.

[David]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:367
@@ -365,3 +366,3 @@
   uint32_t SumWeight = MBPI->getSumForBlock(BB, WeightScale);
   DEBUG(dbgs() << "Attempting merge from: " << getBlockName(BB) << "\n");
   for (MachineBasicBlock *Succ : BB->successors()) {
----------------
Do SumWeight adjustment here -- outside the main loop:

uint32_t SumWeight = ...
SumWeight = AdjustWeightSumExcludingInternalBackEdge(SumWeight, BB);

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:386
@@ +385,3 @@
+    // when BB belongs to a loop but we are building chains for another loop
+    // (either outer or peer).  In this case, the edge from BB to the successor
+    // outside of the inner loop usually has a small probability, due to edges
----------------
Do you have examples of the problem when chain formation is for a 'peer' loop?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:393
@@ +392,3 @@
+    auto SuccLoop = MLI->getLoopFor(Succ);
+    if (L && L != SuccLoop && !(SuccLoop && L->contains(SuccLoop)))
+      for (auto SuccBB : BB->successors())
----------------
need to check SuccLoop !=0 ?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:393
@@ +392,3 @@
+    auto SuccLoop = MLI->getLoopFor(Succ);
+    if (L && L != SuccLoop && !(SuccLoop && L->contains(SuccLoop)))
+      for (auto SuccBB : BB->successors())
----------------
davidxl wrote:
> need to check SuccLoop !=0 ?
It is more readable to move the Sumweight adjustment loop outside the main for loop and probably in a helper function.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:397
@@ -382,1 +396,3 @@
+          SumWeight2 -= MBPI->getEdgeWeight(BB, SuccBB) / WeightScale;
+    BranchProbability SuccProbInLoop(SuccWeight / WeightScale, SumWeight2);
 
----------------
No need to change SuccProb to SuccProbInLoop. 

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:402
@@ -385,3 +401,3 @@
     // successors must be optional. Don't do this for cold branches.
-    if (OutlineOptionalBranches && SuccProb > HotProb.getCompl() &&
+    if (OutlineOptionalBranches && SuccProbInLoop > HotProb.getCompl() &&
         UnavoidableBlocks.count(Succ) > 0) {
----------------
No need for var name change.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:434
@@ -417,2 +433,3 @@
       // important predecessor.
+      BranchProbability SuccProb(SuccWeight / WeightScale, SumWeight);
       BlockFrequency CandidateEdgeFreq =
----------------
Change SuccProb to RealSuccProb


http://reviews.llvm.org/D10825