[PATCH] D10717: Enhance loop rotation with existence of profile data in MachineBlockPlacement pass.

[David]

davidxl added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:834-838
@@ +833,7 @@
+
+  // We calculate the benefit and cost of a loop rotation respectively, which
+  // are represented by BlockFrequency. When comparing the benefit/cost between
+  // two different rotations, we represent the actual benefit as benefit - cost,
+  // and check (benefit1 - cost1) > (benefit2 - cost2) from (benefit1 + cost2) >
+  // (benefit2 + cost1). Hopefully there is no overflow.
+  BlockFrequency BestRotationBenefit(0);
----------------
congh wrote:
> congh wrote:
> > davidxl wrote:
> > > chandlerc wrote:
> > > > As above, I think it would be useful to have just cost, and to use the natural addition and subtraction to find the minimum cost rotation.
> > > > 
> > > > Also, "hopefully there is no overflow" seems a bad sign. We should *definitely* have no overflow, and I think it makes sense to add asserts and such to enforce that.
> > > Cong is actually computing the overall benefit and maximize it. 
> > > 
> > > Cong, It is equivalent to compute cost only -- simply change it to LoopExternalConnectionCost. For rotation with positive benefit in current patch, the connection cost will be 0, while for rotation with no benefit, the connection cost will be the branch freq to the head (or branch out freq)
> > The arithmetic operation defined in BlockFrequency already considers overflow, but it may return the saturated result, which may not be what we want. I may have to use the similar way that is used in getSumForBlock() to prevent potential overflow.
> David, I am not clear how LoopExternalConnectionCost is working: do we only need this cost, or do we have other cost/benefit? I ask this because there may exist more than one rotation that is beneficial.
For each loop rotation, the entry-connection cost will be the sum of  frequencies (multiplied by misfetch cost) of all incoming edges that are not 'fall through' edges. Similarly, the exit-connection cost can be computed.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:915
@@ +914,3 @@
+    //   will be put ahead of the other one. Assume the frequency of those two
+    //   branches are x and y (x >= y), then the cost will be (x * MisfetechCost
+    //   + y * JumpInstCost) * tail node frequency.
----------------
congh wrote:
> davidxl wrote:
> > I don't think this formula is correct. The correct formula is
> > 
> > (x-x')*MisfetchCost + y*JumpInstrCost
> > 
> > Where x' is the frequency of the edge from the tail node to the block which is not its layout successor (before the rotation).
> We don't have to compare each rotation with the current layout before rotation. We just calculate its own cost. Your formula is equivalent to the current one: you subtract x' * MisfetchCost from every rotation (so the layout before rotation is x' - x' = 0 here).
What is needed is the cost introduced due to the rotation, however my version of the cost function is indeed incorrect. We can actually prove it is the following:

(x*MisFetchCost + min(x,y)*JumpInstCost)*TailNodeFreq

Note that this is slightly different from yours -- x is not the larger of the probabilities, but the original fall through probability. The second term is the same.

Suppose before the rotation, the layout is
... BB1,... TailBB, BB2

where TailBB is ended with 'jcc BB1'. The fall through (not taken) prob is x, and taken probability is y.

After rotation, the layout is
BB2, ... BB1, .. TailBB.  

There are two cases:
1) x > y. The jump instructions added to TailBB will be
   jcc' BB2 followed by direct jump: jmp BB1

Rotation cost will be: x*MisFetchCost + y*MisFetchCost + y *JumpInstCost - y*MisFetchCost = x*MisFetchCost + y * JumpInstCost

2) x < y, The jump instructions added will be
 jcc BB1 + jmp BB2

rotation cost is: x * MisFetchCost + x*JumpInstCost

Combine 1) and 2), 




http://reviews.llvm.org/D10717