[PATCH] D23191: [BranchFolding] Restrict tail merging loop blocks after machine block placement

[Kyle Butt via llvm-commits]

iteratee added a comment.

Mostly I'm fine with this Tidy up the comments and the test.


================
Comment at: lib/CodeGen/BranchFolding.cpp:1001
@@ +1000,3 @@
+
+    // Bail out if IBB is the loop header after the block placement because
+    // -- If merging predecessors that belong to the same loop as IBB, the
----------------
This is tricky to parse. It would be clearer as:
Bail if merging after placement and IBB is the loop header.

Whether IBB is the loop header or not shouldn't be changed by placement.

================
Comment at: lib/CodeGen/BranchFolding.cpp:1006
@@ +1005,3 @@
+    // tail and require more branches. This can be relaxed if
+    // MachineBlockPlacement::findBestLoopTop is more flexable.
+    // --If merging predecessors that do not belong to the same loop as IBB, the
----------------
nit: flexible.

================
Comment at: lib/CodeGen/BranchFolding.cpp:1010
@@ +1009,3 @@
+    // block placement again may make big change to the layout and eliminate the
+    // reasone to do tail merging here.
+    if (AfterBlockPlacement && MLI) {
----------------
reasone

================
Comment at: test/CodeGen/X86/tail-merge-after-mbp.ll:13
@@ +12,3 @@
+; check loop block BB#9 is not merged with BB#5, BB#7
+define i32 @foo(%0* nocapture readonly, i32) local_unnamed_addr {
+; CHECK-LABEL: foo:
----------------
I'm not a fan of undef in tests. They have a tendency to be brittle when the optimizer decides to pick a value for you as it is allowed to do. Please take arguments or call a function for the values.


Repository:
  rL LLVM

https://reviews.llvm.org/D23191