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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:836
+      ++PredCount;
+      if (SeenPreds.count(SuccPred))
+        continue;
----------------
iteratee wrote:
> davidxl wrote:
> > merge this with the insert before?
> It can't, we have to count those predecessors, we just don't have to check them again.
Can you move insert call down after ++PreCount. If insertion returns false, continue.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:817
+/// predecessors of all of its successors have the same successors as BB. We
+/// ignore lattices where BB doesn't have 2 successors because for fewer than 2,
+/// it's trivial, and for 3 or greater they are very uncommon and complex to
----------------
Since lattice with triangle is handled, the definition of lattice here is no longer accurate -- some predecessor does not have the same successors as BB.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:847
+        continue;
+      if (!hasSameSuccessors(*SuccPred, Successors))
+        return false;
----------------
how does this handle triangle case? In the example, S2 is predecessor of S1, but does not have the same successors of BB.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:880
+  // Collect the edge frequencies of all edges that form the lattice.
+  for (auto Succ : ViableSuccs) {
+    for (MachineBasicBlock *SuccPred : Succ->predecessors()) {
----------------
It is better to split the BestEdges into two BestIncomingEdges vector one for each viable successors and then sort them (or BestOutgoingEdges vectors one for each precessor). This makes the following code much more readable.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:901
+  // Sort the edges, and then for each successor, find the best and second best
+  // incoming predecessor. If the best incoming predecessors don't conflict,
+  // then that is clearly the best layout. If there is a conflict, one of the
----------------
don't conflict (aka sharing the same successor)


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:912
+  auto SecondBestB = BestEdges.end();
+  for (auto It = BestEdges.begin(); It != BestEdges.end(); ++It) {
+    auto &Edge = *It;
----------------
If BestEdges are split, there is no need to do linear search for the best incoming edge for each successor -- after sorting, they are already accessible.

Split BestEdges according to Predecessor is fine too. Either way, it is easy to detect conflict.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1296
 
+  // if BB is part of a lattice, Use the lattice to determine the optimal
+  // fallthrough edges
----------------
Extract the special handling of lattice code into a helper to make the main flow of the caller cleaner.


https://reviews.llvm.org/D28522