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

[Cong Hou]

congh 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()) {
----------------
davidxl wrote:
> congh wrote:
> > davidxl wrote:
> > > Do SumWeight adjustment here -- outside the main loop:
> > > 
> > > uint32_t SumWeight = ...
> > > SumWeight = AdjustWeightSumExcludingInternalBackEdge(SumWeight, BB);
> > The adjustment on SumWeight is done only when it has a successor outside of its loop that is also in BlockFilter (if the successor outside of the loop is not in BlockFilter, we are building chains for the loop). It makes more sense to calculate this adjusted weight inside of this loop. Note that when we build chains for the loop, we don't need this adjusted weight at all.
> I think the adjustment can be done without even needing to have a top level for loop like
> 
> for (auto Succ: BB->successors()) {
>    if (...) continue;
>    for (auto SuccBB : BB->successors())
> 	​        if (MLI->getLoopFor(SuccBB) == L)
> 		​          SumWeight2 -= MBPI->getEdgeWeight(BB, SuccBB) / WeightScale;
>     ..
> }
> 
> but simply:
> 
> 
>  for (auto SuccBB : BB->successors())
>         // We find out that SuccBB is already in 'Chain', but it is not the end of 'Chain' -- it must be a block in the inner 
>         // loop including BB that has already be laid out. Skip it 
>         if (BlockToChain(SuccBB) == &Chain)   
> 		​   SumWeight -= MBPI->getEdgeWeight(BB, SuccBB) / WeightScale;
But if the first successor in BB->successors() is not in the same loop as BB, we won't get the correct SumWeight in this way.

As most CFG nodes have at most two outgoing edges, we won't get many repeated computations.

================
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
----------------
congh wrote:
> davidxl wrote:
> > Do you have examples of the problem when chain formation is for a 'peer' loop?
> The following partial CFG shows such a case:
> 
> ```
> A
> ⇵
> B
> ↓
> C
> ⇵
> D
> ```
> 
> Here, B->C is an edge between two peer loops.
I think my comment here is incorrect. In the case above, we are not building chains for the peer loop but the outer loop, while the successor belongs to a peer loop. I will correct the comment.

================
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
----------------
davidxl wrote:
> congh wrote:
> > congh wrote:
> > > davidxl wrote:
> > > > Do you have examples of the problem when chain formation is for a 'peer' loop?
> > > The following partial CFG shows such a case:
> > > 
> > > ```
> > > A
> > > ⇵
> > > B
> > > ↓
> > > C
> > > ⇵
> > > D
> > > ```
> > > 
> > > Here, B->C is an edge between two peer loops.
> > I think my comment here is incorrect. In the case above, we are not building chains for the peer loop but the outer loop, while the successor belongs to a peer loop. I will correct the comment.
> Ok.  A test case would be good.
OK. Will add such a case.


http://reviews.llvm.org/D10825