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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
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.
----------------
iteratee wrote:
> davidxl wrote:
> > 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. 
> The straightline test that you're asking for will be redundant as soon as D20505 lands. (It removes the outlining flags from this file, and verifies the single triangle case by just doing multiple triangles)
> 
> This test is basically as small as possible. It needs a block that:
> a) has predecessors that can accept a tail-duplicated copy
> b) wouldn't be chosen based on some outlining algorithm
> c) has at least 2 successors
> 
> The block then2 has 2 predecessors: else1, and test2
> 
> It has 2 successors: end1 and end2
> and test2 has an alternate path around then2: else2
It may become redundant after D20505 lands, but this patch still needs a simplest test case possible in addition to this one.


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:116
+; CHECK: bl c
+define void @avoidable_test(i32 %tag) {
+entry:
----------------
iteratee wrote:
> davidxl wrote:
> > The term 'avoidable'/'unavoidable' is not well defined and can be confusing. I also suggest add this test case in a different patch.
> The goal of the test is a block that wouldn't be chosen due to some outlining strategy. Do you have a name you prefer?
The name ties to much to the implementation details of outlining heuristics. Just change the function name to something like tail_dup_test or something and add a comment that test2 has an alternate path around then2 to disable outllining.  Perhaps add a check to make sure outlining does not happen?


https://reviews.llvm.org/D27742