[PATCH] D27742: CodeGen: Allow small copyable blocks to "break" the CFG.

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:568
+
+  if (!IsSimple && BB->succ_size() == 1)
+    return false;
----------------
davidxl wrote:
> iteratee wrote:
> > davidxl wrote:
> > > && --> || ?
> > > 
> > > In other words, do we care about block dup of simple blocks in layout mode?
> > Simple blocks contain only an unconditional jump. I don't see a good reason to exclude them during layout.
> Basically you are trying to tail duplicate such simple blocks in layout -- what is the benefit? It does not create more fall-through opportunities.
Should the condition be just 

if (BB->succ_size() == 1) 
  return false;

If not, can you come up with a test case showing tail-dup simple BB helps the layout?


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:97
 
+; The block then2 is not unavoidable, but since it can be tail-duplicated, it
+; should be placed as a fallthrough from test2 and copied.
----------------
The intention of this test case is not clear. I expect a simpler and straightforward test case for instance just a control flow with two concatenated triangles.  The first triangle branch is annotated with profile data such that without this patch it will layout the blocks in topo order. 


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:116
+; CHECK: bl c
+define void @avoidable_test(i32 %tag) {
+entry:
----------------
The term 'avoidable'/'unavoidable' is not well defined and can be confusing. I also suggest add this test case in a different patch.


https://reviews.llvm.org/D27742