[PATCH] D18226: Codegen: Tail-duplicate during placement.

[Kyle Butt via llvm-commits]

iteratee added inline comments.

================
Comment at: lib/CodeGen/MachineBlockPlacement.cpp:1910
@@ +1909,3 @@
+                << getBlockName(Pred) << " -> "
+                << getBlockName(BB) << " -> "
+                << getBlockName(NewSucc) << "\n");
----------------
davidxl wrote:
> iteratee wrote:
> > davidxl wrote:
> > > There are two problems here
> > > 1) many Pred (other than LPred) has been skipped as illegal candidates, but tailDupiicator does not know about will happily do tailDup -- as the tailDuplicator does not know about the new constraints from MBP
> > > 2) if the total number of candidate predecessors that BB can tailDuped into is fewer than 2, there is no point do the tail duplication
> > I think you misunderstand the point of this loop. The point is not to find "legal candidates", but to find the candidates for which duplicating will require update.
> > 
> > Because of that, we don't have an accurate count, and don't want one. We'll gladly let tailDuplicator do its job.
> Ok.
> 
> Do you see the possibility that some Pred simply can not be selected as dup target bb (for legality or performance reasons)? Overall, the code should be organizied in the way that the list of BBs that MBP thinks it can/should be tailDup should match the actual transformations.  
> 
> 
I can see that we might want to limit the set of predecessors duplicate to. A good case would be to filter out predecessors that are too cold.

The performance results have been reasonable so far without that change, and It would certainly make this more complicated.
For now, I think it would be best to simply go with the cleaner version of getting a list of blocks back from tailDuplicateAndUpdate.

This will make it easy to add filters later, and to add them inside of tailDuplicator as well without worrying about breaking this loop.


https://reviews.llvm.org/D18226