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

[Kyle Butt via Phabricator via llvm-commits]

iteratee marked 4 inline comments as done.
iteratee added inline comments.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:847
+        continue;
+      if (!hasSameSuccessors(*SuccPred, Successors))
+        return false;
----------------
davidxl wrote:
> how does this handle triangle case? In the example, S2 is predecessor of S1, but does not have the same successors of BB.
There was a merge mistake where it disappeared, sorry. I put it back.


================
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()) {
----------------
davidxl wrote:
> 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.
Good catch, that is simpler.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1296
 
+  // if BB is part of a lattice, Use the lattice to determine the optimal
+  // fallthrough edges
----------------
davidxl wrote:
> Extract the special handling of lattice code into a helper to make the main flow of the caller cleaner.
It already is in 2 helpers. Ignoring the logging the logic is:
if (isLattice)
  return getBestLattice();

I don't see how a helper would be useful in this case.


https://reviews.llvm.org/D28522