[PATCH] D20017: Aggressive choosing best loop top

[Chuang-Yu Cheng via llvm-commits]

cycheng 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 *
----------------
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?



http://reviews.llvm.org/D20017