[PATCH] D28522: Codegen: Make chains from lattice-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:962
+      // Else merge
+      BlockToChain[Succ1]->merge(Succ2, BlockToChain[Succ2]);
+    }
----------------
davidxl wrote:
> 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?
I don't agree. We are conservative on purpose in the normal layout. When we find that we have a lattice, the need to be conservative shrinks. I don't expect them to get the same answer. Even if we did, I think it would be wasteful to re-compute the answer.


================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:821
+/// and for 3 or greater they are very uncommon and complex to compute
+/// optimally.
+bool MachineBlockPlacement::isLattice(
----------------
jlebar wrote:
> One thing that's confusing to me is: What exactly is "in" the lattice?
> 
> We say that BB is part of the lattice if its successors form a lattice.  So it sounds like a recursive definition.  But then the criteria for whether BB's successors "form a lattice" is different from the criteria for whether BB is itself in a lattice.
> 
> The other thing I have no intuition for here is, why do we use the mathematical word "lattice" for this shape?  I'm sure there's a good reason, and understanding that might help in general.
I didn't have a good word for it. I also can't find one googling. I'm open to suggestions. Lattice matched my initial intuitions, but really it's a subgraph where all maximal linear matchings are the same size.


================
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.
----------------
davidxl wrote:
> 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.
I reread it. It's correct. D should be the fallthrough successor of (C+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.

I'm not sure what you mean by the above. The comment already says "unplaced predecessors"


https://reviews.llvm.org/D28522