[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:
> > 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.
It is not ideal, but my point still holds -- it is not a good idea to embed chain transformation code inside analysis code. If you want to improve the situation so that the analysis result can be better maintained -- that is fine -- but probably as a diferent patch.


================
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.
----------------
iteratee wrote:
> 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"
For the lattice including BB, C+BB, Succ and D, only when {BestA, BestB} == { BB->D, D->Succ}, will the tail duplication check is called. Does it mean D can not be C+BB's successor?  

What I meant is you can pass 'D' as a parameter to this call and only check if Succ can be tail dup into D.


https://reviews.llvm.org/D28522