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

[Justin Lebar via Phabricator via llvm-commits]

jlebar accepted this revision.
jlebar added a comment.
This revision is now accepted and ready to land.

With some changes this looks good for the part I was asked to review.



================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:832
+/// successors form the lower part of a trellis. A successor set S forms the
+/// lower part of trellis if all of the predecessors of S are either in S or
+/// have all of S as successors. We ignore trelliss where BB doesn't have 2
----------------
a trellis


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:833
+/// lower part of trellis if all of the predecessors of S are either in S or
+/// have all of S as successors. We ignore trelliss where BB doesn't have 2
+/// successors because for fewer than 2, it's trivial, and for 3 or greater they
----------------
trellises


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:851
+
+  for (auto Succ : ViableSuccs) {
+    int PredCount = 0;
----------------
Nit, auto*, or even just write out the type?  "for (auto foo : bar)" is scary because it looks like you might be copying a nontrivially-sized object.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:859
+          BlockToChain[SuccPred] == &Chain ||
+          BlockToChain[SuccPred] == BlockToChain[Succ])
+        continue;
----------------
Do you want to avoid the double map lookup on SuccPred here?  This function looks hot.


================
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();
----------------
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.  :)


================
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
----------------
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.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:935
+                                                       BB->succ_end());
+  SmallVector<SmallVector<WeightedEdge, 8>, 2> Edges(2);
+
----------------
Nit, I'd move this down to under the comment that says "Collect the edge frequencies".  Especially since the "2" in the constructor only makes sense after the if statement below.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:940
+  // than 2 successors is fairly uncommon, and a trellis is basically
+  // non-existent.
+  if (Successors.size() != 2 || ViableSuccs.size() != 2)
----------------
Maybe "and a trellis of that size is basically unheard of"?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:963
+  SmallVector<WeightedEdge, 2> BestEdges;
+  getBestNonConflictingEdges(BB, Edges, BestEdges);
+  auto BestA = BestEdges[0];
----------------
Any reason you don't want to return a SmallVector from getBestNonConflictingEdges?  Or even an std::pair?  Then you could say

  WeightedEdge BestA, BestB;
  std::tie(BestA, BestB) = getBestNonConflictingEdges(BB, Edges);

which seems closer to what you mean.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:982
+    auto Succ1 = BestA.Dest;
+    auto Succ2 = BestB.Dest;
+    // Check to see if tail-duplication would be profitable.
----------------
Personally I'd prefer not to use "auto" here.  MachineBasicBlock* isn't too much to type out, and otherwise there's no obvious, nearby anchor for the reader as to what's going on.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1334
 
+  // if we already precomputed the best successor for BB as part of a trellis we
+  // saw earlier, return that if still applicable.
----------------
Nit, capital letter


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1341
+    BlockChain *SuccChain = BlockToChain[Succ];
+    if (BB->isSuccessor(Succ) && (!BlockFilter || BlockFilter->count(Succ)) &&
+        SuccChain != &Chain && Succ == *SuccChain->begin()) {
----------------
This is unrelated to your patch, so you don't need to change it or anything, but if we always check `!BlockFilter || BlockFilter->count(Foo)`, then shouldn't we just pass BlockFilter by reference and initialize it to an empty map when necessary?  This would be much more ergonomic.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1348
+
+  // if BB is part of a trellis, Use the trellis to determine the optimal
+  // fallthrough edges
----------------
Capital "i", lower-case "U", period.


https://reviews.llvm.org/D28522