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

[David Li via llvm-commits]

davidxl added inline comments.

================
Comment at: include/llvm/CodeGen/TailDuplicator.h:58
@@ -52,3 +57,3 @@
               const MachineBranchProbabilityInfo *MBPI,
-              unsigned TailDupSize = 0);
+              MachineLoopInfo *MLI, bool LayoutMode, unsigned TailDupSize = 0);
   bool tailDuplicateBlocks(MachineFunction &MF);
----------------
Document the new parameters and behavior (zero default etc)

================
Comment at: lib/CodeGen/BranchFolding.cpp:699
@@ -698,3 +698,3 @@
   MPIterator HighestMPIter = std::prev(MergePotentials.end());
   for (MPIterator CurMPIter = std::prev(MergePotentials.end()),
                   B = MergePotentials.begin();
----------------
CurMPIter = HighestMPIter; 

Maybe a separate cleanup patch

================
Comment at: lib/CodeGen/BranchFolding.cpp:707
@@ +706,3 @@
+      if (AfterBlockPlacement) {
+        MachineBasicBlock &Block1 = *CurMPIter->getBlock();
+        if (Block1.empty())
----------------
MBB1 can be computed in common path

================
Comment at: lib/CodeGen/BranchFolding.cpp:709
@@ +708,3 @@
+        if (Block1.empty())
+          // On some platforms return instructions are marked as indirect
+          // branches and not as return instructions. Work around this by
----------------
On what platforms? Is there a better way to query it?

================
Comment at: lib/CodeGen/BranchFolding.cpp:713
@@ +712,3 @@
+          if (Block1.back().isIndirectBranch() && Block1.succ_size() != 0)
+            MinCommonTailLengthInside = 21;
+      }
----------------
where does 21 magic number come from? Why not just define a special value to signal merging is disabled/skipped for such blocks?

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:139
@@ +138,3 @@
+// Heuristic for tail duplication.
+static cl::opt<unsigned> TailDuplicateMergeThresholdPlacement(
+    "tail-dup-merge-threshold-placement",
----------------
This name is very confusing. Just name it as TailDupThresholdInLayout. Perhaps document that this is also used to 'force' the tail merge at the high threshold to avoid it undoing tailDup decisions.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1009
@@ +1008,3 @@
+    // Check for that now.
+    if (TailDupPlacement && BestSucc) {
+
----------------
This makes the primary loop body too large. Outline it into its own method.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:545
@@ -534,1 +544,3 @@
 
+  // When tail-duplicating relatively early, we shouldnt' de-normalize loops by
+  // duplicating latches, headers, or loop-predecessors. This interferes with
----------------
Irrelevant change?

================
Comment at: lib/CodeGen/TailDuplicator.cpp:572
@@ +571,3 @@
+    // the CFG.
+    HasIndirectbr = back.isIndirectBranch() && TailBB.succ_size() != 0;
+  }
----------------
Irrelevant change?

================
Comment at: lib/CodeGen/TailDuplicator.cpp:575
@@ -544,3 +574,3 @@
 
-  if (HasIndirectbr && PreRegAlloc)
+  if (HasIndirectbr && (PreRegAlloc || LayoutMode))
     MaxDuplicateCount = 20;
----------------
Is this change needed (handled by previous taildup invocation)?


https://reviews.llvm.org/D18226