[PATCH] D43256: [MBP] Move a latch block with conditional exit and multi predecessors to top of loop

[Guozhi Wei via Phabricator via llvm-commits]

Carrot added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1991
+        OutBB = *Pred->succ_rbegin();
+      if (LoopBlockSet.count(OutBB))
+        continue;
----------------
davidxl wrote:
> This check is not necessary. See example at https://reviews.llvm.org/F8921829
> 
> If B6 is selected as the new loop top, the fall through frequencies can be increased from 99 to 150.
You are right. This code was used as heuristic when I didn't quantitatively compute the number of fallthrough. Now that we have more precise cost model, it should be removed.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1808
+  BlockFrequency MaxFreq = 0;
+  for (MachineBasicBlock *Pred : Top->predecessors()) {
+    BlockChain *PredChain = BlockToChain[Pred];
----------------
davidxl wrote:
> This part looks complete.  Are all the paths covered by some tests?
New tests added.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1918
+   BlockFrequency Gains = BackEdgeFreq + NewFreq;
+   BlockFrequency Lost = FallThrough2Top + FallThrough2Exit +
+       FallThroughFromPred;
----------------
davidxl wrote:
> The logic looks correct. Are all cases covered by tests? 
A lot of existing test cases are impacted by this patch, and this function is intensively tested by those test cases.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1983
+      // It must have more than 1 predecessors.
+      if (Pred->pred_size() == 1)
+        continue;
----------------
davidxl wrote:
> more generally, you want largest pred edge frequency to be smaller than the new back edge frequency.
Again it is a heuristic before FallThroughGains is implemented. Now it can be removed.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D43256/new/

https://reviews.llvm.org/D43256