[PATCH] D20276: [MBP] Reduce code size by running tail merging in MBP

[Amaury SECHET via llvm-commits]

deadalnix added a comment.

I've noticed this bad pattern in my code, thanks for tackling this.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1350
@@ -1339,1 +1349,3 @@
+
+  return isBrUpdated;
 }
----------------
This is a bit confusing as this can return false in cases where basic blocks are scrambled but no branch was updated. Not that it is incorrect, but it goes against conventions. Also are we sure that this is the only cases that can crate opportunity for the optimization to take place ? Have you tried to just run the optimization every time ? I'd be interested to know if there is any changes.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1467
@@ -1454,3 +1466,3 @@
   MBPI = &getAnalysis<MachineBranchProbabilityInfo>();
-  MBFI = &getAnalysis<MachineBlockFrequencyInfo>();
+  MBFI = new MBFIWrapper(getAnalysis<MachineBlockFrequencyInfo>());
   MLI = &getAnalysis<MachineLoopInfo>();
----------------
Maybe you could use a unique pointer here.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1479
@@ +1478,3 @@
+  // TailMerge can create jump into if branches that make CFG irreducible for
+  // HW that requires structurized CFG.
+  bool EnableTailMerge = !F.getTarget().requiresStructuredCFG() &&
----------------
Out of curiosity, do you have example of hardware doing this ?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1496
@@ +1495,3 @@
+    }
+  }
+
----------------
Worth looping maybe ?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1518
@@ -1482,2 +1517,3 @@
 
+  delete MBFI;
   // We always return true as we have no way to track whether the final order
----------------
Remove once you use unique ptr.


Repository:
  rL LLVM

http://reviews.llvm.org/D20276