[PATCH] D18226: Codegen: Tail-duplicate during placement.

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: lib/CodeGen/BranchFolding.cpp:595
@@ -594,3 +594,3 @@
 static bool
 ProfitableToMerge(MachineBasicBlock *MBB1, MachineBasicBlock *MBB2,
                   unsigned minCommonTailLength, unsigned &CommonTailLen,
----------------
Since you are here, please document arguments to this method: MBB1, MBB2, PredBB, SuccBB, AfterPlacement.

================
Comment at: lib/CodeGen/BranchFolding.cpp:624
@@ -622,2 +623,3 @@
     unsigned NumTerms = CountTerminators(MBB1 == PredBB ? MBB2 : MBB1, I);
-    if (CommonTailLen > NumTerms)
+    // Don't undo tail-duplication during layout.
+    if (!AfterPlacement && CommonTailLen > NumTerms)
----------------
Expand the comment a little more: TailMerging invoked after block placement. Do not undo ...

================
Comment at: lib/CodeGen/BranchFolding.cpp:648
@@ +647,3 @@
+  // duplicated.
+  if (!AfterPlacement)
+    if (SuccBB && MBB1 != PredBB && MBB2 != PredBB &&
----------------
Is there a better way to identify Pred BBs which are tailduplicated into during block placement? Looks to me we should avoid undoing any taildups done in layout stage.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:512
@@ -509,3 +511,3 @@
 /// Determine if it is profitable to duplicate this block.
 bool TailDuplicator::shouldTailDuplicate(const MachineFunction &MF,
                                          bool IsSimple,
----------------
Document new parameters. While you are here, document IsSimple as well.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:741
@@ -727,2 +740,3 @@
                                    MachineBasicBlock *TailBB,
+                                   MachineBasicBlock *ForcedLayoutPred,
                                    SmallVectorImpl<MachineBasicBlock *> &TDBBs,
----------------
Same here -- document parameter.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:847
@@ -827,2 +846,3 @@
   // block, which falls through unconditionally, move the contents of this
-  // block into the prior block.
+  // block into the prior block. Don't do this when ForcedLayoutPred is
+  // non-null, as it can break layout to remove blocks.
----------------
Are there other places in taildup where blocks may get removed? It seems to me that tailDupllicator needs to have member to remember it is in 'layout' state, and the block deletor code should check that flag instead.


http://reviews.llvm.org/D18226