[PATCH] D11662: Filter cold blocks off the loop chain when profile data is available.

[Cong Hou]

congh marked 2 inline comments as done.
congh added a comment.

In http://reviews.llvm.org/D11662#215634, @davidxl wrote:

> How do you come up with the 5/1 ratio?  Why not making it 1/1? Here are the reasons


It complies with the ratio that is used in selectBestSuccessor(): when the loop has only one iteration, this will lead to similar result if we treat the loop as a non-loop by removing back edges.

> 1. if the loop has high trip count, a block that executed fewer than 1 time per loop does not have any cache reuse


The cold blocks that are not filtered off will be placed to the end of the loop chain (or beginning after loop rotation). So for loop itself the icache locality is still good as all hot blocks are still put together. When connecting those unfiltered cold blocks to blocks outside of the loop, we can use the similar logic that is used in selectBestSuccessor(): we follow CFG constraints when the block is not too cold (>20%).

> 2. if the loop has very short trip count, it is likely that the loop won't be rotated. Splitting the cold block out increases the chances to connect loop with the exit BB with a fallthrough ..


In an extreme situation, if the loop has only one iteration, and if we use 1/1 as the threshold, it seems that too many  blocks in the loop will be filtered off.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:825
@@ +824,3 @@
+
+    const BranchProbability ColdProb(1, 5); // 20%
+    for (MachineBasicBlock *LoopBB : L.getBlocks()) {
----------------
davidxl wrote:
> Overloading BranchProbablity for the purpose of computing ratios is confusing IMO. Why not just 
> 
> const unsigned LoopToColdBBRatio = 5;
At first I think this may be more flexible as we can choose other threshold like 2/5. But now I prefer your suggestion. This can also prevent the potential overflow when converting uint64 to uint32.


http://reviews.llvm.org/D11662