[PATCH] D17625: Do not select EhPad BB in MachineBlockPlacement when there is regular BB to schedule

[Chandler Carruth via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

I still have some unanswered questions here, but I don't think we need to solve them in this patch, it seems like a pretty strict improvement as-is. LGTM.

Also, thanks to David for doing the lion's share of the review here. =]


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:376
@@ -371,3 +375,3 @@
       // loop predecessor count of the destination chain.
-      if (SuccChain.UnscheduledPredecessors > 0 && --SuccChain.UnscheduledPredecessors == 0)
-        BlockWorkList.push_back(*SuccChain.begin());
+      if (SuccChain.UnscheduledPredecessors == 0 || --SuccChain.UnscheduledPredecessors > 0)
+        continue;
----------------
80-columns / clang-format

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:567-580
@@ +566,16 @@
+
+    // For ehpad, we layout the least probable first as to avoid jumping back
+    // from least probable landingpads to more probable ones.
+    //
+    // The goal is to get:
+    //
+    //                 +--------------------------+
+    //                 |                          V
+    // InnerLp -> InnerCleanup    OuterLp -> OuterCleanup -> Resume
+    //
+    // Rather than:
+    //
+    //                 +-------------------------------------+
+    //                 V                                     |
+    // OuterLp -> OuterCleanup -> Resume     InnerLp -> InnerCleanup
+    if (BestBlock && (IsEHPad ^ (BestFreq >= CandidateFreq)))
----------------
This goal makes perfect sense, but I'm not 100% convinced that the probabilities are a really reliable way of achieving it. I don't tihnk you need to solve all of this right away though, maybe just leave a FIXME that we should have a more principled way of ensuring nice layout of nested cleanups?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1151
@@ -1105,3 +1150,3 @@
   for (MachineBasicBlock *LoopBB : LoopBlockSet)
-    fillWorkLists(LoopBB, UpdatedPreds, BlockWorkList, &LoopBlockSet);
+    fillWorkLists(LoopBB, UpdatedPreds, BlockWorkList, EHPadWorkList, &LoopBlockSet);
 
----------------
80-columns / clang-format


http://reviews.llvm.org/D17625