[PATCH] D28522: Codegen: Make chains from trellis-shaped CFGs

[Kyle Butt via Phabricator via llvm-commits]

iteratee marked an inline comment as done.
iteratee added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:895
+  std::stable_sort(Edges[0].begin(), Edges[0].end(), Cmp);
+  std::stable_sort(Edges[1].begin(), Edges[1].end(), Cmp);
+  auto BestA = Edges[0].begin();
----------------
jlebar wrote:
> It looks like you don't actually care about anything other than the first two elements of this stable_sort?
> 
> Last time I checked, std::stable_sort was relatively slow, and prone to allocate heap memory.  If this is hot, you may want to avoid std::stable_sort.  std::nth_element will sort of do what you want, except it doesn't seem to be stable.  You may do better with a custom "GetTopTwo" function.
> 
> Or maybe that's a premature optimization.  :)
I don't expect the lists to be large in practice, so I'll just use stable_sort for now. It's simple enough to revisit if it becomes a bottleneck.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:919
+/// Get the best successor from \p BB based on \p BB being part of a trellis.
+/// We only handle trelliss with 2 successors, so the algorithm is
+/// straightforward: Find the best pair of edges that don't conflict. We find
----------------
jlebar wrote:
> trellises?  Now I'm not sure if this a typo or not, but I can't find "trelliss" as the plural in any dictionary I have onhand.
I messed up find and replace. It's fixed now.


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:254
+; CHECK-LABEL: lattice_test
+; The most important edge here is f->ret. It's twice as hot as the rest
+; We should also see:
----------------
davidxl wrote:
> Is this comment relevant here?
Yes.


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:275
+  %tagbits.a.eq0 = icmp eq i32 %tagbits.a, 0
+  br i1 %tagbits.a.eq0, label %c, label %b, !prof !2 ; balanced
+c:
----------------
davidxl wrote:
> Using non-equal branch probability here to make the result more obvious in different scenarios:
> 
> 1. there are conflict in best incoming edges
> 2. there are no conflict etc.
The test is now larger. It handles conflicting incoming edges, and a couple of non-conflicting edges, and a triangle.


================
Comment at: test/CodeGen/PowerPC/tail-dup-layout.ll:305
+ret:
+  ret void
+}
----------------
davidxl wrote:
> We probably also need a test for trellis+triangle shape (without taildup)
f->ret is a triangle edge.

I'll make the test bigger with non-balanced edges to cover more scenarios.


https://reviews.llvm.org/D28522