[PATCH] D20017: Aggressive choosing best loop top

[Ehsan Amiri via llvm-commits]

amehsan added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:726-742
@@ -719,2 +725,19 @@
 /// header is always profitable.
+///
+/// a latch block with conditional exit, and similar to following cfg:
+///
+///            entry               original    better
+///              |                  layout     layout
+///  ------> loop.header (body)    --------    ------
+///  |97%    /       \             entry       entry
+///  |      /50%      \50%         loop.header latch
+///  --- latch <--- if.then        if.then     loop.header
+///         \    97%  /            latch       if.then
+///          \3%     /3%           loop.end    loop.end
+///           loop.end
+///
+/// "original layout" cause latch needs a branch jumping back to loop.header
+/// when condition is true, but in "better layout", latch can fall through
+/// loop.header without this jump.
+
 MachineBasicBlock *
----------------
cycheng wrote:
> amehsan wrote:
> > amehsan wrote:
> > > Could this hurt performance for the following example
> > > 
> > > ```
> > > 
> > >      --------->entry
> > >     |  |         |
> > >     |  |    loop header
> > >     |  |         |    \
> > >     |  |         |      \
> > >     |  ---- --Latch1<----- if.then
> > >     |           |           |
> > >     ----------Latch2 <------
> > > 
> > > Probabilities that matter: loop.heaer--->if.then 99%,  if then->Latch 1 99%, Latch1 ---> header 80%
> > > ```
> > > 
> > > The problem is that before your change the branch from if.then to Latch 1 is fall thru. After your change it is a jump back. If we have a single latch this shouldn't hurt the performance, but with multiple latches, the number of fall-thrus that your code, removes is more than those that it creates. I am not sure how important this is.
> > > 
> > > Checking for a single latch should be straightforward using LoopInfo API if this is really a problematic edge case.
> > > 
> > In the graph that I have drawn, arrows from Latch1 and Latch2 should go back to loop header. I hope it was not confusing.
> Thanks Ehsan for pointing out this!
> 
> I changed latch1 --> loop.header probability to 90%, because my patch checks this probability, if it <= 80%, then skip latch1.
> 
> ```
>          entry
>            |
> +----> loop.header
> |       /      \
> | 90%  /1%      \99%
> +--latch1 <--- if.then
> |     |    99%   |
> |     v 10%      | 1%
> ---latch2 <------+
> 
> original:       this patch:
> 
> latch2          latch1
> loop.header     loop.header
> if.then         if.then
> latch1          latch2      
> ```
> 
> When do the original win:
> loop.header -> if.then -> latch1 -> latch2 -> loop.header
> 
> this patch require two additional branch: 
>   - if.then -> latch1 
>   - latch2 -> loop.header
> 
> When do this patch win:
> loop.header -> latch1 -> loop.header
> 
> I think I should check the probability of loop.header -> latch1, 50% looks like we can still get benefit, consider the two path again, with loop.header -> latch1 = 50%: 
>   # loop.header -> if.then -> latch1 -> latch2 -> loop.header
>     - 0.5 * 0.99 * 0.1 * 1.0 = (path probability) = 0.0495
>     - original:  * 1 (branch) = 0.0495
>     - this patch: * 3 (branch) = 0.1485 
>   # loop.header -> latch1 -> loop.header
>     - 0.5 * 0.90 = (path probability) = 0.45
>     - original: * 2 (branch) = 0.9
>     - this patch: * 1 (branch) = 0.45
> 
> Right?
> 
The question is how much we can rely on these numbers? The probabilities are chosen statically by the compiler (or under FDO they come from training workload). So we might have probabilities during compilation that allow us to perform the code change, but real probabilities during execution may differ, and cause slow down.

For single latch case, your change is very robust, and does not cause degradation, even if actual probabilities differ from our static assumptions. But for multiple latch case this is not true. 

There is still one possibility: we may want to accept the risk of degradation in some cases to have improvement in other cases. 

Personally, I prefer to be conservative and disable it for multiple latches, unless some reason for accepting risky change is given. But I may be wrong.


http://reviews.llvm.org/D20017