[PATCH] D28583: CodeGen: Allow small copyable blocks to "break" the CFG.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1059
+      if (SuccProb > BestProb && TailDupPlacement && shouldTailDuplicate(Succ))
+        DupCandidates.push_back(std::make_tuple(SuccProb, Position, Succ));
       continue;
----------------
Remove the first 'SuccProb > BestProb' check -- it provides only very tiny compile time win depending on the iteration order, but adds more confusion.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1076
+    BestSucc.BB = Succ;
+    BestSucc.ShouldTailDup = false;
     BestProb = SuccProb;
----------------
 no need to set ShouldTailDup in the loop -- it is already initalized outside.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1086
+  if (DupCandidates.size() != 0)
+    std::make_heap(DupCandidates.begin(), DupCandidates.end());
+  while (DupCandidates.size() != 0) {
----------------
Why not just stable sort it? The vector should be of size 1 for most of the cases.  Also why do you need position ?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1096
+        std::make_pair(BestProb, BestPosition))
+      continue;
+    if (canTailDuplicateUnplacedPreds(BB, Succ, Chain, BlockFilter)
----------------
Should it break instead?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1098
+    if (canTailDuplicateUnplacedPreds(BB, Succ, Chain, BlockFilter)
+        && isProfitableToTailDup(BB, Succ, AdjustedSumProb, Chain,
+                                     BlockFilter)) {
----------------
isProfitableToTailDup assumes the baseline layout does not pick Succ.  The assumption may not be true here as there are other two possibilities:
1) Succ == BestSucc.BB in the base layout
2) BestSucc.BB == null in the base layout (all BB's successors have conflicts).

In such two cases, isProfitable check should probably be skipped (as it is benefitial)


https://reviews.llvm.org/D28583