[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:962
+      // Else merge
+      BlockToChain[Succ1]->merge(Succ2, BlockToChain[Succ2]);
+    }
----------------
iteratee wrote:
> davidxl wrote:
> > Why do you need to do chain merging here? The method is supposed to do analysis only.
> Well, I'm open to suggestions, but if we don't merge the triangle edge, it doesn't get chosen. We know it's optimal right now.
> 
> Should we have a src->dest map that saves the other side of lattices and then follow it when we are trying to choose the successor for src?
> That would be cleaner.
ideally this should not happen. If it happens, it means the lattice based cost analysis is not consistent with current heuristic of determine better layout predecessor (which is likely the case).  We should probably enhance that logic in the future?


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1055
+        // We can do this because C already has a profitable fallthrough, namely
+        // D. TODO(iteratee): ignore sufficiently cold predecessors for
+        // duplication and for this test.
----------------
This comment does not seem to be correct. D can not be C's fall through. The lattice analysis has decided that D is the fall-through of BB.

You won't need to check all Preds of Succ either -- only Preds that are potential layout predecessor of Succ. In this case, C can not be Succ's layout predecessor.


https://reviews.llvm.org/D28522