[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:
> > > 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.
> If the successor of BB is not in the same loop as BB but matches the condition I listed, there are two cases:
> 1) the succBB is in an inner loop of BB's loop, in this case we want to skip this SuccBB
> 2) the succBB is in the current loop being laid out.  If succBB satisfies the condition, this block must be a hotter block that is laid out before 'BB' with top-order broken. In this case, edge weight (BB->SuccBB) should also be skipped -- this is not any different from the backedge case. 
Now I understand your suggested simple loop: you are putting that simple loop before the outer for loop instead of inserting the if statement inside of it.

Considering the case 2 above, I think now it makes more sense to hoist this sum weight update outside of the loop.

I will update the patch accordingly. Thanks for the advice!



http://reviews.llvm.org/D10825