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

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

For all the test cases update, please also validate if they make sense or not if possible.



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


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1918
+   BlockFrequency Gains = BackEdgeFreq + NewFreq;
+   BlockFrequency Lost = FallThrough2Top + FallThrough2Exit +
+       FallThroughFromPred;
----------------
The logic looks correct. Are all cases covered by tests? 


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1977
+    if (Pred->succ_size() == 2) {
+      // The candidate should have an exit edge.
+      OutBB = *Pred->succ_begin();
----------------
The comment should be fixed.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1983
+      // It must have more than 1 predecessors.
+      if (Pred->pred_size() == 1)
+        continue;
----------------
more generally, you want largest pred edge frequency to be smaller than the new back edge frequency.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1985
+        continue;
+    } else
+      // OldTop is the only successor of Pred.
----------------
The 'else' here seems wrong. It needs to fall through to do the same check.


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

https://reviews.llvm.org/D43256