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

[Kyle Butt via llvm-commits]

iteratee added a comment.

I have some questions, but most of the suggestions are done.


================
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
----------------
davidxl wrote:
> On what platforms? Is there a better way to query it?
I believe ARM marks the actual return as an indirect branch.
I don't see a better way. Indirect tail-calls will likely get caught here as well.
Since what we're looking for is jump tables within the function, checking the successor size seems reasonable.
Would you like me to re-write the comment so that it seems more intentional?

================
Comment at: lib/CodeGen/BranchFolding.cpp:713
@@ +712,3 @@
+          if (Block1.back().isIndirectBranch() && Block1.succ_size() != 0)
+            MinCommonTailLengthInside = 21;
+      }
----------------
davidxl wrote:
> where does 21 magic number come from? Why not just define a special value to signal merging is disabled/skipped for such blocks?
It's one more than 20, from tail-duplication of the same pattern. I'm open to other suggestions as to how to make these two cooperate.

================
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
----------------
davidxl wrote:
> Irrelevant change?
No. With tail-duplication occurring only once, we had to duplicate those blocks when we got the chance. Now with layout-duplication we can wait and duplicate them later.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:572
@@ +571,3 @@
+    // the CFG.
+    HasIndirectbr = back.isIndirectBranch() && TailBB.succ_size() != 0;
+  }
----------------
davidxl wrote:
> Irrelevant change?
No. Because this check is only necessary if you want to expand these blocks later in layout mode.
This change is required by the previous one above, because otherwise it breaks a test.

================
Comment at: lib/CodeGen/TailDuplicator.cpp:575
@@ -544,3 +574,3 @@
 
-  if (HasIndirectbr && PreRegAlloc)
+  if (HasIndirectbr && (PreRegAlloc || LayoutMode))
     MaxDuplicateCount = 20;
----------------
davidxl wrote:
> Is this change needed (handled by previous taildup invocation)?
Yes, there are tests where the loop-latch is a switch. I presume it occurs in the real world as well.


https://reviews.llvm.org/D18226